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 768995 - New API: StreamGroupDone event
New API: StreamGroupDone event
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
unspecified
Other Linux
: Normal enhancement
: 1.9.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-07-20 14:15 UTC by Jan Schmidt
Modified: 2016-07-25 09:56 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
events: Implement the stream-group-done event (7.68 KB, patch)
2016-07-20 14:16 UTC, Jan Schmidt
none Details | Review
inputselector: Handle stream-group-done (3.86 KB, patch)
2016-07-20 14:16 UTC, Jan Schmidt
none Details | Review
events: Implement the stream-group-done event (7.68 KB, patch)
2016-07-20 14:23 UTC, Jan Schmidt
accepted-commit_now Details | Review
inputselector: Handle stream-group-done (3.87 KB, patch)
2016-07-20 14:23 UTC, Jan Schmidt
reviewed Details | Review
decodebin: Send stream-group-done to unblock downstream (1.84 KB, patch)
2016-07-20 14:25 UTC, Jan Schmidt
committed Details | Review
events: Implement the stream-group-done event (9.69 KB, patch)
2016-07-25 09:22 UTC, Jan Schmidt
committed Details | Review
inputselector: Handle stream-group-done (3.34 KB, patch)
2016-07-25 09:22 UTC, Jan Schmidt
committed Details | Review

Description Jan Schmidt 2016-07-20 14:15:34 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.
Comment 1 Jan Schmidt 2016-07-20 14:16:29 UTC
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.
Comment 2 Jan Schmidt 2016-07-20 14:16:35 UTC
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.
Comment 3 Jan Schmidt 2016-07-20 14:23:44 UTC
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.
Comment 4 Jan Schmidt 2016-07-20 14:23:50 UTC
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.
Comment 5 Jan Schmidt 2016-07-20 14:25:26 UTC
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.
Comment 6 Sebastian Dröge (slomo) 2016-07-25 06:33:21 UTC
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
Comment 7 Sebastian Dröge (slomo) 2016-07-25 06:39:36 UTC
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
Comment 8 Sebastian Dröge (slomo) 2016-07-25 06:42:09 UTC
Review of attachment 331824 [details] [review]:

Same for decodebin3 I guess?
Comment 9 Sebastian Dröge (slomo) 2016-07-25 06:42:57 UTC
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?
Comment 10 Jan Schmidt 2016-07-25 09:22:04 UTC
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.
Comment 11 Jan Schmidt 2016-07-25 09:22:13 UTC
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 12 Sebastian Dröge (slomo) 2016-07-25 09:27:18 UTC
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 13 Sebastian Dröge (slomo) 2016-07-25 09:27:51 UTC
Comment on attachment 332088 [details] [review]
events: Implement the stream-group-done event

Probably want to add the docs to docs/gst/*sections.txt :)
Comment 14 Jan Schmidt 2016-07-25 09:42:36 UTC
(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.
Comment 15 Sebastian Dröge (slomo) 2016-07-25 09:52:45 UTC
Yeah I saw, all good to go except for the docs :)
Comment 16 Jan Schmidt 2016-07-25 09:55:16 UTC
Added the 2 new functions to the docs and pushed.