GNOME Bugzilla – Bug 768995
New API: StreamGroupDone event
Last modified: 2016-07-25 09:56:39 UTC
RFC on a new piece of API. This event precedes EOS in situations like decodebin waiting for a pending group to drain before it can activate a new one. decodebin won't send EOS, which means a downstream input-selector won't unblock other pads from the same group. Somewhat related to bug 603378, but the problem isn't the same and Drain isn't the right semantic.
Created attachment 331820 [details] [review] events: Implement the stream-group-done event A new event which precedes EOS in situations where we need downstream to unblock any pads waiting on a stream before we can send EOS. E.g, decodebin draining a chain so it can switch pads.
Created attachment 331821 [details] [review] inputselector: Handle stream-group-done Handle the new stream-group-done message to unblock pads which are waiting for the running time to advance on that group.
Created attachment 331822 [details] [review] events: Implement the stream-group-done event A new event which precedes EOS in situations where we need downstream to unblock any pads waiting on a stream before we can send EOS. E.g, decodebin draining a chain so it can switch pads.
Created attachment 331823 [details] [review] inputselector: Handle stream-group-done Handle the new stream-group-done message to unblock pads which are waiting for the running time to advance on that group.
Created attachment 331824 [details] [review] decodebin: Send stream-group-done to unblock downstream When processing EOS for a pad, send a stream-group-done for the pad in case downstream is waiting for more data on this stream before it can process related streams from the group.
Review of attachment 331822 [details] [review]: Good to go and makes sense to me, just: ::: gst/gstevent.c @@ +668,3 @@ + * new pads can be exposed before sending EOS on the existing pads. + * + * Returns: (transfer full): the new stream-group-done event. Since: 1.10 @@ +687,3 @@ + * + * Parse a stream-group-done @event and store the result in the given + * @group_id location. Since: 1.10 ::: gst/gstevent.h @@ +98,3 @@ + * @GST_EVENT_STREAM_GROUP_DONE: Indicates that there is no more data for + * the stream group ID in the message. Sent before EOS + * in some instances and should be handled mostly the same. Since: 1.10
Review of attachment 331823 [details] [review]: Generally seems good to me ::: plugins/elements/gstinputselector.c @@ +485,3 @@ + * running time, as this pad just switched to EOS and + * may enable others to progress */ + GST_INPUT_SELECTOR_BROADCAST (self); This seems like a separate bugfix that can go into another commit? What does it fix? :) @@ +638,3 @@ + selpad->group_done = TRUE; + if (sel->sync_streams && active_sinkpad == pad) + GST_INPUT_SELECTOR_BROADCAST (sel); It's usually a good idea to take the lock before you change variables and broadcast/signal for those variable changes
Review of attachment 331824 [details] [review]: Same for decodebin3 I guess?
Review of attachment 331822 [details] [review]: ::: gst/gstevent.h @@ +155,3 @@ GST_EVENT_BUFFERSIZE = GST_EVENT_MAKE_TYPE (90, FLAG(DOWNSTREAM) | FLAG(SERIALIZED) | FLAG(STICKY)), GST_EVENT_SINK_MESSAGE = GST_EVENT_MAKE_TYPE (100, FLAG(DOWNSTREAM) | FLAG(SERIALIZED) | FLAG(STICKY) | FLAG(STICKY_MULTI)), + GST_EVENT_STREAM_GROUP_DONE = GST_EVENT_MAKE_TYPE (105, FLAG(DOWNSTREAM) | FLAG(SERIALIZED) | FLAG(STICKY)), As this is sticky, should there be some new code in gstpad.c to remove the event again when a new stream-start is received. Maybe with a different group id?
Created attachment 332088 [details] [review] events: Implement the stream-group-done event A new event which precedes EOS in situations where we need downstream to unblock any pads waiting on a stream before we can send EOS. E.g, decodebin draining a chain so it can switch pads.
Created attachment 332089 [details] [review] inputselector: Handle stream-group-done Handle the new stream-group-done message to unblock pads which are waiting for the running time to advance on that group.
Comment on attachment 332089 [details] [review] inputselector: Handle stream-group-done Looks good to me, just misses your EOS fix now (which I assume you will just push)
Comment on attachment 332088 [details] [review] events: Implement the stream-group-done event Probably want to add the docs to docs/gst/*sections.txt :)
(In reply to Sebastian Dröge (slomo) from comment #7) > Review of attachment 331823 [details] [review] [review]: > > Generally seems good to me > > ::: plugins/elements/gstinputselector.c > @@ +485,3 @@ > + * running time, as this pad just switched to EOS and > + * may enable others to progress */ > + GST_INPUT_SELECTOR_BROADCAST (self); > > This seems like a separate bugfix that can go into another commit? What does > it fix? :) Thanks, you're right - separated out. > @@ +638,3 @@ > + selpad->group_done = TRUE; > + if (sel->sync_streams && active_sinkpad == pad) > + GST_INPUT_SELECTOR_BROADCAST (sel); > > It's usually a good idea to take the lock before you change variables and > broadcast/signal for those variable changes The whole function takes the lock and releases it outside the switch(). (In reply to Sebastian Dröge (slomo) from comment #9) > Review of attachment 331822 [details] [review] [review]: > > ::: gst/gstevent.h > @@ +155,3 @@ > GST_EVENT_BUFFERSIZE = GST_EVENT_MAKE_TYPE (90, > FLAG(DOWNSTREAM) | FLAG(SERIALIZED) | FLAG(STICKY)), > GST_EVENT_SINK_MESSAGE = GST_EVENT_MAKE_TYPE (100, > FLAG(DOWNSTREAM) | FLAG(SERIALIZED) | FLAG(STICKY) | FLAG(STICKY_MULTI)), > + GST_EVENT_STREAM_GROUP_DONE = GST_EVENT_MAKE_TYPE (105, > FLAG(DOWNSTREAM) | FLAG(SERIALIZED) | FLAG(STICKY)), > > As this is sticky, should there be some new code in gstpad.c to remove the > event again when a new stream-start is received. Maybe with a different > group id? Done, in the same places EOS is cleared.
Yeah I saw, all good to go except for the docs :)
Added the 2 new functions to the docs and pushed.