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 746249 - aggregator: Add gap event support
aggregator: Add gap event support
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal normal
: 1.5.1
Assigned To: Reynaldo H. Verdejo Pinochet
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-03-15 15:17 UTC by Reynaldo H. Verdejo Pinochet
Modified: 2015-04-03 21:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Preliminary solution by putting the pad in "idle" mode (3.31 KB, patch)
2015-03-15 15:21 UTC, Reynaldo H. Verdejo Pinochet
needs-work Details | Review
Preliminary solution by putting the pad in "idle" mode (3.32 KB, patch)
2015-03-15 15:45 UTC, Reynaldo H. Verdejo Pinochet
none Details | Review
gap event to gap buffer (1.40 KB, patch)
2015-03-18 01:40 UTC, Reynaldo H. Verdejo Pinochet
none Details | Review
gap event to gap buffer (1.43 KB, patch)
2015-03-23 14:24 UTC, Reynaldo H. Verdejo Pinochet
none Details | Review
gap event to gap buffer (1.44 KB, patch)
2015-03-27 22:18 UTC, Reynaldo H. Verdejo Pinochet
none Details | Review
test for proper gap handling (4.14 KB, patch)
2015-03-27 22:20 UTC, Reynaldo H. Verdejo Pinochet
none Details | Review
gap event to gap buffer (1.40 KB, patch)
2015-03-29 16:59 UTC, Reynaldo H. Verdejo Pinochet
committed Details | Review
document gap handling behavior (1.01 KB, patch)
2015-03-29 20:57 UTC, Reynaldo H. Verdejo Pinochet
committed Details | Review
test for proper gap handling (4.76 KB, patch)
2015-04-03 21:14 UTC, Reynaldo H. Verdejo Pinochet
committed Details | Review

Description Reynaldo H. Verdejo Pinochet 2015-03-15 15:17:56 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.
Comment 1 Reynaldo H. Verdejo Pinochet 2015-03-15 15:21:27 UTC
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.
Comment 2 Reynaldo H. Verdejo Pinochet 2015-03-15 15:45:09 UTC
Created attachment 299458 [details] [review]
Preliminary solution by putting the pad in "idle" mode
Comment 3 Olivier Crête 2015-03-16 15:35:13 UTC
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.
Comment 4 Reynaldo H. Verdejo Pinochet 2015-03-18 01:40:32 UTC
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?
Comment 5 Thibault Saunier 2015-03-18 09:47:54 UTC
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.
Comment 6 Reynaldo H. Verdejo Pinochet 2015-03-22 21:13:25 UTC
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?
Comment 7 Sebastian Dröge (slomo) 2015-03-23 08:41:52 UTC
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.
Comment 8 Reynaldo H. Verdejo Pinochet 2015-03-23 14:24:09 UTC
Created attachment 300139 [details] [review]
gap event to gap buffer

Set DTS and compare against FLOW_OK on _pad_chain() as requested.
Comment 9 Reynaldo H. Verdejo Pinochet 2015-03-27 22:18:08 UTC
Created attachment 300496 [details] [review]
gap event to gap buffer

rebased on top of current master
Comment 10 Reynaldo H. Verdejo Pinochet 2015-03-27 22:20:45 UTC
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.
Comment 11 Tim-Philipp Müller 2015-03-28 00:21:30 UTC
I don't think DTS should be set to the PTS here, why not just leave it unset?
Comment 12 Reynaldo H. Verdejo Pinochet 2015-03-28 03:52:53 UTC
(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?
Comment 13 Tim-Philipp Müller 2015-03-28 10:10:11 UTC
I think instead maybe Thibault should elaborate why it should be set :)
Comment 14 Thibault Saunier 2015-03-28 10:48:49 UTC
(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
Comment 15 Nicolas Dufresne (ndufresne) 2015-03-28 13:30:12 UTC
DTS has no meaning if there is no decoding needed. Setting it or not is not relevant, I would encourage not setting it.
Comment 16 Reynaldo H. Verdejo Pinochet 2015-03-29 16:59:47 UTC
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 17 Sebastian Dröge (slomo) 2015-03-29 20:10:53 UTC
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.
Comment 18 Reynaldo H. Verdejo Pinochet 2015-03-29 20:57:37 UTC
Created attachment 300548 [details] [review]
document gap handling behavior

Thanks for the OK Sebastian. What about this for documentation?
Comment 19 Thibault Saunier 2015-03-30 15:50:39 UTC
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?
Comment 20 Thibault Saunier 2015-03-30 15:50:41 UTC
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?
Comment 21 Thibault Saunier 2015-03-30 15:53:09 UTC
Review of attachment 300497 [details] [review]:

Looks good, make sure make libs/aggregator.forever runs for a reasonnable long time and go ahead :_
Comment 22 Reynaldo H. Verdejo Pinochet 2015-04-03 21:14:01 UTC
Created attachment 300915 [details] [review]
test for proper gap handling

Previous test was racy and made aggregator.forever fail
after a couple of iterations.
Comment 23 Thiago Sousa Santos 2015-04-03 21:20:30 UTC
Review of attachment 300915 [details] [review]:

Looks good, please push
Comment 24 Reynaldo H. Verdejo Pinochet 2015-04-03 21:28:43 UTC
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
Comment 25 Reynaldo H. Verdejo Pinochet 2015-04-03 21:29:19 UTC
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
Comment 26 Reynaldo H. Verdejo Pinochet 2015-04-03 21:29:39 UTC
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