Created attachment 283102 [details] [review]
videodecoder: Call the child class finish() function on stream changes

Call the subclass finish() function when a new STREAM_START
event is received, otherwise derived decoders (libav) that queue
data internally can output stale data when the new segment is
activated. Calling finish gives them the chance to drain.

Also reset last_timestamp_out on a segment change, as is done
for the other sequence tracking variables, to avoid confusion
over new timestamp timelines when a seamless segment change
happens

Comment on attachment 283102 [details] [review]
videodecoder: Call the child class finish() function on stream changes

We might want to add a drain() vfunc at some point and deprecate finish(). I think this will also cause problems with e.g. gst-omx and androidmedia, as those intercept the EOS event via finish() and then send it manually from finish()

After thinking about this a bit more, I think we should introduce a drain() vfunc that only does that... and deprecate finish() in favor of that. Too much old code we would break otherwise.

Created attachment 283178 [details] [review]
videodecoder: Call the child class finish() function on stream changes

Call the subclass finish() function when a new STREAM_START
event is received, otherwise derived decoders (libav) that queue
data internally can output stale data when the new segment is
activated. Calling finish gives them the chance to drain.

Also reset last_timestamp_out when applying the output segment
change, to avoid decoder confusion over new timestamp timelines when
a seamless segment change happens.

We should merge the last_timestamp_out part now already, the other parts are separate and need some further work.

Committed just that part, and the rest is pending using a new drain() vfunc.

Hey, could we backport videodecoder: Reset last_timestamp_out on new segment (aka 946dc6b09f168a0b5087a612ff25c1493ed44035 on master) to 1.4 ?

Yes

Created attachment 296342 [details] [review]
videodecoder: Add drain() vfunc

drain() is a new vfunc which does what finish() does, while
explicitly requiring the decoder be able to continue processing
data afterward.

I'm not convinced by the need for this new drain() vfunc though - it looks like videodecoder has always called finish() when doing reverse playback - so any elements that assume it means EOS are already broken.

In fact, that was you, Sebastian:

commit 6189847ed055b9e63c498145aa43a65bfb24fa63
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Sun Mar 30 18:26:59 2014 +0200

    videodecoder: Always drain the decoder after a discont group in reverse playback mode

I agree life would have been better if we had a drain() that just drain ;-P I'm
curious though in what calling finish() on stream-start break gst-omx ?

Yes, I made it call finish() in reverse playback because without it was just broken and that's what was available in all subclasses at that point and made things work. Ideally a separate drain() vfunc would be better though, and have finish() only called if there is no implementation of drain().

Created attachment 297571 [details] [review]
updated patch for drain() vfunc

Updated patch which uses drain() for non-EOS flushing/drain when it's implemented, but will call finish() for backwards compat with decoders that don't implement drain()

Review of attachment 297571 [details] [review]:

Looks good to me

::: gst-libs/gst/video/gstvideodecoder.c
@@ +991,3 @@
+    } else {
+      if (decoder_class->drain)
+        ret = decoder_class->drain (dec);

Maybe this should be
if (drain) drain();
else { g_warning("implement drain!"); finish(); }

?

We shouldn't g_warning() on backwards compat behaviour, I think - it'll be too verbose. I'll put a GST_FIXME()

Created attachment 297649 [details] [review]
videodecoder: Add drain() vfunc

drain() is a new vfunc which does what finish() does, while
explicitly requiring the decoder be able to continue processing
data afterward.