GNOME Bugzilla – Bug 709868
Keep still meaningful pending events on FLUSH_STOP
Last modified: 2014-11-17 10:05:26 UTC
Only EOS and segment should be deleted in that case.
Created attachment 256954 [details] [review] audioencoder: Keep still meaningfull pending events on FLUSH_STOP
Created attachment 256958 [details] [review] audioencoder: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case.
Probably also applies to videoencoder, audiodecoder and videoencoder
Review of attachment 256958 [details] [review]: Same comments as for the streamsplitter commit
Created attachment 257310 [details] [review] streamsplitter: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case. https://bugzilla.gnome.org/show_bug.cgi?id=709867
Created attachment 257311 [details] [review] videoencoder: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case.
Created attachment 257312 [details] [review] audiodecoder: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case.
Created attachment 257313 [details] [review] videodecoder: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case.
*** Bug 709867 has been marked as a duplicate of this bug. ***
Created attachment 257314 [details] [review] streamsplitter: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case.
Created attachment 257315 [details] [review] audioencoder: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case.
Review of attachment 257311 [details] [review]: ::: gst-libs/gst/video/gstvideoencoder.c @@ +363,3 @@ + } + g_list_free (priv->current_frame_events); + priv->current_frame_events = NULL; You also need to check the pending events of all previous frames and handle them. There might be queued frames that have some sticky events.
Comment on attachment 257313 [details] [review] videodecoder: Keep still meaningfull pending events on FLUSH_STOP same as for the encoder
Comment on attachment 257312 [details] [review] audiodecoder: Keep still meaningfull pending events on FLUSH_STOP These could also contain non-sticky events. Also you forget to actually free the GList
Comment on attachment 257315 [details] [review] audioencoder: Keep still meaningfull pending events on FLUSH_STOP same as for the decoder
Comment on attachment 257314 [details] [review] streamsplitter: Keep still meaningfull pending events on FLUSH_STOP This could also contain other non-sticky events
Created attachment 257354 [details] [review] streamsplitter: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case.
Created attachment 257355 [details] [review] videoencoder: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case.
Created attachment 257356 [details] [review] audiodecoder: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case.
Created attachment 257357 [details] [review] videodecoder: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case.
Thibault, what's the status of this?
It needs to be reviewed and merged afaict.
I would be surprised if this still applies cleanly... can you check that and also if it works properly with the unit tests (especially the new ones added by Thiago). And then also add a tests for this?
Created attachment 268058 [details] [review] streamsplitter: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case.
Created attachment 268059 [details] [review] videoencoder: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case.
Created attachment 268060 [details] [review] audiodecoder: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case.
Created attachment 268061 [details] [review] audiodecoder: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case.
Created attachment 268062 [details] [review] videodecoder: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case.
Created attachment 268063 [details] [review] audioencoder: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case.
(In reply to comment #23) > I would be surprised if this still applies cleanly... can you check that and > also if it works properly with the unit tests (especially the new ones added by > Thiago). And then also add a tests for this? I uploaded rebased patches and make check passes. What kind of test do you expect for that?
Ideally one that fails before your patches and succeeds afterwards :) If that's not easily possible that would be sad but acceptable too... I think all this crazy event handling in the base classes really needs some tests as it's too easy to break things there :(
Thibault, any news here? Would be nice to get this in before 1.4 :)
I totally forgot about that, I will have a look soon.
Ping again? :)
Created attachment 277276 [details] [review] audioencoder: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case.
Created attachment 277277 [details] [review] streamsplitter: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case.
Created attachment 277278 [details] [review] videoencoder: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case.
Created attachment 277279 [details] [review] audiodecoder: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case.
Created attachment 277280 [details] [review] videodecoder: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case. + Add a testcase
Are we completely sure of that ? TAGs may not be right any-more after flush-stop. CAPS may change, hence if you keep it, you might endup with caps being sent twice, and we'll get yet another configure, cleanup, configure dance. Or do we guaranty they will be updated before being sent again (a bit like what the pad does with sticky I guess).
Nicolas, that's exactly what GstPad is doing too when flushing
Also queue, queue2 and multiqueue do it like that. Without this it can happen that you get data-flow without a caps event if flushing happens while the initial caps event is still somewhere in a pending_events list. commit a91b51c9b7cc7adabfa0cf0153a2e25efd0b6b7c Author: Sebastian Dröge <sebastian@centricular.com> Date: Sat May 31 17:35:52 2014 +0200 typefind: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case. https://bugzilla.gnome.org/show_bug.cgi?id=709868
Attachment 277276 [details] pushed as 967d1fb - audioencoder: Keep still meaningfull pending events on FLUSH_STOP Attachment 277277 [details] pushed as 42ecafe - streamsplitter: Keep still meaningfull pending events on FLUSH_STOP Attachment 277278 [details] pushed as 2843f35 - videoencoder: Keep still meaningfull pending events on FLUSH_STOP Attachment 277279 [details] pushed as 12df7fa - audiodecoder: Keep still meaningfull pending events on FLUSH_STOP Attachment 277280 [details] pushed as d2ea326 - videodecoder: Keep still meaningfull pending events on FLUSH_STOP
Created attachment 277794 [details] [review] capsfilter: Keep still meaningfull pending events on FLUSH_STOP Only EOS and segment should be deleted in that case.
Comment on attachment 277794 [details] [review] capsfilter: Keep still meaningfull pending events on FLUSH_STOP Not sure if this is correct. You're by-passing the basetransform event handling vfunc with that, right?
The difference between the code from that patch and old code is that in old code we were just removing the SEGMENT from the list, now we are removing everything that is not STICKY + SEGMENT + EOS.
Review of attachment 277794 [details] [review]: ::: plugins/elements/gstcapsfilter.c @@ +384,3 @@ + gst_event_unref (tmp->data); + } else { + gst_pad_store_sticky_event (pad, GST_EVENT_CAST (tmp->data)); Not really. You're not filtering the list, you store those events on the pad and then free the list.
(In reply to comment #47) > Review of attachment 277794 [details] [review]: > > ::: plugins/elements/gstcapsfilter.c > @@ +384,3 @@ > + gst_event_unref (tmp->data); > + } else { > + gst_pad_store_sticky_event (pad, GST_EVENT_CAST (tmp->data)); > > Not really. You're not filtering the list, you store those events on the pad > and then free the list. Ah, you are right sorry. Not sure what should be done then, should we just remove EOSes also and be done?
I think so
Created attachment 286877 [details] [review] capsfilter: Remove EOS event from pending_event list on FLUSH_STOP
Comment on attachment 286877 [details] [review] capsfilter: Remove EOS event from pending_event list on FLUSH_STOP Also to 1.4
Hrm, why is that still open? It is not clear to me at this point.
Thibault, it would seem the capsfilter patch marked as needs-work is still outstanding?
(In reply to comment #53) > Thibault, it would seem the capsfilter patch marked as needs-work is still > outstanding? It has been replace and pushed as: commit d1181ed98aa79fd472e90f0324d54655bd67cc65 Author: Thibault Saunier <tsaunier@gnome.org> Date: Tue Jun 3 14:23:30 2014 +0200 capsfilter: Remove EOS event from pending_event list on FLUSH_STOP https://bugzilla.gnome.org/show_bug.cgi?id=709868 Closing for good.