GNOME Bugzilla – Bug 791986
aggregator: add a drain vmethod
Last modified: 2018-11-03 12:44:12 UTC
Created attachment 366019 [details] [review] add a drain vmethod See descriptions in the change.
Review of attachment 366019 [details] [review]: ::: libs/gst/base/gstaggregator.c @@ +1294,3 @@ + tmppad = (GstAggregatorPad *) tmp->data; + + if (g_atomic_int_get (&tmppad->priv->pending_segment_done) == TRUE) { No need for atomic, your holding the object lock of the pad
Review of attachment 366019 [details] [review]: ::: libs/gst/base/gstaggregator.c @@ +1412,3 @@ aggpad->priv->tail_position = GST_CLOCK_TIME_NONE; update_time_level (aggpad, FALSE); + aggpad->priv->pending_segment_done = priv->segmented_seeking; What's "segmented_seeking"?
(In reply to Olivier Crête from comment #1) > Review of attachment 366019 [details] [review] [review]: > > ::: libs/gst/base/gstaggregator.c > @@ +1294,3 @@ > + tmppad = (GstAggregatorPad *) tmp->data; > + > + if (g_atomic_int_get (&tmppad->priv->pending_segment_done) == TRUE) { > > No need for atomic, your holding the object lock of the pad I have the OBJECT_LOCK, but not the PAD_LOCK. (In reply to Mathieu Duponchelle from comment #2) > Review of attachment 366019 [details] [review] [review]: > > ::: libs/gst/base/gstaggregator.c > @@ +1412,3 @@ > aggpad->priv->tail_position = GST_CLOCK_TIME_NONE; > update_time_level (aggpad, FALSE); > + aggpad->priv->pending_segment_done = priv->segmented_seeking; > > What's "segmented_seeking"? Something that I missed from splitting out from the changes. Thanks for careful watching. I'll update the patch.
Created attachment 366026 [details] [review] add a drain vmethod
Created attachment 366027 [details] [review] add a drain vmethod
Review of attachment 366027 [details] [review]: ::: libs/gst/base/gstaggregator.c @@ +1413,3 @@ aggpad->priv->tail_position = GST_CLOCK_TIME_NONE; update_time_level (aggpad, FALSE); + aggpad->priv->pending_segment_done = priv->segmented_seeking; What will happen if eg we receive a non-flushing, non-segment seek, before upstream branches have pushed their segment dones ?
A flushing seek will cause priv->segmented_seeking=False. This is a good idea for a test case.
Created attachment 366042 [details] [review] remaining changes for non-flushing seeks
(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #7) > A flushing seek will cause priv->segmented_seeking=False. This is a good > idea for a test case. It shouldn't really matter for a flushing seek anyway, my question was about non-flushing, non-segment seeks. I guess what I'm trying to point out is that having that single boolean field added "segmented_seeking", doesn't really work with non-flushing seeks, that can stack up, and some sort of queuing is needed.
I don't understand: 1) a non-flushing, segmented seek comes 2) we set pending_segment_done=TRUE 3) a non-flushing, non-segment seek comes before we got the segment_done 4) we set pending_segment_done=FALSE 5) the source will close the current segment and start a new segment
I don't mind to add queuing, but we need to do this step by step. If we add queuing, this should be done with a test to prove it is needed.
(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #10) > I don't understand: > 1) a non-flushing, segmented seek comes > 2) we set pending_segment_done=TRUE > 3) a non-flushing, non-segment seek comes before we got the segment_done > 4) we set pending_segment_done=FALSE At that point we will also set "segmented_seeking" to FALSE, which afaiu means that segment-done handling will fail.
This brings us back to that we need to define the semantics here. What does it mean to send a non-flushing seek in the mid of another non-flushing seek. For seamless looping one sends non-flushing seeks after one get segment-done and if one wants to cancel a work-in-progress segment, one sends a flushing seek.
I don't think this is defined at the moment? I don't believe we handle this ("queuing"/"stacking" of non-flushing seeks) anywhere else at the moment. A demuxer or parser will not queue/stack anything but just stop streaming, reposition immediately and start streaming with the new segment. It won't finish up any pending segment. In any case, IMHO we can figure this out later. There is such a thing as the perfect being the enemy of the good :)
Review of attachment 366027 [details] [review]: Apart from the concern I had with the queuing of seeks, this patch looks good to go to me, couldn't see any problems with locking in particular. I assumed basesrc already handled the chain of events I was worried about, if it does not then it does not make much sense to start handling that in aggregator :) ::: libs/gst/base/gstaggregator.h @@ -219,2 +219,5 @@ * buffers. The passed in query contains the result of the * downstream allocation query. + * @drain: Optional. + * Called before a new segment will be sent. If not flushing, + * clip work-in-progress buffer and send it. This gets called when we have received all segment dones, not when we send a new segment, and whether the next seek will be flushing or not should not matter here? From looking at the audiomixer implementation it doens't concern itself with flushing concerns and always clips, which makes sense to me.
Review of attachment 366042 [details] [review]: Looks good to me, I assume the test suites still work? Needs an actual commit message too :) ::: libs/gst/base/gstaggregator.c @@ +1934,3 @@ /* don't pass it along as some (file)sink might claim it does * whereas with a collectpads in between that will not likely work */ + // FIXME: wat? ^^^^^^^^^^^ This comes from cc216208233f44b2df689665c58a2365ccc3e286, the explanation in the commit makes a bit more sense: collectpads2: provide query default and callback handling ... which presently mainly serves to answer SEEKING query negatively to dissuade upstream encoders from doing any seeking and "header finalization" (since the returned result of pushing a sticky event is fairly useless nowadays). However that seems pretty fishy to me, we should probably instead either let muxer subclasses do that themselves, or provide some API @@ +2073,3 @@ priv->segmented_seeking = segmented; + /* we apply the segment, once we have segment from all sources */ + /* FIXME: do we need a queue of seek-events? */ I suppose this FIXME can be removed as long as we don't need to support non-flushing segment seeks followed by non flushing segmented seeks
Please don't bother reviewing 'remaining changes for non-flushing seeks' ? this is just for context. I am going to upload that when it is ready for review.
(In reply to Mathieu Duponchelle from comment #15) > Review of attachment 366027 [details] [review] [review]: > > Apart from the concern I had with the queuing of seeks, this patch looks > good to go to me, couldn't see any problems with locking in particular. > > I assumed basesrc already handled the chain of events I was worried about, > if it does not then it does not make much sense to start handling that in > aggregator :) > > ::: libs/gst/base/gstaggregator.h > @@ -219,2 +219,5 @@ > * buffers. The passed in query contains the result of > the > * downstream allocation query. > + * @drain: Optional. > + * Called before a new segment will be sent. If not > flushing, > + * clip work-in-progress buffer and send it. > > This gets called when we have received all segment dones, not when we send a > new segment, and whether the next seek will be flushing or not should not > matter here? From looking at the audiomixer implementation it doens't > concern itself with flushing concerns and always clips, which makes sense to > me. Do you want me to change the wording here?
> Do you want me to change the wording here? Well the current documentation is incorrect right? If so then yeah that could be helpful :)
Created attachment 367836 [details] [review] add a drain vmethod Updated the docs.
Review of attachment 367836 [details] [review]: ::: libs/gst/base/gstaggregator.c @@ +1337,3 @@ + tmppad = (GstAggregatorPad *) tmp->data; + + if (g_atomic_int_get (&tmppad->priv->pending_segment_done) == TRUE) { This still doesn't need an atomic :)
(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #17) > Please don't bother reviewing 'remaining changes for non-flushing seeks' ? > this is just for context. I am going to upload that when it is ready for > review. What's the status on that patch?
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/267.