GNOME Bugzilla – Bug 613589
typefind: Export AAC level in caps
Last modified: 2010-04-12 14:08:52 UTC
Created attachment 156739 [details] [review] typefind: Export AAC level in caps I'm attaching a patch to export the AAC 'level' in caps. This could be used while linking (for example, a hardware decoder might support AAC LC upto level 2), and is useful metadata for certain applications (the DLNA spec requires that the level of a stream be known). I've made the level a string to account for codecs that have non-numeric levels (for example, H.264 has a level '1.b'). The level calculation code will initially be duplicated across modules (for qtdemux and matroskademux) because it's a rather smaller bit of code. Eventually, as the amount of shared AAC-related code increases, we can move this to -base as a library. I'll post the code for level guessing in qtdemux after this patch is reviewed to avoid causing bug-spam.
Created attachment 156762 [details] [review] typefind: Export AAC level in caps Minor update to the patch.
Ping on this too.
I think this is mostly blocking on the other bug (bug #612312) where we're discussing how the profiles information should be put into caps. There isn't really a reason why the level field in aac caps can't be an integer and the level field in h264 caps a string, but given that level numbers seem to be guaranteed to be <=8, making it a string and keeping the type consistent isn't too awkward, so I'm all for it.
Review of attachment 156762 [details] [review]: Couple of nitpicks: ::: gst/typefind/Makefile.am @@ -1,3 @@ plugin_LTLIBRARIES = libgsttypefindfunctions.la -libgsttypefindfunctions_la_SOURCES = gsttypefindfunctions.c The gstaacutil.h header file will need to be added somewhere too, otherwise it won't be disted. ::: gst/typefind/gstaacutil.c @@ +108,3 @@ + break; + default: + g_return_val_if_reached (-1); g_return_val_if_reached() and friends are mostly meant as guards for bad input to public API, and for programming mistakes. In case of bad data input, we should just silently return -1 (but may do a GST_WARNING or so if we want to). If we want to guard against bad arguments from other GStreamer functions (e.g.. if they should have checked bad data already), then a g_assert_if_reached() might be better. Not sure which one is the case here. @@ +114,3 @@ + /* 0xd and 0xe are reserved. 0xf means the sample frequency is directly + * specified in the header (FIXME) */ + g_return_val_if_reached (-1); See above. ::: gst/typefind/gsttypefindfunctions.c @@ +687,3 @@ snc = GST_READ_UINT16_BE (c.data + len); if ((snc & 0xfff6) == 0xfff0) { + gint mpegversion, profile, level, sample_freq_idx, channel_config; We should make the ones that are always unsigned a guint IMHO. @@ +714,3 @@ + "profile", G_TYPE_STRING, profile_to_string[profile], + "level", G_TYPE_STRING, + g_ascii_dtostr (buf, sizeof (buf), (gdouble) level), Ew :-) I don't think we should be using g_ascii_dstrostr() for integers here, even if the returns-buffer thing is rather convenient (also, if we use it, we should probably use the provided define for the buffer size). I would suggest a plain g_snprintf() before the _suggest_simple().
Created attachment 158095 [details] [review] Updated patch incorporating review Thanks for the review, Tim. Agree on all counts. Attaching an updated patch.
Committed with some minor changes: commit 43a04483d9e4f9dce2dd9351f548ceb4262601ad Author: Arun Raghavan <arun.raghavan@collabora.co.uk> Date: Mon Apr 12 13:33:18 2010 +0100 typefinding: add AAC level to ADTS caps This adds code to calculate the level for a given AAC stream and export it in the stream caps. For AAC LC streams, the level is calculated according to the definition under the AAC Profile. For other streams, the definition under the Main Profile is used. HE-AAC support is still to be done, and is dependent on detecting the presence of SBR and PS in the stream. Level is added as a field of type string because that's the way it's done in H.264 caps as well. There are only a few possible levels, so not using a numerical type is not too painful in this case, and consistency is nice. Fixes #613589.