After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 734617 - videodecoder: Call the child class finish() function on stream changes
videodecoder: Call the child class finish() function on stream changes
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal normal
: 1.5.91
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-08-11 14:15 UTC by Jan Schmidt
Modified: 2015-08-28 18:27 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
videodecoder: Call the child class finish() function on stream changes (4.25 KB, patch)
2014-08-11 14:16 UTC, Jan Schmidt
reviewed Details | Review
videodecoder: Call the child class finish() function on stream changes (4.20 KB, patch)
2014-08-12 12:01 UTC, Jan Schmidt
needs-work Details | Review
videodecoder: Add drain() vfunc (2.65 KB, patch)
2015-02-07 18:21 UTC, Jan Schmidt
none Details | Review
updated patch for drain() vfunc (3.42 KB, patch)
2015-02-22 13:37 UTC, Jan Schmidt
accepted-commit_now Details | Review
videodecoder: Add drain() vfunc (3.60 KB, patch)
2015-02-23 14:38 UTC, Jan Schmidt
committed Details | Review

Description Jan Schmidt 2014-08-11 14:15:04 UTC
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 1 Jan Schmidt 2014-08-11 14:16:01 UTC
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 2 Sebastian Dröge (slomo) 2014-08-11 15:22:20 UTC
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()
Comment 3 Sebastian Dröge (slomo) 2014-08-12 08:34:34 UTC
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.
Comment 4 Jan Schmidt 2014-08-12 12:01:12 UTC
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.
Comment 5 Sebastian Dröge (slomo) 2014-08-13 15:18:50 UTC
We should merge the last_timestamp_out part now already, the other parts are separate and need some further work.
Comment 6 Jan Schmidt 2014-08-14 08:20:59 UTC
Committed just that part, and the rest is pending using a new drain() vfunc.
Comment 7 Mathieu Duponchelle 2014-09-18 20:16:59 UTC
Hey, could we backport videodecoder: Reset last_timestamp_out on new segment (aka 946dc6b09f168a0b5087a612ff25c1493ed44035 on master) to 1.4 ?
Comment 8 Sebastian Dröge (slomo) 2014-09-23 16:44:24 UTC
Yes
Comment 9 Jan Schmidt 2015-02-07 18:21:07 UTC
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.
Comment 10 Jan Schmidt 2015-02-07 18:25:25 UTC
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.
Comment 11 Jan Schmidt 2015-02-07 18:34:17 UTC
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
Comment 12 Nicolas Dufresne (ndufresne) 2015-02-07 18:35:45 UTC
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 ?
Comment 13 Sebastian Dröge (slomo) 2015-02-08 08:39:44 UTC
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().
Comment 14 Jan Schmidt 2015-02-22 13:37:25 UTC
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()
Comment 15 Sebastian Dröge (slomo) 2015-02-23 14:25:07 UTC
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(); }

?
Comment 16 Jan Schmidt 2015-02-23 14:29:26 UTC
We shouldn't g_warning() on backwards compat behaviour, I think - it'll be too verbose. I'll put a GST_FIXME()
Comment 17 Jan Schmidt 2015-02-23 14:38:19 UTC
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.