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 709224 - audio/videodecoder: Not returning GST_FLOW_EOS when after segment
audio/videodecoder: Not returning GST_FLOW_EOS when after segment
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other All
: Normal blocker
: 1.3.3
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 726656 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-10-01 18:55 UTC by Mathieu Duponchelle
Modified: 2014-06-03 14:19 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
audiodecoder: return EOS when segment is over (1.72 KB, patch)
2013-10-01 22:39 UTC, Thiago Sousa Santos
committed Details | Review
videodecoder: return EOS when segment is over (1.21 KB, patch)
2013-10-01 22:40 UTC, Thiago Sousa Santos
committed Details | Review
oggdemux: do not push more buffers after getting EOS (1.42 KB, patch)
2013-10-02 01:04 UTC, Thiago Sousa Santos
needs-work Details | Review
gstutils: add gst_element_combine_flow_returns (8.21 KB, patch)
2013-10-02 18:01 UTC, Thiago Sousa Santos
needs-work Details | Review
oggdemux: use new gstutils function for flowreturn combination (4.62 KB, patch)
2013-10-02 18:01 UTC, Thiago Sousa Santos
accepted-commit_now Details | Review
gstutils: add gst_element_combine_flow_returns (8.95 KB, patch)
2013-10-02 19:25 UTC, Thiago Sousa Santos
none Details | Review
oggdemux: use new gstutils function for flowreturn combination (6.51 KB, patch)
2013-10-02 20:04 UTC, Thiago Sousa Santos
none Details | Review
gstutils: add GstFlowReturnCombiner (7.35 KB, patch)
2013-10-30 19:47 UTC, Thiago Sousa Santos
needs-work Details | Review
oggdemux: use new gstutils helper GstFlowCombiner (7.33 KB, patch)
2013-10-30 19:49 UTC, Thiago Sousa Santos
none Details | Review
gstutils: add GstFlowReturnCombiner (11.70 KB, patch)
2013-10-31 05:06 UTC, Thiago Sousa Santos
needs-work Details | Review
oggdemux: use new gstutils helper GstFlowCombiner (7.20 KB, patch)
2013-10-31 05:06 UTC, Thiago Sousa Santos
accepted-commit_now Details | Review
gstutils: add GstFlowReturnCombiner (11.84 KB, patch)
2013-10-31 17:40 UTC, Thiago Sousa Santos
none Details | Review
gstutils: add GstFlowReturnCombiner (12.34 KB, patch)
2013-10-31 19:26 UTC, Thiago Sousa Santos
reviewed Details | Review
New version: (12.88 KB, patch)
2013-11-05 19:56 UTC, Thiago Sousa Santos
reviewed Details | Review
pad: store last flow return and provide acessor function (6.46 KB, patch)
2014-05-23 21:49 UTC, Thiago Sousa Santos
reviewed Details | Review
gstutils: add GstFlowCombiner (12.53 KB, patch)
2014-05-23 21:49 UTC, Thiago Sousa Santos
none Details | Review
qtdemux: use GstFlowCombiner (5.80 KB, patch)
2014-05-23 21:50 UTC, Thiago Sousa Santos
committed Details | Review
pad: store last flow return and provide acessor function (7.69 KB, patch)
2014-05-26 02:15 UTC, Thiago Sousa Santos
needs-work Details | Review
gstutils: add GstFlowCombiner (12.53 KB, patch)
2014-05-26 02:15 UTC, Thiago Sousa Santos
needs-work Details | Review
pad: store last flow return and provide acessor function (18.18 KB, patch)
2014-05-26 14:04 UTC, Thiago Sousa Santos
committed Details | Review
flowcombiner: add GstFlowCombiner (16.95 KB, patch)
2014-05-26 15:37 UTC, Thiago Sousa Santos
committed Details | Review

Description Mathieu Duponchelle 2013-10-01 18:55:03 UTC
That behaviour was introduced recently by 1618084bc8730343b7e572f8076d39dafafb7f02 but I can't find the rationale behind that.

This implies nasty freezes in pitivi's playback, as we wait for the segment to be done in gnonlin to go to the next one.

If it's indeed necessary, we need to find a way to somehow work around that behaviour, any opinion appreciated.
Comment 1 Thiago Sousa Santos 2013-10-01 19:21:09 UTC
The rationale is that if you want a segment that stops at time X, the demuxer must push enough data so that the frame at X can be decoded. This means pushing data until the next keyframe so decoders have enough info to decode in all cases.

Decoders should be able to return GST_FLOW_EOS when they reached the end of the segment, and then the demuxer can stop earlier than the next keyframe. We had a brief chat about this a month ago on IRC and it was decided to start doing these changes to decoders as soon as a new release cycle started. (It has just started!)

I'd vote for start fixing decoders and demuxers to properly return and aggregate GST_FLOW_EOS now, so we have enough time to find and fix regressions.
Comment 2 Mathieu Duponchelle 2013-10-01 19:52:55 UTC
I vote for that too thanks for the answer.

Would we need API in video / audio decoder for that ? I can spend some time on that issue if needed.
Comment 3 Thiago Sousa Santos 2013-10-01 22:39:16 UTC
Created attachment 256223 [details] [review]
audiodecoder: return EOS when segment is over

if a buffer is clipped by being completely out of segment, check if this
buffer is after the end of the segment and return EOS upstream
Comment 4 Thiago Sousa Santos 2013-10-01 22:40:58 UTC
Created attachment 256224 [details] [review]
videodecoder: return EOS when segment is over

if a buffer is clipped by being completely out of segment, check if this
buffer is after the end of the segment and return EOS upstream

--
Those 2 patches make the base decoders return EOS when segment is over, the
next step is to go though the demuxers and make sure they combine this return
properly. There should be some documentation about how to combine all flow returns
somewhere (can't seem to find it now).

It looks like a good idea to have a scenario to test flow return combination with
defined stop positions in gst-validate.
Comment 5 Tim-Philipp Müller 2013-10-01 22:44:24 UTC
Please note that the first thing that needs to be done here is to fix all demuxers to aggregate FLOW_EOS correctly.
Comment 6 Thiago Sousa Santos 2013-10-02 01:04:50 UTC
Created attachment 256227 [details] [review]
oggdemux: do not push more buffers after getting EOS

Makes oggdemux mark pads that received EOS and avoid pushing data
to them

This makes oggdemux handle EOS correctly. Could someone double check
oggdemux, please? Having one element doing it all correctly we can go
over and copy the same behavior to other demuxers.
Comment 7 Sebastian Dröge (slomo) 2013-10-02 08:40:29 UTC
Comment on attachment 256223 [details] [review]
audiodecoder: return EOS when segment is over

(after demuxers are fixed of course)
Comment 8 Sebastian Dröge (slomo) 2013-10-02 08:40:59 UTC
Comment on attachment 256224 [details] [review]
videodecoder: return EOS when segment is over

(after demuxers are fixed of course)
Comment 9 Sebastian Dröge (slomo) 2013-10-02 08:50:57 UTC
Comment on attachment 256227 [details] [review]
oggdemux: do not push more buffers after getting EOS

I'm not sure this is correct. This will return EOS immediately if one stream is EOS. That's wrong, it should if all are EOS, see gst_ogg_demux_check_eos().

The correct behaviour for flow combination in demuxers is:
- One or more "fatal" errors => fatal error
- All not-linked => not-linked
- All eos => EOS
- Otherwise: OK

Fatal error would be not-negotiated and below. I'm not sure about flushing but AFAIU flushing should only be returned if all pads are flushing.


Please add a utility function to gstutils.[ch] for this. Something like

GstFlowReturn gst_element_combine_flow_returns (GstElement *e, SomeCallback cb, gpointer user_data);

Where SomeCallback is
GstFlowReturn SomeCallback (gpointer user_data) and returns the flow return of the next pad.

So it would iterate over all pads, collect the flow returns and combine them. When done it would return GST_FLOW_CUSTOM_SUCCESS or something like that. Alternative do it index based and put a number of pads as parameter in the function.
Comment 10 Tim-Philipp Müller 2013-10-02 09:56:00 UTC
> I'm not sure about flushing but AFAIU flushing
> should only be returned if all pads are flushing.

I think currently we return FLUSHING as soon as any pad returns FLUSHING. This is an optimisation for seeking/shutdown really. I don't know if there are legitimate use cases where it makes sense to keep one branch flushing, but not the others. I would not change that for now unless there's a compelling use case.
Comment 11 Sebastian Dröge (slomo) 2013-10-02 10:08:32 UTC
(In reply to comment #10)
> > I'm not sure about flushing but AFAIU flushing
> > should only be returned if all pads are flushing.
> 
> I think currently we return FLUSHING as soon as any pad returns FLUSHING. This
> is an optimisation for seeking/shutdown really. I don't know if there are
> legitimate use cases where it makes sense to keep one branch flushing, but not
> the others. I would not change that for now unless there's a compelling use
> case.

Well, it's semantically wrong as in theory the other downstreams did not tell us to go flushing asap and might still be running. I think it's a wrong optimization.
Comment 12 Thiago Sousa Santos 2013-10-02 18:01:28 UTC
Created attachment 256309 [details] [review]
gstutils: add gst_element_combine_flow_returns

Adds a utility function to have a common place where the GstFlowReturn
aggregation can be done. This way all demuxers will have a standard function
to use and have the same expected results.

The function works by calling a GstElementGetNextFlowReturnFunction
callback to get the flow returns from
the GstElement and uses those returns to do the aggregation.

Includes tests.
Comment 13 Thiago Sousa Santos 2013-10-02 18:01:33 UTC
Created attachment 256310 [details] [review]
oggdemux: use new gstutils function for flowreturn combination

Fixes the handling of GST_FLOW_EOS by using the  new function
from gstutils that does the correct combination of flow returns.
Comment 14 Sebastian Dröge (slomo) 2013-10-02 18:20:05 UTC
Review of attachment 256309 [details] [review]:

Looks generally good to me, but I'd like to have a second opinion on the names of the function and callback type :)

::: gst/gstutils.c
@@ +3837,3 @@
+    has_a_pad = TRUE;
+
+    if (ret <= GST_FLOW_NOT_NEGOTIATED) {

As mentioned above I think we should handle FLUSHING the same way as EOS/NOT_LINKED. And between EOS and NOT_LINKED

@@ +3851,3 @@
+
+  if (G_UNLIKELY (!has_a_pad)) {
+    cret = GST_FLOW_OK;

You initialize that above already ;) I think this case must not happen and you can add an assertion for that

::: gst/gstutils.h
@@ +916,3 @@
+ * Returns: %TRUE when a #GstFlowReturn was set, %FALSE if there are no more
+ *
+ * Since: 1.3

Since: 1.4
Comment 15 Sebastian Dröge (slomo) 2013-10-02 18:20:56 UTC
Comment on attachment 256310 [details] [review]
oggdemux: use new gstutils function for flowreturn combination

Is the check_all_eos() function in oggdemux still needed now? If not: remove
Comment 16 Thiago Sousa Santos 2013-10-02 19:25:21 UTC
Created attachment 256316 [details] [review]
gstutils: add gst_element_combine_flow_returns

Updated version:

Since is now 1.4
Added FLUSHING handling and tests
Now also ignores NOT_LINKED properly whenever the combination is 
NOT_LINKED + * = * to handle cases like: EOS + EOS + NOT_LINKED = EOS
Comment 17 Thiago Sousa Santos 2013-10-02 20:04:46 UTC
Created attachment 256321 [details] [review]
oggdemux: use new gstutils function for flowreturn combination

Updated to remove check_eos calls, seems not to be needed anymore.
Tested with playback and seeking. Any chained ogg links I could test, too?
Comment 18 Tim-Philipp Müller 2013-10-02 21:10:32 UTC
This looks a bit weird, esp. how it's used in oggdemux, but can't quite put my finger on what exactly right now. Will have another look tomorrow.
Comment 19 Thiago Sousa Santos 2013-10-25 22:11:43 UTC
(In reply to comment #18)
> This looks a bit weird, esp. how it's used in oggdemux, but can't quite put my
> finger on what exactly right now. Will have another look tomorrow.

Using a struct with an integer inside might make it more clear. Also an int-based function can be provided if that is easier, but I guess it doesn't add much.

The other option I see is to create some kind of GstPadFlowCombiner with an API like:

* gst_pad_flow_combiner_set_pad_flow_return (combiner, pad, flow_ret)
* gst_pad_flow_combiner_get_combined_flow_return (combiner)

The advantage of this is that you only update the flow of the pad you pushed and you can get a small performance improvement, but then most scenarios only have a few pads and it shouldn't matter much. It adds more to the core side, and makes it easier to use on demuxers.
Comment 20 Tim-Philipp Müller 2013-10-26 22:15:55 UTC
I think I don't really like the way it's callback based.
Comment 21 Thiago Sousa Santos 2013-10-30 19:47:57 UTC
Created attachment 258608 [details] [review]
gstutils: add GstFlowReturnCombiner

New version that uses an opaque structure to combine flow returns, easier
and simpler to use from demuxers.
Comment 22 Thiago Sousa Santos 2013-10-30 19:49:10 UTC
Created attachment 258609 [details] [review]
oggdemux: use new gstutils helper GstFlowCombiner

Showing usage of the new GstFlowCombiner.

GstFlowCombiner still misses docs, will add once we settle on a solution.
Comment 23 Sebastian Dröge (slomo) 2013-10-30 19:57:42 UTC
Review of attachment 258608 [details] [review]:

::: gst/gstutils.c
@@ +3889,3 @@
+gst_flow_return_combiner_new (void)
+{
+  GstFlowReturnCombiner *combiner = g_new0 (GstFlowReturnCombiner, 1);

g_slice_new0()

@@ +3952,3 @@
+
+GstFlowReturn
+gst_flow_return_combiner_get_flow_return (GstFlowReturnCombiner * combiner)

Instead I would propose
GstFlowReturn gst_flow_return_combiner_get_flow_return (GstFlowReturnCombiner *c, GstPad *pad, GstFlowReturn ret)

This should return ret immediately for errors instead of combining anything

@@ +3993,3 @@
+
+void
+gst_flow_return_combiner_set_flow_return (GstFlowReturnCombiner * combiner,

Docs missing everywhere ;) And make sure to say that this is not threadsafe and the caller has to ensure that
Comment 24 Thiago Sousa Santos 2013-10-31 05:06:14 UTC
Created attachment 258630 [details] [review]
gstutils: add GstFlowReturnCombiner

Updated according to comments, added docs.
Comment 25 Thiago Sousa Santos 2013-10-31 05:06:32 UTC
Created attachment 258631 [details] [review]
oggdemux: use new gstutils helper GstFlowCombiner

Updated usage on oggdemux
Comment 26 Sebastian Dröge (slomo) 2013-10-31 13:59:09 UTC
Review of attachment 258630 [details] [review]:

Looks good except for some nitpicks :)

::: gst/gstutils.c
@@ +3801,3 @@
+ * GstFlowCombiner:
+ *
+ * Utilitary struct to help handling #GstFlowReturn combination. Useful for

Is utilitary the right word here? Utility struct maybe?

@@ +3853,3 @@
+  GstFlowCombiner *combiner = g_slice_new (GstFlowCombiner);
+
+  combiner->table = g_hash_table_new (g_direct_hash, NULL);

g_direct_equal() ?

@@ +3993,3 @@
+gst_flow_combiner_update_flow_return (GstFlowCombiner * combiner, GstPad * pad,
+    GstFlowReturn fret)
+{

Add some g_return_val_if_fail() here and in other public functions
Comment 27 Reynaldo H. Verdejo Pinochet 2013-10-31 16:47:56 UTC
Review of attachment 258630 [details] [review]:

Some gram fixes

::: gst/gstutils.c
@@ +3806,3 @@
+ *
+ * #GstFlowCombiner works by storing the #GstFlowReturn for a given #GstPad
+ * and when a #GstFlowReturn is stored it computes the combined return value

once a #GstFlowReturn maybe?

is stored,

@@ +3813,3 @@
+ * Whenever the #GstFlowReturn for a pad needs to be updated, call
+ * gst_flow_combiner_update_flow_return(), this function already returns
+ * the new combined #GstFlowReturn for the current #GstPad<!-- -->s in it.

already returns/will return? Not entirely sure about this one though. Maybe
I'm not getting this sentence's meaning right

@@ +3818,3 @@
+ *
+ * Please be aware that this struct isn't thread safe as its major use case
+ * is for demuxers, that usually will have a single thread operating it.

as it's designed to be used by demuxers. These will likely have just one thread operating on them

You might want to go with another rephrasing alternative though. The idea is
that demuxers are not an 'use cases' per se but 'users'.

@@ +3826,3 @@
+ * Aside from reducing the user's code size, the main advantage of using this
+ * helper struct is to follow the standard rules for #GstFlowReturn combination,
+ * those rules are:

combination. These rules are:

@@ +3862,3 @@
+ * @combiner: the #GstFlowCombiner to clear
+ *
+ * Resets a #GstFlowCombiner to its initial states, without any #GstPad<!-- -->s

initial state

@@ +4023,3 @@
+ * @pad: (transfer-none): the #GstPad to remove
+ *
+ * Removes a #GstPad to the #GstFlowCombiner. Its flow return is also

Removes a #GstPad from the

@@ +4024,3 @@
+ *
+ * Removes a #GstPad to the #GstFlowCombiner. Its flow return is also
+ * removed and not used for future combinations.

Are there any chances of using it if it has been removed? Seems redundant
Comment 28 Thiago Sousa Santos 2013-10-31 17:40:03 UTC
Created attachment 258679 [details] [review]
gstutils: add GstFlowReturnCombiner

Fixes applied. Except the g_direct_equal() one because the documentation
says that NULL defaults to it on a faster version that saves a function
call.
Comment 29 Thiago Sousa Santos 2013-10-31 19:26:11 UTC
Created attachment 258683 [details] [review]
gstutils: add GstFlowReturnCombiner

Yet another update, adding gst_flow_combiner_reset_pad to get a pad's state
back to GST_FLOW_OK. Useful when demuxer's pads are flushing or reconfiguring.
Comment 30 Sebastian Dröge (slomo) 2013-11-01 13:57:23 UTC
Review of attachment 258683 [details] [review]:

Looks good to me now

::: gst/gstutils.c
@@ +3845,3 @@
+ * * %GST_FLOW_NOT_LINKED: only if all returns are NOT_LINKED too
+ * * %GST_FLOW_ERROR or below: if at least one returns an error return
+ * * %GST_FLOW_OK: otherwise

Mention that errors are returned immediately?
Comment 31 Thiago Sousa Santos 2013-11-01 15:17:48 UTC
http://cgit.freedesktop.org/~thiagoss/gst-plugins-good/
http://cgit.freedesktop.org/~thiagoss/gst-plugins-base/
http://cgit.freedesktop.org/~thiagoss/gst-plugins-bad/

Examples of plugins ported to using this on master branch of those repos.
Comment 32 Sebastian Dröge (slomo) 2013-11-01 15:27:59 UTC
Looked over the good changes, looks all good. Please push all that if Tim agrees on the new API :)
Comment 33 Thiago Sousa Santos 2013-11-01 22:37:23 UTC
I ported tee over and decided to check if performance would be the same. For 2 pads it was the same, but then I tried with 5 pads and the difference was of about 60s to process 10M buffers.

The pipeline was "fakesrc ! tee ! [queue ! fakesink] * 5"

I used 'time' for measuring it.

I know 10M is too much, even for a 60fps video it would take a 2 day long clip for going over 10M frames (if my math is correct). In any way, I would prefer to look for an alternative for a compatible performance.

The issue is that for every GST_FLOW_OK it will iterate over the pads and check the combination, while most demuxers and tee won't do that as they assume all other pads are _OK while they are demuxing/looping. They only go over all pads in NOT_LINKED/EOS cases. I think it makes sense for GstFlowCombiner to do the same.

Once a _ERROR is received it returns immediately (might store or not this error for the pad) and then if the user decides to proceed and gets an _OK it would return _OK. The user decided to ignore the _ERROR, so I think it is ok not to store this GstFlowReturn and move on.

If we decide to store the _ERROR, then the user must explicitly set a new flow-return if it decides to move on, this is also ok for me.

Any opinions? Am I being too picky with performance?


Another idea is to have counters for the different flow-returns and increment/decrement as the returns from the pads change. This way we have constant combination time, regardless of the number of pads. It adds a little overhead when setting the flow returns, but this is also constant.
Comment 34 Tim-Philipp Müller 2013-11-01 23:05:36 UTC
Generally looks fine to me (thanks for reworking this!), but I still have a nagging feeling that it could be even better.

Some random comments:

 - in the commit message: GstFlowReturnCombiner -> GstFlowCombiner

 - FLOW_FLUSH aggregation should be documented in docs chunk too

 - gst_flow_combiner_update_flow_return() - quite long, how about
   just _add_flow() or _combine_flow()?

 - gst_flow_combiner_get_flow_return(): do we really need to iterate over
   all pads every time? Can't we store the last aggregated flow return and
   combine that with the new one in most cases?

 - all this hashtable stuff irks me a little, even if it's just an internal
   implementation detail.

 - wonder if it shouldn't go into gstpad.c as GstPadFlowCombiner, then we
   could just use the GstPad private struct and store the last flow there ;)
   (or we could do that anyway via some internal helper funcs if we wanted to)

 - another idea: I think most elements (esp. demuxers) have a derived pad or
   pad-related stream structure which they operate on, so they could store
   some kind of GstFlowCombinerPadID in there as well. You could hand those
   out when the demuxer/element does _add_pad(), and it could just be a
   counter or whatever (just typedef it to guintptr then it can be anything
   in future), then you don't need the hashtable and can just do pads[i]
   inside the flow combiner, etc.
Comment 35 Thiago Sousa Santos 2013-11-04 22:23:08 UTC
(In reply to comment #34)
> Generally looks fine to me (thanks for reworking this!), but I still have a
> nagging feeling that it could be even better.
> 
> Some random comments:
> 
>  - in the commit message: GstFlowReturnCombiner -> GstFlowCombiner
> 
>  - FLOW_FLUSH aggregation should be documented in docs chunk too
> 
>  - gst_flow_combiner_update_flow_return() - quite long, how about
>    just _add_flow() or _combine_flow()?

Agreed.

> 
>  - gst_flow_combiner_get_flow_return(): do we really need to iterate over
>    all pads every time? Can't we store the last aggregated flow return and
>    combine that with the new one in most cases?

Good idea, this would make the default GST_FLOW_OK case as fast as before with tee and the demuxers.

> 
>  - all this hashtable stuff irks me a little, even if it's just an internal
>    implementation detail.
> 
>  - wonder if it shouldn't go into gstpad.c as GstPadFlowCombiner, then we
>    could just use the GstPad private struct and store the last flow there ;)
>    (or we could do that anyway via some internal helper funcs if we wanted to)

Would the GstFlowCombiner be setting it directly or pad_push would do it automatically? If this is the case we could make this variable public directly or via a function, as most demuxers seem to use it. Is this useful elsewhere?

This would get rid of the hashtable and we could use a linked list instead.

> 
>  - another idea: I think most elements (esp. demuxers) have a derived pad or
>    pad-related stream structure which they operate on, so they could store
>    some kind of GstFlowCombinerPadID in there as well. You could hand those
>    out when the demuxer/element does _add_pad(), and it could just be a
>    counter or whatever (just typedef it to guintptr then it can be anything
>    in future), then you don't need the hashtable and can just do pads[i]
>    inside the flow combiner, etc.

This is a good idea, too. But what happens when a pad is removed? We can just set it to NOT_LINKED and everything else should work, but then the array of pads
will only increase and we can have the flow iteration taking longer and longer, aside from memory consumption for the arrays (unless we iterate the array or have some mechanism to reuse old ids when new pads are added).

The 1st idea is simpler to implement, but the 2nd is less invasive. Going to try the 2nd now.
Comment 36 Thiago Sousa Santos 2013-11-05 19:56:41 UTC
Created attachment 259043 [details] [review]
New version:

* uses GArray instead of GHashTable (faster performance)
* Dropped reset_pad as it is not really useful when you have set_flow public
* _FLUSH is now returned promptly
* Store the last return to avoid iterating the pads everytime
* Shorter function names as suggested (replaced _flow_return() with _flow())
Comment 37 Sebastian Dröge (slomo) 2013-11-06 15:30:44 UTC
Review of attachment 259043 [details] [review]:

::: gst/gstutils.c
@@ +3952,3 @@
+ * gst_flow_combiner_set_flow:
+ * @combiner: the #GstFlowCombiner
+ * @padid: (transfer-none): the #GstFlowCombinerPadId that represents the

(transfer none)

@@ +3964,3 @@
+void
+gst_flow_combiner_set_flow (GstFlowCombiner * combiner,
+    GstFlowCombinerPadId padid, GstFlowReturn fret)

I really don't like the addition of a new type just for identifying pads. That makes the usage quite ugly, just do this internally or keep the hash table or just do a linear search in a list or array.

API users should not need to worry about internal optimizations
Comment 38 Tim-Philipp Müller 2013-11-06 15:46:08 UTC
Sebastian, I would agree in principle, but I think you have to make a decision here whether efficiency is a consideration or not. If yes, then something like the id may be needed to implement things efficiently and avoid hashtable lookups.
Comment 39 Tim-Philipp Müller 2013-11-06 15:57:32 UTC
Or maybe it's not so important any more if we use an aggregate last flow?
Comment 40 Thiago Sousa Santos 2013-11-11 16:12:03 UTC
http://cgit.freedesktop.org/~thiagoss/gstreamer

Added different implementation to branches here, the performance of only storing the not-linked is the worst of them, but not terribly worse. The array one and the offsets are a little better.

The offsets was a new idea I had to keep the last GstFlowReturn at the demuxer stream strucuters/pad and use G_STRUCT_OFFSET / G_STRUCT_MEMBER to access them. Guess this removes the responsibility of storing the data internally and save us some time, leaving the GstFlowCombiner storing only the list of streams to iterate.

For GstTee with 5 pads, all GstFlowCombiner add some extra overhead (gst_tee_handle_data going from 4.96% (358 000 000) to 5.10% (439 000 000) for the offsets option), but for ogg they actually saved some time.
Comment 41 Tim-Philipp Müller 2013-11-11 16:39:20 UTC
The offsets thing is a bit too clever IMHO. It's not really "nice" API, and unusable from bindings if I'm not mistaken. Also assumes that all pads are of the same type, which is almost always true I guess, but not always.
Comment 42 Sebastian Dröge (slomo) 2013-11-11 16:48:28 UTC
The offset thing looks not very reusable and rather hackish imho.

How fast was the not-linked counting compared to that? And I think you should count EOS too instead of using the flag. The flag is e.g. not set if the demuxer decided that the pad is EOS because buffers are outside the segment.
Comment 43 Thiago Sousa Santos 2013-11-14 18:42:00 UTC
Finally organized all the branches and ran some scenarios for all of them under valgrind. Here are the candidates:

1) Current version - the upstream implementation, no flow combiner.
2) Array iteration - Stores the pad last_return into an array, has the drawback of needing to return a GstPadFlowId to identify the pad, users need to store this data.
3) Hashtable - Uses a hashtable internally, straightforward approach, but there is a hashtable
4) EOS/Notlinked lists - Stores a list of pads currently on EOS and another list for NOT_LINKED. Everything else is returned immediately. This has a different meaning for the scenario where you do: _OK, _ERROR, _OK. The returns for those updates would be: _OK, _ERROR, and *_OK*. The last one returns _OK because the error has already been returned and the user and it was ignored as it proceeded to combine flows.
5) Callback - Uses a callback function to ask the user for the current flow-return of a stream. It also doesn't store pads, but gpointer to generic structs that represent the streams for the elements using them.

All of the implementations can be found at: http://cgit.freedesktop.org/~thiagoss/gst-plugins-bad

The scenarios tested (all under callgrind):

* tee: gst-launch-1.0 fakesrc num-buffers=1000000 ! tee name=t ! queue! fakesink t. ! queue ! fakesink t. ! queue ! fakesink t. ! queue ! fakesink t. ! queue ! fakesink
* tee-notlinked: gst-launch-1.0 fakesrc num-buffers=1000000 ! tee name=t ! queue! fakesink t. ! queue ! fakesink t. ! queue ! fakesink t. ! queue ! fakesink t. ! queue
* ogg: gst-launch-1.0 filesrc location=/home/thiagoss/gst/head/test.ogg ! oggdemux name=d ! queue ! fakesink d. ! queue ! fakesink d. ! queue ! fakesink
* ogg-notlinked: gst-launch-1.0 filesrc location=/home/thiagoss/gst/head/test.ogg ! oggdemux name=d ! queue ! fakesink d. ! queue ! fakesink

test.ogg contains 1 video and 2 audios, ~28min.

Results:
                tee          | tee not linked | ogg         | ogg not linked
Current      : 1 358 000 000 | 1 355 000 003  | 237 625 753 | 235 570 313
array        : 1 479 000 000 | 1 571 999 907  | 244 333 265 | 247 011 452
hashtable    : 1 989 000 000 | 2 312 937 087  | 266 927 779 | 283 175 577
eos/notlinked: 2 757 000 000 | 2 566 038 218  | 255 379 011 | 255 598 118
callback     : 1 469 000 000 | 1 571 978 800  | 243 745 984 | 246 878 300

For tee those are the callgrind 'absolute' results for the gst_tee_handle_data function, for ogg are for the gst_ogg_demux_chain_peer function.
Comment 44 Sebastian Dröge (slomo) 2013-11-26 10:48:37 UTC
IMHO that means we should use the original, callback based version. That's least annoying for the user and the fastest in all cases after the current code.

The callback variant is the one we had in the very beginning, right? The "foreach"-style-function approach?
Comment 45 Thiago Sousa Santos 2013-12-02 12:01:28 UTC
http://cgit.freedesktop.org/~thiagoss/gstreamer/log/?h=flowcombiner-callback

The code is here, it is different from the initial approach as it uses a struct to store the structs that are being used for the flow combination.
Comment 46 Sebastian Dröge (slomo) 2013-12-30 08:46:28 UTC
So what are we going to do here? I personally prefer the callback solution but Tim does not like it at all :) However I still prefer the iterator approach over what is there now, but can accept both
Comment 47 Thiago Sousa Santos 2013-12-31 11:06:38 UTC
Given the performance results, I prefer this approach http://cgit.freedesktop.org/~thiagoss/gstreamer/log/?h=flowcombiner-callback as the API is easy enough to use and the performance penalty is very low.

Use:

/* Define a callback to get the last flow return for a pad */
GstFlowReturn my_get_last_flow_return_callback(pad_struct) {
  return pad_struct->last_ret;
}

/* create a flowcombiner with the callback */
flowcombiner = gst_flow_combiner_new (my_get_last_flow_return_callback);

/* add your pad structs to id */
gst_flow_combiner_add_stream (flowcombiner, pad_struct);

/* use it by passing the new GstFlowReturn for a pad struct and getting
 * the new return */
ret = gst_flow_combiner_update_flow (tee->flowcombiner, pad_struct, new_ret);
Comment 48 Sebastian Dröge (slomo) 2014-01-14 19:47:14 UTC
Tim, your opinion?
Comment 49 Mathieu Duponchelle 2014-03-18 19:01:27 UTC
*** Bug 726656 has been marked as a duplicate of this bug. ***
Comment 50 Thiago Sousa Santos 2014-03-21 12:41:10 UTC
For 1.3 we need to either decide on the API here or update the demuxers manually as this is a regression for GES.

The currently winning API:
http://cgit.freedesktop.org/~thiagoss/gstreamer/diff/gst/gstutils.h?h=flowcombiner-callback&id=35bc18d7a000fa879f808a96e86da39189412c3e

Other alternatives at flowcombiner-* branches:
http://cgit.freedesktop.org/~thiagoss/gstreamer/?h=flowcombiner-callback
Comment 51 Thiago Sousa Santos 2014-04-03 21:55:33 UTC
ping
Comment 52 Tim-Philipp Müller 2014-04-04 09:10:24 UTC
Let's start by taking the "offsets" branch and the "arrayiter" branch off the table.

In terms of API, I prefer the notlinkedcount/hashtable branches, which are more or less the same as far as I can tell, internal implementation details aside.

I'm not so fond of the callback branch/API, because (a) it's callback based, and (b) the gpointer stuff. Ultimately this all just seems like a workaround for not storing the last_flow in the GstPad struct directly somewhere (either private or public).

So why are we not doing that again?
Comment 53 Sebastian Dröge (slomo) 2014-05-23 08:36:24 UTC
So, what should we do about this here? after 1.4?
Comment 54 Thiago Sousa Santos 2014-05-23 14:00:14 UTC
I guess it is too late to merge a new API, but gnonlin/ges has a regression after some already pushed patches.

Do you gnonlin/ges people have a solution/idea for 1.4?

Of course we can just go everywhere and fix returns without a standard API as well and merge the API when we have enough time to review/change before a release.
Comment 55 Mathieu Duponchelle 2014-05-23 19:12:02 UTC
I don't have any proposal regarding that except the "go everywhere and fix it" one, which kind of feels like a waste of time, I haven't investigated more than that as I thought it would get taken care of I have to admit, so I can't really estimate the amount of time to get it done in at least the most important demuxers, can you estimate that thiago ?
Comment 56 Tim-Philipp Müller 2014-05-23 20:19:06 UTC
I don't mind seeing this get merged before 1.4 if we can agree on the API. I think my personal preference would be to just adding the last_flow to GstPad and make it GstPad API unless there's a reason not to do that.
Comment 57 Thiago Sousa Santos 2014-05-23 20:20:26 UTC
Started working on this with having the pad store its last flow return and have the GstFlowCombiner use that, will report and show patches shortly.
Comment 58 Thiago Sousa Santos 2014-05-23 21:49:35 UTC
Created attachment 277086 [details] [review]
pad: store last flow return and provide acessor function

API: gst_pad_get_last_flow_return
Comment 59 Thiago Sousa Santos 2014-05-23 21:49:43 UTC
Created attachment 277087 [details] [review]
gstutils: add GstFlowCombiner

Adds a utility struct that is capable of storing and aggregating flow returns
associated with pads.

This way all demuxers will have a standard function to use and have the
same expected results.

Includes tests.
Comment 60 Thiago Sousa Santos 2014-05-23 21:50:10 UTC
Created attachment 277088 [details] [review]
qtdemux: use GstFlowCombiner

Removes the common code to combining flow returns to let it be
handled by core gstutils' GstFlowCombiner
Comment 61 Thiago Sousa Santos 2014-05-23 21:53:39 UTC
The patch for storing the last flow return for the pad. One detail is that I decided not to take the object lock on the getter function as the main use case doesn't require id.

Then I have the new GstFlowCombiner using it and an example element ported to use it. (Already ported all the other main demuxers: avi, ogg, matroska, asf, mpegts)
Comment 62 Sebastian Dröge (slomo) 2014-05-24 07:47:02 UTC
Maybe take the object lock in the accessor functions... but provide a lockless macro as for all the other things?
Comment 63 Sebastian Dröge (slomo) 2014-05-24 07:47:52 UTC
Review of attachment 277086 [details] [review]:

::: gst/gstpad.c
@@ +392,3 @@
   pad->priv->events_cookie = 0;
   pad->priv->last_cookie = -1;
+  pad->priv->last_flowret = GST_FLOW_OK;

I think initially it should be GST_FLOW_FLUSHING until the pad is activated... and GST_FLOW_FLUSHING again when it's deactivated. Otherwise good, just see the locking comment.
Comment 64 Thiago Sousa Santos 2014-05-24 14:28:19 UTC
(In reply to comment #63)
> Review of attachment 277086 [details] [review]:
> 
> ::: gst/gstpad.c
> @@ +392,3 @@
>    pad->priv->events_cookie = 0;
>    pad->priv->last_cookie = -1;
> +  pad->priv->last_flowret = GST_FLOW_OK;
> 
> I think initially it should be GST_FLOW_FLUSHING until the pad is activated...
> and GST_FLOW_FLUSHING again when it's deactivated. Otherwise good, just see the
> locking comment.

Setting it to FLUSHING seems more consistent with the overall GstPads behavior but Tim wanted to use it to do some further error tracking in the pipeline. Resetting it to FLUSHING will force Tim's idea to run before the pipeline was put to NULL after an error, it should be fine but I'd like him to double-check that we meet his requirements as well.

After his review I'll create new patches.
Comment 65 Tim-Philipp Müller 2014-05-24 15:17:55 UTC
That doesn't matter for what I had in mind, I think starting with FLUSHING is fine.
Comment 66 Thiago Sousa Santos 2014-05-26 02:15:06 UTC
Created attachment 277170 [details] [review]
pad: store last flow return and provide acessor function

Updated the patch after the review.

A doubt I had was about the GST_PADDING at the end as sizeof(enum) can be
different then sizeof(gpointer) depending on the compiler. How to have
it consistent? Use unions? Any less ugly option or the compiler will add
some padding itself as it should make it pointer-size aligned?
Comment 67 Thiago Sousa Santos 2014-05-26 02:15:28 UTC
Created attachment 277171 [details] [review]
gstutils: add GstFlowCombiner

Updated after GstPad patch changes
Comment 68 Sebastian Dröge (slomo) 2014-05-26 07:00:01 UTC
(In reply to comment #66)
> Created an attachment (id=277170) [details] [review]
> pad: store last flow return and provide acessor function
> 
> Updated the patch after the review.
> 
> A doubt I had was about the GST_PADDING at the end as sizeof(enum) can be
> different then sizeof(gpointer) depending on the compiler. How to have
> it consistent? Use unions? Any less ugly option or the compiler will add
> some padding itself as it should make it pointer-size aligned?

union {
  gpointer _gst_reserved[GST_PADDING];
  struct {
    GstFlowReturn last_flow;
  } abi;
} ABI;
Comment 69 Sebastian Dröge (slomo) 2014-05-26 07:05:40 UTC
Comment on attachment 277170 [details] [review]
pad: store last flow return and provide acessor function

Looks good, just change the padding as described in previous comment :)

Also please add a test or two for this
Comment 70 Sebastian Dröge (slomo) 2014-05-26 07:08:23 UTC
Review of attachment 277171 [details] [review]:

Looks good to me, just misses some cleanup

::: gst/gstutils.c
@@ +3868,3 @@
+  GstFlowReturn last_ret;
+
+  GstFlowCombinerGetStreamFlow getstreamflow_func;

This is unused, right? Please remove :)

::: gst/gstutils.h
@@ +33,3 @@
 G_BEGIN_DECLS
 
+typedef GstFlowReturn (*GstFlowCombinerGetStreamFlow)(gpointer stream);

And here remove too
Comment 71 Sebastian Dröge (slomo) 2014-05-26 07:09:46 UTC
Review of attachment 277088 [details] [review]:

Looks good. We should also at least port over oggdemux, avidemux, matroskademux, flvdemux, tsdemux and mpegpsdemux for consistency... but those should also all be simple.
Comment 72 Thiago Sousa Santos 2014-05-26 14:04:41 UTC
Created attachment 277198 [details] [review]
pad: store last flow return and provide acessor function

Updated patch:

* Handling the padding ABI correctly with an union
* Adds tests for push/pull mode operation
* Setting the flowret in more places for pull mode
Comment 73 Sebastian Dröge (slomo) 2014-05-26 14:18:17 UTC
Review of attachment 277198 [details] [review]:

I like it :)
Comment 74 Thiago Sousa Santos 2014-05-26 15:37:50 UTC
Created attachment 277213 [details] [review]
flowcombiner: add GstFlowCombiner

new version of flowcombiner added to libgstbase
Comment 75 Tim-Philipp Müller 2014-05-26 16:04:10 UTC
+1
Comment 76 Thiago Sousa Santos 2014-05-27 02:33:08 UTC
Thanks everyone for the reviews and help.
Here is the long patch list.

core:
commit 9e8bd15c124eb62fdaeb7dcbae40cf816f8a63a9
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Mon May 26 12:31:33 2014 -0300

    flowcombiner: add GstFlowCombiner
    
    Adds a utility struct that is capable of storing and aggregating flow return
    associated with pads.
    
    This way all demuxers will have a standard function to use and have the
    same expected results.
    
    Includes tests.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=709224

commit c6f92562b666d09a01545bee4fce7536d44fc5cc
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Fri May 23 13:25:35 2014 -0300

    pad: store last flow return and provide acessor function
    
    Stores the last result of a gst_pad_push or a pull on the GstPad and provide
    a getter and a macro to access this field.
    
    Whenever the pad is inactive it is set to FLUSHING
    
    API: gst_pad_get_last_flow_return
    
    https://bugzilla.gnome.org/show_bug.cgi?id=709224

base:
commit 5d35675a74d57bc45e7bc531441487d0e809bca9
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Mon May 26 20:57:30 2014 -0300

    tests: videodecoder: audiodecoder: add tests for eos after segment
    
    Tests that pushing a buffer after the segment returns EOS

commit 0cb5ea439669b1791b66b7bbd5f80e90a9c6b134
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Mon May 26 21:24:07 2014 -0300

    videodecoder: actually return the push result in backwards playback
    
    It was always returning _OK regardless of what downstream returned

commit ff9e37ea668e06b13ab4b739f0eb87a3607310e3
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Mon May 26 12:44:48 2014 -0300

    videodecoder: return EOS when segment is over
    
    if a buffer is clipped by being completely out of segment, check if this
    buffer is after the end of the segment and return EOS upstream
    
    https://bugzilla.gnome.org/show_bug.cgi?id=709224

commit 09b8f902eab42d10b5392bc6b2f9f78cd7e670f5
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Mon May 26 12:44:38 2014 -0300

    audiodecoder: return EOS when segment is over
    
    if a buffer is clipped by being completely out of segment, check if this
    buffer is after the end of the segment and return EOS upstream
    
    https://bugzilla.gnome.org/show_bug.cgi?id=709224

commit d3ca7271dcd2369434df1652684993af690d9f95
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Mon May 26 11:45:29 2014 -0300

    oggdemux: use new gstutils helper GstFlowCombiner
    
    Fixes the handling of GST_FLOW_EOS by using the helper object
    from gstutils that does the correct combination of flow returns.
    
good:
commit fd6b3488984ecd8b9b1e01e2a92028be0cc6df7c
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Mon May 26 19:47:39 2014 -0300

    avidemux: remove stream last flow return
    
    GstPad already stores that information
    
    https://bugzilla.gnome.org/show_bug.cgi?id=709224

commit 2b454bf87f7bfcdbcd8069bfca76c81004233025
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Mon May 26 19:37:46 2014 -0300

    qtdemux: remove last flow return from stream struct
    
    It is already stored on GstPad on core
    
    https://bugzilla.gnome.org/show_bug.cgi?id=709224

commit 3b887887be14ca7df0a9ffa9efc8f8ac94dc39f9
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Mon May 26 19:19:45 2014 -0300

    flvdemux: Use GstFlowCombiner
    
    Use the flow combiner to have the standard combination results and avoid
    repeating the same code
    
    https://bugzilla.gnome.org/show_bug.cgi?id=709224

commit c7c25071e313497b69eee472812d66df82f0087c
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Mon May 26 13:21:25 2014 -0300

    matroskademux: use GstFlowCombiner
    
    Use the flow combiner to have the standard combination results and avoid
    repeating the same code
    
    https://bugzilla.gnome.org/show_bug.cgi?id=709224

commit da3c0316278933dde369cf2e9f15d8e3a61a32ea
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Mon May 26 13:04:10 2014 -0300

    avidemux: use GstFlowCombiner
    
    Removes flow return combination code to use the newly added GstFlowCombiner

commit 4b0ce7dc305b041b3c11ddce20bbc0872e049a8e
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Fri May 23 17:53:00 2014 -0300

    qtdemux: use GstFlowCombiner
    
    Removes the common code to combining flow returns to let it be
    handled by core gstutils' GstFlowCombiner
    
    https://bugzilla.gnome.org/show_bug.cgi?id=709224

ugly:
commit 8263652d2c88f7fb7ca4dde913e6fe1644a547cc
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Mon May 26 17:55:43 2014 -0300

    rmdemux: use GstFlowCombiner
    
    Removes flow return combination code to use the newly added GstFlowCombiner
    
    https://bugzilla.gnome.org/show_bug.cgi?id=709224

commit df768bc99cbf6fc4b2db880e1c51ac5425780a6d
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Fri May 23 18:17:11 2014 -0300

    asfdemux: use GstFlowCombiner
    
    Removes flow return combination code to use the newly added GstFlowCombiner
    
    https://bugzilla.gnome.org/show_bug.cgi?id=709224

bad:
commit ae839d8dc293800dcfbfad7ee767944bf5488b1e
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Mon May 26 17:34:40 2014 -0300

    mxfdemux: use GstFlowCombiner
    
    Removes flow return combination code to use the newly added GstFlowCombiner
    
    https://bugzilla.gnome.org/show_bug.cgi?id=709224

commit 6dc571b5cfa897e30f36993ddc74f43f1d1e2178
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Mon May 26 17:18:01 2014 -0300

    mpegdemux: use GstFlowCombiner
    
    Removes flow return combination code to use the newly added GstFlowCombiner
    
    https://bugzilla.gnome.org/show_bug.cgi?id=709224

commit b66012a586cd94194a0d16dc56c15b9e7398c244
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Fri May 23 18:26:57 2014 -0300

    tsdemux: use GstFlowCombiner
    
    Removes flow return combination code to use the newly added GstFlowCombiner

gst-libav:
commit 66588ae60df40c60052a9f094444b15b5faf9c07
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Mon May 26 16:04:50 2014 -0300

    avdemux: use GstFlowCombiner
    
    To remove replicated code from all demuxers to a single standard way
    of aggregating flow returns
Comment 77 Thiago Sousa Santos 2014-05-27 02:35:03 UTC
On a side notice, there were 2 demuxers that are now using flowcombiner but still keep the stream structs with a last_flow variable:

oggdemux: it has an optimization to avoid pushing data on linked pads, I decided to keep that for now until we get a replacement for it

mxfdemux: has some get_earliest_pad function that compares last_flow from all streams. Not sure what is the logic behind that so I decided not to touch it.
Comment 78 Sebastian Dröge (slomo) 2014-05-27 07:02:04 UTC
(In reply to comment #77)

> mxfdemux: has some get_earliest_pad function that compares last_flow from all
> streams. Not sure what is the logic behind that so I decided not to touch it.

mxfdemux tries to keep the interleaving between the different streams small, by default smaller than 0.5 seconds. For that it checks the earliest pad against the current one, and if the current one is too far ahead it will jump to the earliest pad to catch up.
Comment 79 Thiago Sousa Santos 2014-06-03 14:19:27 UTC
The real -ugly commits were pushed today:

commit a71a23957203853690b438703bc1d8419aca0046
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Mon May 26 17:55:43 2014 -0300

    rmdemux: use GstFlowCombiner
    
    Removes flow return combination code to use the newly added GstFlowCombiner
    
    https://bugzilla.gnome.org/show_bug.cgi?id=709224

commit df4a98e51031a2649d98836a297dfd3cb85a301b
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Fri May 23 18:17:11 2014 -0300

    asfdemux: use GstFlowCombiner
    
    Removes flow return combination code to use the newly added GstFlowCombiner
    
    https://bugzilla.gnome.org/show_bug.cgi?id=709224