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 612313 - qtdemux: Post AAC profile/level in caps
qtdemux: Post AAC profile/level in caps
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: 0.10.26
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 617314
Blocks:
 
 
Reported: 2010-03-09 16:36 UTC by Arun Raghavan
Modified: 2010-10-05 18:52 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch to expose AAC profile as a tag (2.32 KB, patch)
2010-03-09 16:37 UTC, Arun Raghavan
none Details | Review
qtdemux: Post AAC profile in caps (1.96 KB, patch)
2010-03-10 13:46 UTC, Arun Raghavan
none Details | Review
Updated patch (2.39 KB, patch)
2010-03-19 16:51 UTC, Arun Raghavan
none Details | Review
qtdemux: Export AAC level in caps (2.54 KB, patch)
2010-04-30 19:54 UTC, Arun Raghavan
none Details | Review
qtdemux: Export AAC profile in caps (1.72 KB, patch)
2010-04-30 19:55 UTC, Arun Raghavan
none Details | Review
qtdemux: Export AAC profile and level in caps (2.55 KB, patch)
2010-05-30 20:10 UTC, Arun Raghavan
committed Details | Review
qtdemux: AAC codec_data can be > 2 bytes long (2.52 KB, patch)
2010-10-04 09:56 UTC, Arun Raghavan
reviewed Details | Review

Description Arun Raghavan 2010-03-09 16:36:44 UTC
This patch looks at the ESDS atom for the AAC profile, and if present, export it as a tag.
Comment 1 Arun Raghavan 2010-03-09 16:37:23 UTC
Created attachment 155660 [details] [review]
patch to expose AAC profile as a tag
Comment 2 Arun Raghavan 2010-03-10 13:46:49 UTC
Created attachment 155745 [details] [review]
qtdemux: Post AAC profile in caps

Post the profile in caps instead of a tag.
Comment 3 Arun Raghavan 2010-03-19 16:51:52 UTC
Created attachment 156565 [details] [review]
Updated patch

Updated patch with a pointer to bug #612312 for notes on how to use the profile string.
Comment 4 Arun Raghavan 2010-04-30 19:54:54 UTC
Created attachment 160002 [details] [review]
qtdemux: Export AAC level in caps

This exports the AAC level in caps for use as metadata and (eventually)
for more fine-grained selection of decoders at caps-negotiation time.
Comment 5 Arun Raghavan 2010-04-30 19:55:00 UTC
Created attachment 160003 [details] [review]
qtdemux: Export AAC profile in caps

This exports the profile for AAC streams in caps, for use as metadata
and (eventually) finer-grained decoder-selection.
Comment 6 Arun Raghavan 2010-04-30 19:56:36 UTC
Comment on attachment 156565 [details] [review]
Updated patch

The profile/level guessing code has been moved to pbutils in bug #617314, so we can avoid code duplication. Attached patches to use the utility functions.
Comment 7 Arun Raghavan 2010-05-30 20:10:50 UTC
Created attachment 162337 [details] [review]
qtdemux: Export AAC profile and level in caps

This exports the AAC profile and level in caps for use as metadata and
(eventually) for more fine-grained selection of decoders at
caps-negotiation time.
Comment 8 Arun Raghavan 2010-05-30 20:12:06 UTC
Updated (and simpler) patch to use setter functions. Since the changes are quite small, I've merged the two patches.
Comment 9 Tim-Philipp Müller 2010-10-01 10:45:18 UTC
Ok, I've pushed this now:

 commit 845a3d6c3d1b013c2450f0d09a0c1e583e7ab4cf
 Author: Arun Raghavan <arun.raghavan@collabora.co.uk>
 Date:   Fri Apr 30 14:06:27 2010 +0530

    qtdemux: Export AAC profile and level in caps
    
    This exports the AAC profile and level in caps for use as metadata and
    (eventually) for more fine-grained selection of decoders at
    caps-negotiation time. (Doesn't work for HE-AAC yet though.)
    
    https://bugzilla.gnome.org/show_bug.cgi?id=612313

along with an unrelated fix for the channel override in this code section.


Main question I have is this: why is the _set_level_and_profile() call within the if (codec_data_len == 2) block? Shouldn't it be outside that block, with the codec utils function handling this correctly?

It seems to me that the caller of the codec utils functions shouldn't have to worry about the HE-AAC thing - if we have to worry about that, then maybe the codec utils API isn't quite right (not passed enough information or somesuch).
Comment 10 Arun Raghavan 2010-10-04 09:56:25 UTC
Created attachment 171679 [details] [review]
qtdemux: AAC codec_data can be > 2 bytes long

This fixes the assumption that DecoderSpecificInfo must be 2 bytes long
for AAC files. The specification allows HE-AAC to be explicitly
signalled in a backward compatible way. This is done by means of an
additional information after the regular AAC header. It is expected that
decoders that can play AAC but not HE-AAC will parse the header normally
and ignore extended bits, much as they do for the HEAAC specific payload
in the actual stream.
Comment 11 Arun Raghavan 2010-10-04 10:00:10 UTC
Tim: thanks for pushing all these patches!

You're right - the codec utils should handle the HE-AAC bits when they get added. This may not always be possible (for implicitly signalled HE-AAC, you know that it is HE-AAC when you see the first SBR payload in the actual stream - no extra data is provided in headers). However, for the cases where it is explicitly signalled, we should be handle it in the codec utils function.

I've attached a patch that removes the assumption about the header length and  lets the codec utils function decide how to extract the profile/level.
Comment 12 Tim-Philipp Müller 2010-10-05 18:51:28 UTC
I took the liberty to change your patch a little, I don't think it does what you meant to do (we want to keep the data_len == 2 check for the part where we override the stream parameters):

 commit 4a244e0d552f01d17b04de2cfcfb270ecd848521
 Author: Arun Raghavan <arun.raghavan@collabora.co.uk>
 Date:   Tue Oct 5 19:40:50 2010 +0100

    qtdemux: AAC codec_data can be > 2 bytes long
    
    This fixes the assumption that DecoderSpecificInfo must be 2 bytes long
    for AAC files. The specification allows HE-AAC to be explicitly
    signalled in a backward compatible way. This is done by means of an
    additional information after the regular AAC header. It is expected that
    decoders that can play AAC but not HE-AAC will parse the header normally
    and ignore extended bits, much as they do for the HE-AAC specific payload
    in the actual stream.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=612313

Now we just need to fix up the codec utils a little for HE-AAC. I'll file a new bug for that.