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 763780 - flvdemux: don't emit pad-added until caps are ready
flvdemux: don't emit pad-added until caps are ready
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other All
: Normal major
: 1.8.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-03-16 19:22 UTC by Håvard Graff (hgr)
Modified: 2016-03-29 07:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
tests and fix (15.14 KB, patch)
2016-03-16 19:22 UTC, Håvard Graff (hgr)
committed Details | Review

Description Håvard Graff (hgr) 2016-03-16 19:22:20 UTC
Created attachment 324135 [details] [review]
tests and fix

In other words, gst_pad_get_current_caps should never return NULL
in a pad-added callback from the demuxer.

Added tests for the two special cases with AAC and H.264 where this
would happen every time.
Comment 1 Nicolas Dufresne (ndufresne) 2016-03-16 19:32:46 UTC
Review of attachment 324135 [details] [review]:

Somehow it would be nice to describe the fix, it's rather simply as you only move some parsing earlier. Otherwise, looks good. We should consider this for 1.8.1 too.
Comment 2 Håvard Graff (hgr) 2016-03-16 19:38:51 UTC
(In reply to Nicolas Dufresne (stormer) from comment #1)
> Review of attachment 324135 [details] [review] [review]:
> 
> Somehow it would be nice to describe the fix, it's rather simply as you only
> move some parsing earlier. Otherwise, looks good. We should consider this
> for 1.8.1 too.

Sure!

I am always expecting to be able to call gst_pad_get_current_caps in a pad-added callback from a demuxer, to know what to link this pad onwards to. Currently this is not the case for flvdemux, since you *do* get a pad-added for H.264 and AAC even though the caps has not yet been decided. This forces you to jump through hoops with adding an additional caps-event pad-probe in the pad-added callback, and then being able to finally complete the linking on this new callback. There are also issues with stream-start then being sent on a pad that is not yet linked.

The proposed fix, as you say, is simply moving the special-casing for AAC and H.264 to before the section of code that "adds the pad". If the caps can not yet be determined, the code, as it used to, now jumps to "beach" (the end), but because the section is moved, this does now also mean that the pad will not be added as well.

The supplied tests uses a "generic" pad-added callback that will do gst_pad_get_current_caps and fail if the caps is not present, making sure this functionality is maintained even though the code might be refactored further!
Comment 3 Nicolas Dufresne (ndufresne) 2016-03-16 19:43:37 UTC
Great ! let's reuse that to update the commit message. We should add a check in gst-validate for that, as it's pretty generic.
Comment 4 Sebastian Dröge (slomo) 2016-03-24 12:35:51 UTC
commit bcbb8fc1da844cfbab2e2d950d6b506c47ee7d55
Author: Havard Graff <havard.graff@gmail.com>
Date:   Wed Mar 16 20:17:55 2016 +0100

    flvdemux: don't emit pad-added until caps are ready
    
    In other words, gst_pad_get_current_caps should never return NULL
    in a pad-added callback from the demuxer.
    
    Added tests for the two special cases with AAC and H.264 where this
    would happen every time.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=763780