GNOME Bugzilla – Bug 709224
audio/videodecoder: Not returning GST_FLOW_EOS when after segment
Last modified: 2014-06-03 14:19:27 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.
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.
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.
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
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.
Please note that the first thing that needs to be done here is to fix all demuxers to aggregate FLOW_EOS correctly.
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 on attachment 256223 [details] [review] audiodecoder: return EOS when segment is over (after demuxers are fixed of course)
Comment on attachment 256224 [details] [review] videodecoder: return EOS when segment is over (after demuxers are fixed of course)
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.
> 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.
(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.
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.
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.
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 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
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
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?
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.
(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.
I think I don't really like the way it's callback based.
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.
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.
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
Created attachment 258630 [details] [review] gstutils: add GstFlowReturnCombiner Updated according to comments, added docs.
Created attachment 258631 [details] [review] oggdemux: use new gstutils helper GstFlowCombiner Updated usage on oggdemux
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
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
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.
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.
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?
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.
Looked over the good changes, looks all good. Please push all that if Tim agrees on the new API :)
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.
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.
(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.
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())
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
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.
Or maybe it's not so important any more if we use an aggregate last flow?
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.
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.
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.
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.
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?
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.
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
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);
Tim, your opinion?
*** Bug 726656 has been marked as a duplicate of this bug. ***
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
ping
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?
So, what should we do about this here? after 1.4?
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.
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 ?
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.
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.
Created attachment 277086 [details] [review] pad: store last flow return and provide acessor function API: gst_pad_get_last_flow_return
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.
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
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)
Maybe take the object lock in the accessor functions... but provide a lockless macro as for all the other things?
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.
(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.
That doesn't matter for what I had in mind, I think starting with FLUSHING is fine.
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?
Created attachment 277171 [details] [review] gstutils: add GstFlowCombiner Updated after GstPad patch changes
(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 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
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
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.
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
Review of attachment 277198 [details] [review]: I like it :)
Created attachment 277213 [details] [review] flowcombiner: add GstFlowCombiner new version of flowcombiner added to libgstbase
+1
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
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.
(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.
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