Created attachment 197041 [details] [review]
basetransform: delay events we cannot send right away

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

Blocker for 0.10.36 because it causes dropped NEWSEGMENT events in decodebin2 since the capsfilters are added for parsers.

Created attachment 197046 [details] [review]
basetransform: delay events we cannot send right away

Created attachment 197047 [details] [review]
basetransform: delay events when src caps are not set yet

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 ?

Yes but not in basetransform IIRC. Look where the negotiation with downstream happens, IIRC that's all in sink_setcaps()

Created attachment 197057 [details] [review]
basetransform: delay serialized events when src caps are not set yet

> 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.

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

Created attachment 197060 [details] [review]
basetransform: delay serialized events when src caps are not set yet

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

This causes bug #665205 , nonetheless this change is IMO correct.