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 659571 - basetransform: delay events we cannot send right away
basetransform: delay events we cannot send right away
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
unspecified
Other All
: Normal blocker
: 0.10.36
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-09-20 12:04 UTC by Vincent Penquerc'h
Modified: 2011-12-05 08:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
basetransform: delay events we cannot send right away (3.49 KB, patch)
2011-09-20 12:04 UTC, Vincent Penquerc'h
needs-work Details | Review
basetransform: delay events we cannot send right away (3.21 KB, patch)
2011-09-20 12:35 UTC, Vincent Penquerc'h
none Details | Review
basetransform: delay events when src caps are not set yet (3.22 KB, patch)
2011-09-20 12:35 UTC, Vincent Penquerc'h
none Details | Review
basetransform: delay serialized events when src caps are not set yet (4.85 KB, patch)
2011-09-20 13:34 UTC, Vincent Penquerc'h
needs-work Details | Review
basetransform: delay serialized events when src caps are not set yet (4.49 KB, patch)
2011-09-20 14:09 UTC, Vincent Penquerc'h
committed Details | Review

Description Vincent Penquerc'h 2011-09-20 12:04:48 UTC
basetransform: delay events we cannot send right away
Comment 1 Vincent Penquerc'h 2011-09-20 12:04:50 UTC
Created attachment 197041 [details] [review]
basetransform: delay events we cannot send right away
Comment 2 Sebastian Dröge (slomo) 2011-09-20 12:12:38 UTC
Review of attachment 197041 [details] [review]:

::: libs/gst/base/gstbasetransform.c
@@ +368,3 @@
+    GList *tmp;
+    for (tmp = trans->priv->delayed_events; tmp; tmp = tmp->next)
+      gst_event_unref (tmp->data);

g_list_foreach (trans->priv->delayed_events, (GFunc) gst_event_unref, NULL)

@@ +2054,3 @@
+  while (1) {
+    GST_OBJECT_LOCK (trans);
+    tmp = trans->priv->delayed_events;

Just get the list, set trans->priv->delayed_events to NULL, release the lock and then push all events. That's a bit easier

@@ +2097,3 @@
+
+    /* We ref the event to avoid it being dumped if it fails to be forwarded,
+       as we want to keep it for later if so */

This is wrong, you should only delay the events if they're GST_EVENT_IS_SERIALIZED() and if the srcpad caps are NULL. If it fails in other situations we really don't care. Also FLUSH_STOP events should never be delayed but result in dropping all previously delayed events and EOS should be immediately forwarded too.

Also drop all delayed events when going PAUSED->READY

@@ +2546,3 @@
       trans->priv->processed++;
+
+      gst_base_transform_send_delayed_events (trans);

Make sure that we definitely have srcpad caps at this point... just check the code above but iirc that's the case here
Comment 3 Sebastian Dröge (slomo) 2011-09-20 12:13:26 UTC
Blocker for 0.10.36 because it causes dropped NEWSEGMENT events in decodebin2 since the capsfilters are added for parsers.
Comment 4 Vincent Penquerc'h 2011-09-20 12:35:03 UTC
Created attachment 197046 [details] [review]
basetransform: delay events we cannot send right away
Comment 5 Vincent Penquerc'h 2011-09-20 12:35:35 UTC
Created attachment 197047 [details] [review]
basetransform: delay events when src caps are not set yet
Comment 6 Vincent Penquerc'h 2011-09-20 12:36:54 UTC
Gah, forgot to do the dropping, not ready yet...

> @@ +2546,3 @@
>        trans->priv->processed++;
> +
> +      gst_base_transform_send_delayed_events (trans);
> 
> Make sure that we definitely have srcpad caps at this point... just check the
> code above but iirc that's the case here

This is in the chain function, can that be called before negotiation ?
Comment 7 Sebastian Dröge (slomo) 2011-09-20 12:50:00 UTC
Yes but not in basetransform IIRC. Look where the negotiation with downstream happens, IIRC that's all in sink_setcaps()
Comment 8 Vincent Penquerc'h 2011-09-20 13:34:14 UTC
Created attachment 197057 [details] [review]
basetransform: delay serialized events when src caps are not set yet
Comment 9 Vincent Penquerc'h 2011-09-20 13:36:27 UTC
> Make sure that we definitely have srcpad caps at this point... just check the
> code above but iirc that's the case here

Following the flow, if not negotiated, gst_base_transform_handle_buffer should not return GST_FLOW_OK, which will mean we don't get to this part of the code.
So it would seem to be the case.
Comment 10 Sebastian Dröge (slomo) 2011-09-20 13:48:01 UTC
Review of attachment 197057 [details] [review]:

::: libs/gst/base/gstbasetransform.c
@@ +389,3 @@
+{
+  if (transition == GST_STATE_CHANGE_PAUSED_TO_READY) {
+    gst_base_transform_drop_delayed_events (GST_BASE_TRANSFORM (element));

Oh there was no state change function yet. Just do it in gst_base_transform_activate() if activate==FALSE then. That's the same

@@ +2108,3 @@
    * something different. */
+  if (forward) {
+    gboolean delay, caps_set = GST_PAD_CAPS (trans->srcpad) != NULL;

Parenthesis around the boolean expression

@@ +2116,3 @@
+      delay = GST_EVENT_IS_SERIALIZED (event) && !caps_set
+          && GST_EVENT_TYPE (event) != GST_EVENT_EOS;
+    }

Maybe a comment here why we do this at all and why FLUSH_STOP/EOS are handled differently

@@ +2125,3 @@
+    } else {
+      if (caps_set)
+        gst_base_transform_send_delayed_events (trans);

Only if GST_EVENT_IS_SERIALIZED(), otherwise you might send serialized events from a thread different to the streaming thread
Comment 11 Vincent Penquerc'h 2011-09-20 14:09:45 UTC
Created attachment 197060 [details] [review]
basetransform: delay serialized events when src caps are not set yet
Comment 12 Sebastian Dröge (slomo) 2011-09-21 11:25:49 UTC
commit 56b3acb043691809c5a4c4d1b65a76f3c05185bf
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Tue Sep 20 13:04:06 2011 +0100

    basetransform: delay serialized events when src caps are not set yet
    
    https://bugzilla.gnome.org/show_bug.cgi?id=659571
Comment 13 Sebastian Dröge (slomo) 2011-12-05 08:32:28 UTC
This causes bug #665205 , nonetheless this change is IMO correct.