[Bug 708416] collectpads: implement flushing seek support

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=708416
  GStreamer | gstreamer (core) | git

Sebastian Dröge (slomo) <slomo> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #255352|none                        |needs-work
             status|                            |

--- Comment #10 from Sebastian Dröge (slomo) <slomo at circular-chaos.org> 2013-10-03 11:07:32 UTC ---
Review of attachment 255352:
 --> (https://bugzilla.gnome.org/review?bug=708416&attachment=255352)

General looks good, apart from all the other comments:

::: libs/gst/base/gstcollectpads.c
@@ +1306,3 @@
+    if (G_UNLIKELY (g_atomic_int_compare_and_exchange (&pads->priv->seeking,
+                TRUE, FALSE) == TRUE)) {
+      GST_INFO_OBJECT (pads, "finished seeking");

Couldn't we get here although we didn't receive the flush events yet for some
pad? Like when collected was still running while we receive the seek event, or
when we still get buffers on some pads before they do the flushing?

@@ +1674,3 @@
+
+        goto eat;
+      } else {

No need to put the code below in an else block

@@ +1843,3 @@
+event_forward_func (GstPad * pad, EventData * data)
+{
+  data->result &= gst_pad_push_event (pad, gst_event_ref (data->event));

If the pad was flushing and the seek could not be forwarded because of that,
that sounds correct. IMHO the code in adder looks like a hack and only works
around another bug somewhere else.

@@ +1844,3 @@
+{
+  data->result &= gst_pad_push_event (pad, gst_event_ref (data->event));
+  return !data->result;

Well, it doesn't really matter. If one fails we're in an inconsistent state
already and things will fail in interesting ways later :) Might be best to also
do an GST_ELEMENT_ERROR() here or something.

::: libs/gst/base/gstcollectpads.h
@@ +243,3 @@
+ * @user_data: user data
+ *
+ * A function that will be called while processing a flushing seek event.

A flushing seek event? Then maybe call it FlushSeekFunction?

Or flush events? Then it's ok to call FlushFunction :)

@@ +250,3 @@
+ * gst_collect_pads_set_flushing nor gst_collect_pads_clear from this
function.
+ *
+ * Since: FIXME

Should be 1.4 :)

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.