Created attachment 255349 [details] [review]
tests: collectpads: update my email address

Created attachment 255350 [details] [review]
tests: collectpads: tweak stub _collect to push all buffers

Created attachment 255351 [details] [review]
tests: collectpads: add flushing seek tests

Created attachment 255352 [details] [review]
collectpads: implement flushing seek support

Implement common flushing seek logic in GstCollectPads. Add new
API so that elements can opt-in to using the new logic
(gst_collect_pads_src_event_default) and can extend it
(gst_collect_pads_set_flush_function) to flush any internal
state.

See https://bugzilla.gnome.org/show_bug.cgi?id=706779 and
https://bugzilla.gnome.org/show_bug.cgi?id=706441 for the
background discussion.

API: gst_collect_pads_set_flush_function()
API: gst_collect_pads_src_event_default()

Review of attachment 255352 [details] [review]:

There is a wrong update of common/ in there.

::: libs/gst/base/gstcollectpads.c
@@ +1199,3 @@
   if (!GST_COLLECT_PADS_STATE_IS_SET (data, GST_COLLECT_PADS_STATE_LOCKED) &&
       (GST_COLLECT_PADS_STATE_IS_SET (data, GST_COLLECT_PADS_STATE_WAITING) !=
+          !!waiting)) {

Did you run it through gst-indent? It does produce this horror

I did(In reply to comment #5)
> Review of attachment 255352 [details] [review]:
> 
> There is a wrong update of common/ in there.

oops


> ::: libs/gst/base/gstcollectpads.c
> @@ +1199,3 @@
>    if (!GST_COLLECT_PADS_STATE_IS_SET (data, GST_COLLECT_PADS_STATE_LOCKED) &&
>        (GST_COLLECT_PADS_STATE_IS_SET (data, GST_COLLECT_PADS_STATE_WAITING) !=
> +          !!waiting)) {
> 
> Did you run it through gst-indent? It does produce this horror

I did. I can take the change out but is ! !waiting better than !!waiting?

Does this make https://bugzilla.gnome.org/attachment.cgi?id=255151 from bug #706779 obsolete?

(In reply to comment #7)
> Does this make https://bugzilla.gnome.org/attachment.cgi?id=255151 from bug
> #706779 obsolete?

It does. I have a new patch for oggmux that uses this new API. I also did the same for qtmux to sort of validate the API.

Review of attachment 255352 [details] [review]:

Thanks a lot for making that patch, we were beating around the bush in both adder and videomixer when the right way was that way.

::: libs/gst/base/gstcollectpads.c
@@ +1843,3 @@
+event_forward_func (GstPad * pad, EventData * data)
+{
+  data->result &= gst_pad_push_event (pad, gst_event_ref (data->event));

I had a problem here when making videomixer use the new API, it so happened that the pad could be flushing, and we failed sending the seek upstream. Using the solution adder uses, which is to grab the peer and if it exists using send_event on it, the problem was solved.

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

Are we sure we want the forwarding to stop when sending the event fails ? What if the source element is a live source ? We might want to handle that case ?

::: libs/gst/base/gstcollectpads.h
@@ +250,3 @@
+ * gst_collect_pads_set_flushing nor gst_collect_pads_clear from this function.
+ *
+ * Since: FIXME

1.3 hopefully :)

Review of attachment 255352 [details] [review]:

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 :)

Created attachment 258603 [details] [review]
This fixes the case I mentioned in my last comment.

Comment on attachment 258603 [details] [review]
This fixes the case I mentioned in my last comment.

This doesn't make sense to me (also remove the whitespace changes please) :)

If the pad is flushing, it is correct that it doesn't accept the seek. You should understand why it flushes in the first place

I accidentially pushed these changes already. Not bad, but I was planning more on top :) Coming soon!

http://stream1.gifsoup.com/view8/4596286/what-did-you-do-o.gif

This is now taken over by Mathieu and Thibault, a new base class instead of another collectpads-like helper. Planned to land for 1.5/1.6.

GstAggregator is already in current master and supports flushing seeks, should that be closed?

Marking as OBSOLETE since there's no RESOLVED|SOMEWHATFIXED (in collectpads).