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 705024 - aacparse: does not propagate downstream sample rate restriction upstream
aacparse: does not propagate downstream sample rate restriction upstream
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-07-28 13:55 UTC by Christian Fredrik Kalager Schaller
Modified: 2013-12-09 14:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
dotfile graph (1.00 MB, image/png)
2013-07-28 13:55 UTC, Christian Fredrik Kalager Schaller
Details
Another similar looking issue, but this time the capsfilter seems to be compatible (980.34 KB, image/png)
2013-07-28 14:33 UTC, Christian Fredrik Kalager Schaller
Details

Description Christian Fredrik Kalager Schaller 2013-07-28 13:55:10 UTC
Created attachment 250301 [details]
dotfile graph

I am having trouble transcoding a file in Transmageddon, as you can see if the attached dotfile graph the issue is that I try to set a rate value with a capsfilter, but the element that should provide that new rate doesn't seem aware of it.
Comment 1 Christian Fredrik Kalager Schaller 2013-07-28 14:33:04 UTC
Created attachment 250303 [details]
Another similar looking issue, but this time the capsfilter seems to be compatible
Comment 2 Tim-Philipp Müller 2013-07-28 21:57:10 UTC
This is with up-to-date git master?

I suspect some element doesn't proxy the caps query properly, most likely streamcombiner. Moving tentatively to encodebin for now.

And it should be audioresample before the encoder that does the rate conversion.

Could you provide a GST_DEBUG=*:6 log for when it doesn't work?
Comment 3 Christian Fredrik Kalager Schaller 2013-07-29 07:59:41 UTC
Yes, git master of everything except gst-libav.

GST_DEBUG log can be found here:
http://uraeus.fedorapeople.org/output.txt.bz2
Comment 4 Tim-Philipp Müller 2013-08-03 14:37:10 UTC
This looks like a bug in aacparse:

gst-launch-1.0 audiotestsrc ! audioconvert ! audio/x-raw,rate=48000,channels=2 ! audioresample ! faac ! aacparse ! audio/mpeg,rate=44100 ! fakesink


Remove the aacparse, and it works.
Replace faac ! aacparse with lamemp3enc ! mpegaudioparse, and it works.
Comment 5 Sebastian Dröge (slomo) 2013-08-13 12:56:04 UTC
I was told that this was introduced by this commit:

commit ca4b5d795bf52ea434470475e2dac6a149282c10
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Tue Jun 5 09:18:12 2012 +0200

    audioparsers: Fix GstBaseParse::get_sink_caps() implementations
    
    They should take the filter caps into account and always return
    the template caps appended to the actual caps. Otherwise the
    parsers stop to accept unparsed streams where upstream does not
    know about channels, rate, etc.
    
    Fixes bug #677401.



The problem here is that if downstream has rate=44100 as a constraint we would need to pass that upstream. However we can't pass that constraint upstream because unparsed AAC streams might not have the rate on the caps.
Comment 6 Wim Taymans 2013-12-03 16:10:14 UTC
> 
> 
> The problem here is that if downstream has rate=44100 as a constraint we would
> need to pass that upstream. However we can't pass that constraint upstream
> because unparsed AAC streams might not have the rate on the caps.


In this case, the template caps add the exact property that is also being constrained (rate=(int){ 8000, 11025, 12000, 16000, 22050, 24000, ...). If this is the case, the contraint should be applied to those added properties. Not sure how to express that...
Comment 7 Sebastian Dröge (slomo) 2013-12-03 17:12:51 UTC
Well, no. The template caps don't contain the rate field, otherwise we could just fix this problem by putting the constraints into the caps we append. However that will require all elements to put the sample rate in AAC caps, which is a value that aacparse extracts and not everything knows.
Comment 8 Wim Taymans 2013-12-03 17:39:50 UTC
(In reply to comment #5)
> I was told that this was introduced by this commit:
> 
> commit ca4b5d795bf52ea434470475e2dac6a149282c10
> Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
> Date:   Tue Jun 5 09:18:12 2012 +0200
> 
>     audioparsers: Fix GstBaseParse::get_sink_caps() implementations
> 
>     They should take the filter caps into account and always return
>     the template caps appended to the actual caps. Otherwise the
>     parsers stop to accept unparsed streams where upstream does not
>     know about channels, rate, etc.
> 
>     Fixes bug #677401.
> 
> 
> 
> The problem here is that if downstream has rate=44100 as a constraint we would
> need to pass that upstream. However we can't pass that constraint upstream
> because unparsed AAC streams might not have the rate on the caps.

I think the trick is to proxy the constraints but implement a custom getcaps function that only intersects so that it can also allow missing fields.
Comment 9 Wim Taymans 2013-12-03 21:28:44 UTC
commit e0a5c07e8d404e72654e016fa0a6c88d573f51f1
Author: Wim Taymans <wtaymans@redhat.com>
Date:   Tue Dec 3 21:41:28 2013 +0100

    audioparsers: use ACCEPT_INTERSECT flag
    
    The parser can accept input that is not completely specified. Use the
    ACCEPT_INTERSECT flag on the sinkpad to tweak the acceptcaps function to
    check for intersection only. This allows us to proxy downstream
    constraints while still allowing non-subset caps as input.
    We can then also remove the appended template caps workaround.
    Make a unit-test to check the new feature.
    
    This reverts commit 26040ee38cb9e7c42f3d9a0282b3e5cace7ca42d
    
    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=705024
Comment 10 Sebastian Dröge (slomo) 2013-12-04 12:49:08 UTC
Wim, this probably needs an update in part-caps and/or part-negotiation now. There it says that caps are considered compatible if they're a subset.

Otherwise this should work, but somehow feels not ideal :) I don't have a better solution though
Comment 11 Wim Taymans 2013-12-09 14:01:44 UTC
(In reply to comment #10)
> Wim, this probably needs an update in part-caps and/or part-negotiation now.
> There it says that caps are considered compatible if they're a subset.
> 
> Otherwise this should work, but somehow feels not ideal :) I don't have a
> better solution though

I think the ideal solution would be to mark some properties as mandatory and others optional. I also think that this would complicate things even more.
Comment 12 Sebastian Dröge (slomo) 2013-12-09 14:05:11 UTC
Yes, let's not go that road :)