GNOME Bugzilla – Bug 785948
audioencoder: Integer overflows in timestamp/granulepos calculation code on discont
Last modified: 2017-08-11 08:06:18 UTC
In gst_audio_encoder_chain() we calculate the difference "diff" between the expected and actual timestamp. > diff = GST_CLOCK_DIFF (next_ts, GST_BUFFER_TIMESTAMP (buffer)); This can sometimes be negative and that's not necessarily a problem. Further below we shift the base_ts by that difference, and that's where things go wrong: > if (discont) { > /* now re-sync ts */ > priv->base_ts += diff; > gst_audio_encoder_set_base_gp (enc); > priv->discont |= discont; > } If base_ts was 0 before (which it usually is), this would cause an integer overflow and suddenly we have a huge timestamp. There seem to be two potential solutions: 1) Make this an error 2) Store base_ts as a signed integer, and make sure that everything is handled correctly with negative values everywhere where it is used As a side-note, it would also make sense to add the "discont-wait" code from e.g. audiobasesink or audiomixer here, to allow a bigger timestamp discontinuity as long as it is not that big for a longer timespan.
Created attachment 357142 [details] [review] audioencoder: also adjust sample count upon discont to avoid ts overflow
When writing the second code-fragment, I was assuming to have enough time-slack to handle the diff. But indeed, the base_ts is usually 0 (or thereabout). However, there is a lot of slack in the sample count, so we could do it as in attached patch. That avoid the tricky-ness of a negative ts, and only errors out if things are really bad. Applying the "discont-wait" could indeed make things a bit less jitterish and align it with audiobasesink (as it was once originally).
Review of attachment 357142 [details] [review]: Looks good to me, please add some further details about what and why and how to the commit message though :) Thanks for the fast patch! ::: gst-libs/gst/audio/gstaudioencoder.c @@ +1330,3 @@ + GST_SECOND, ctx->info.rate * ctx->info.bpf); + + if (shift > GST_BUFFER_TIMESTAMP (buffer)) { I think this can never happen, or can it?
It is very unlikely to happen. It theoretically could if after (say) 5s another buffer would come in with ts 0 while still some other stuff in the adapter. Then I would not want to be caught trying to subtract into the negative, but as said things are already pretty weird for that to happen. But I suppose it's then better to error out than to have more weird stuff happening. A G_UNLIKELY could be sprinkled there for good measure. I shall also endeavour to restrain succinct proclivity ...
Created attachment 357214 [details] [review] audioencoder: also adjust sample count upon discont to avoid ts overflow
Comment on attachment 357214 [details] [review] audioencoder: also adjust sample count upon discont to avoid ts overflow Go for it :)
commit 00fa39befae4076277c9a046f72c9fe8a4275048 Author: Mark Nauwelaerts <mnauw@users.sourceforge.net> Date: Tue Aug 8 20:35:25 2017 +0200 audioencoder: also adjust sample count upon discont to avoid ts overflow Only adjusting the base_ts might lead to a negative ts and as such integer overflow into a huge timestamp which then propagates into the granulepos and so on. Instead, resync to incoming buffer timestamp using both base_ts and sample count rather than only base_ts. Fixes https://bugzilla.gnome.org/show_bug.cgi?id=785948