GNOME Bugzilla – Bug 746249
aggregator: Add gap event support
Last modified: 2015-04-03 21:50:45 UTC
Current implementation mandates buffers to be available on every sink pad before aggregation regardless of gap events happening. In which case the behavior should (arguably) be to stop expecting buffers to be available on that pad for the duration of the gap.
Created attachment 299457 [details] [review] Preliminary solution by putting the pad in "idle" mode This approach has been phased out for a solution involving creating gap buffers for the duration of the gap events, avoiding the need of adding API to handle the "special" case.
Created attachment 299458 [details] [review] Preliminary solution by putting the pad in "idle" mode
In GstAudioAggregator, I've added some code to handle buffers with the GAP flag and just discard them. So it would be nice to have a way to avoid creating gap that will just be discarded.
Created attachment 299670 [details] [review] gap event to gap buffer This was discussed during the hackfest and agreed to be the best/quicker solution for the time being. If we get a gap event we just make that into a gap buffer and avoid subclasses to special case this. They should already be dropping gap buffers anyway (as Olivier just mentioned). What do you guys think?
Review of attachment 299670 [details] [review]: A little unit test would be nice :) ::: gst-libs/gst/base/gstaggregator.c @@ +933,3 @@ + gapbuf = gst_buffer_new (); + + GST_BUFFER_PTS (gapbuf) = pts; You should set DTS = PTS I think. @@ +938,3 @@ + GST_BUFFER_FLAG_SET (gapbuf, GST_BUFFER_FLAG_DROPPABLE); + + gapbuf = gst_buffer_new (); Why do you think you should chain it this way? It might work and actually be a nice solution but looks weird at first sight.
Hi Thibault. Thanks for your review. (In reply to Thibault Saunier from comment #5) > Review of attachment 299670 [details] [review] [review]: > > A little unit test would be nice :) Yes. Will add one. > > ::: gst-libs/gst/base/gstaggregator.c > @@ +933,3 @@ > + gapbuf = gst_buffer_new (); > + > + GST_BUFFER_PTS (gapbuf) = pts; > > You should set DTS = PTS I think. Noted. You are right. Will add that to the next iteration. > > @@ +938,3 @@ > + GST_BUFFER_FLAG_SET (gapbuf, GST_BUFFER_FLAG_DROPPABLE); > + > + gapbuf = gst_buffer_new (); > > Why do you think you should chain it this way? It might work and actually be > a nice solution but looks weird at first sight. Not sure what are you referring to here. Maybe you are talking about using _pad_chain() instead of calling the chain function directly? Can you elaborate please?
Review of attachment 299670 [details] [review]: ::: gst-libs/gst/base/gstaggregator.c @@ +938,3 @@ + GST_BUFFER_FLAG_SET (gapbuf, GST_BUFFER_FLAG_DROPPABLE); + + if (gst_pad_chain (pad, gapbuf) == GST_FLOW_ERROR) { I think that way of chaining is correct, you could also just call the chain function yourself but the above actually seems a bit cleaner. However compare to != GST_FLOW_OK instead of == GST_FLOW_ERROR here.
Created attachment 300139 [details] [review] gap event to gap buffer Set DTS and compare against FLOW_OK on _pad_chain() as requested.
Created attachment 300496 [details] [review] gap event to gap buffer rebased on top of current master
Created attachment 300497 [details] [review] test for proper gap handling Requested test. Had to extend the suite a bit but the minor tweaks should make for easier impl of event handling tests.
I don't think DTS should be set to the PTS here, why not just leave it unset?
(In reply to Tim-Philipp Müller from comment #11) > I don't think DTS should be set to the PTS here, why not just leave it unset? I'm OK either way (actually forgot to add a DTS check in the test). In the sense that its a made-up buffer with no data I figured it having matching presentation and decoding TSs wouldn't hurt and as Thibault asked for it, that logic stuck. More generally, I'd only expect DTS != PTS in coded streams using B frames so I'm kind of thorn in between the two options. Can you be elaborate on why you'd want to left DTS unset?
I think instead maybe Thibault should elaborate why it should be set :)
(In reply to Tim-Philipp Müller from comment #11) > I don't think DTS should be set to the PTS here, why not just leave it unset? Always thought for raw stream the convention was to always set PTS=DTS
DTS has no meaning if there is no decoding needed. Setting it or not is not relevant, I would encourage not setting it.
Created attachment 300541 [details] [review] gap event to gap buffer OK, everyone seems to agree with this so here's a modified patch letting DTS unset in the created gap buffer.
Comment on attachment 300541 [details] [review] gap event to gap buffer Makes sense, let's get it in :) But maybe should add something to the documentation about this behaviour so subclasses don't get confused and try to access the memory of a 0 byte raw video frame and then crash.
Created attachment 300548 [details] [review] document gap handling behavior Thanks for the OK Sebastian. What about this for documentation?
Review of attachment 300548 [details] [review]: ::: gst-libs/gst/base/gstaggregator.c @@ +59,3 @@ + * these into gap buffers with matching PTS and duration. It will also + * flag these buffers with GST_BUFFER_FLAG_GAP and GST_BUFFER_FLAG_DROPPABLE + * to ease their identification and subsequent processing. Maybe mention that it will not have an memory?
Review of attachment 300497 [details] [review]: Looks good, make sure make libs/aggregator.forever runs for a reasonnable long time and go ahead :_
Created attachment 300915 [details] [review] test for proper gap handling Previous test was racy and made aggregator.forever fail after a couple of iterations.
Review of attachment 300915 [details] [review]: Looks good, please push
Review of attachment 300541 [details] [review]: commit 931fbf5e12c464aee9114980d3dae3d87eaf514c Author: Reynaldo H. Verdejo Pinochet <reynaldo@osg.samsung.com> Date: Tue Mar 17 22:13:06 2015 -0300 aggregator: implement gap handling https://bugzilla.gnome.org/show_bug.cgi?id=746249
Review of attachment 300548 [details] [review]: commit 7fb02dc918dc22114ce42ab06544185f48a13b6f Author: Reynaldo H. Verdejo Pinochet <reynaldo@osg.samsung.com> Date: Sun Mar 29 17:53:23 2015 -0300 aggregator: document gap handling behavior https://bugzilla.gnome.org/show_bug.cgi?id=746249
Review of attachment 300915 [details] [review]: commit b24971f2ddf661e0754eae1e97ad82586ef1fdf3 Author: Reynaldo H. Verdejo Pinochet <reynaldo@osg.samsung.com> Date: Fri Mar 27 18:32:27 2015 -0300 aggregator: add gap event handling unit test https://bugzilla.gnome.org/show_bug.cgi?id=746249