GNOME Bugzilla – Bug 617314
pbutils: Add codec-specific utility functions for AAC, H.264, MPEG-4 video
Last modified: 2010-09-30 14:15:24 UTC
This is a set of patches to add codec-specific utility functions. Currently, this includes profile/level extraction code for AAC, H.264, and MPEG-4 Video. The code for these would otherwise need to be replicated across typefindfunctions/demuxers. The patches make typefindfunctions use the factored out AAC code as well. I'll post patches for qtdemux/matroskademux to use these functions separately.
Created attachment 159997 [details] [review] pbutils: Add codec-specific utility functions This allows us to add generic codec-specific functionality, like extracting profile/level data from headers, without having to duplicate code across demuxers and typefindfunctions. As a starting point, this moves over AAC level extraction code from typefindfunctions, so it can be reused in qtdemux, etc.
Created attachment 159998 [details] [review] pbutils: Add AAC profile detection This moves AAC profile detection to pbutils, and uses this in typefindfunctions. This will also be used in qtdemux.
Created attachment 159999 [details] [review] pbutils: Add H.264 profile/level extraction functions This adds code to parse the first few bytes of H.264 sequence parameter set in order to extract the profile and level as const strings. This code was originally in both qtdemux and matroskademux.
Created attachment 160000 [details] [review] pbutils: Add MPEG-4 Video profile/level extraction This adds code to translate the profile_and_level indication from the MPEG-4 video (ISO/IEC 14496-2) headers to a string profile/level. The mappings are taken from the spec and Wireshark's code, and might need to be expanded on.
Tim, any reason why you didn't commit this yet?
Yes, I don't fully like the API yet. Pondering alternatives. Will look at it tomorrow.
Ok, so some thoughts: - I like having these utility functions in a central place, and pbutils seems as good a place as any - I think we should drop the pb_utils from the API, so e.g. gst_codec_utils_aac_*(), gst_codec_utils_h264_*() etc. (it's not very consistent within pbutils, but I think this is nicer). - there's still a bit of a mishmash between parsing bits caller side and not, e.g. _aac_get_sample_rate() makes the caller parse the header/codec data to extract the sample rate index, while other functions do not. I think it would be nicer API-wise if the caller didn't have to do that. If needed, one would have to have an _get_sample_rate() and e.g. a _get_sample_rate_from_adts_header() or somesuch. - while the _get_foo() API is fairly clean and in line with what we do elsewhere, I wonder if it wouldn't be better to have functions that operate on, or return, caps straight away, instead of doing the whole foo = get_foo(); if (foo); gst_caps_set_simple (...); dance caller side. Then you'd just have e.g. gst_codec_utils_aac_get_caps() etc. Thoughts?
(In reply to comment #7) [...] > - I think we should drop the pb_utils from the API, so e.g. > > gst_codec_utils_aac_*(), > gst_codec_utils_h264_*() I find the pb_utils prefix a little messy too, but chose it for consistency. codec_utils does sound better, so let's go with that. [...] > - there's still a bit of a mishmash between parsing bits > caller side and not, e.g. _aac_get_sample_rate() makes > the caller parse the header/codec data to extract the > sample rate index, while other functions do not. I think The sample rate conversion function was really more of a pattern I saw being used in multiple places while parsing the header, with the lookup table being defined in multiple places. I don't see the function being of much value in any other context, and the caller is going to be walking through the header anyway. [...] > - while the _get_foo() API is fairly clean and in line with > what we do elsewhere, I wonder if it wouldn't be better > to have functions that operate on, or return, caps > straight away, instead of doing the whole > foo = get_foo(); if (foo); gst_caps_set_simple (...); > dance caller side. Then you'd just have e.g. > gst_codec_utils_aac_get_caps() > etc. True, the check-return-and-set-caps repetition is ugly. Only concern here is if this function may be used to only extract the profile/level at some point. We could a small wrapper like gst_codec_utils_aac_set_profile_on_caps (though I'd much rather have something shorter :)).
I think in the long run, we want a support lib for every format that is handled by more than one plugin, as that simplifies writing plugins a lot and helps in getting stuff right the first time. A place where I think that's particularly important is handling caps properly. I don't have a lot of experience with codecs, so I have no good idea how to handle it there, but I'll share some experience from my gst-plugins-cairo work. (You can read the header files in http://cgit.freedesktop.org/~company/gst-plugins-cairo/tree/gst-libs/gst/cairo if you want to follow along) The first thing I did is make sure the code pretty much never needs to modify caps directly. Once you get a fixed caps that you want to handle, you run format = gst_cairo_format_new (caps); on it. You then have a GstCairoFormat API that provides exactly te utility functions you care about. Of course, getting caps out of a format works with gst_cairo_format_to_caps() again. For a codec, I'd do something like GstH264Format *gst_h264_format_new_from_data (GstBuffer *codec_data) and GstBuffer *gst_h264_format_to_data (GstH264Format *format) in addition to the caps functions. Then elements can use the format struct to seamlessly transform between data and caps and modify the format using getters and setters. The current functions to me don't look like a very good public API. They seem to be just a bunch of utility functions dumped into some file - I think this can be done better with a bit of thought. You want to have caps code that's as trivial as http://cgit.freedesktop.org/~company/gst-plugins-cairo/tree/gst/cairo/gstcairocolorspace.c#n42 (and that code does more than ffmpegcolorspace).
> I think in the long run, we want a support lib for every format that is handled > by more than one plugin I am not sure we want separate mini libs for every major format, but let's stick to the short run for this bug. > For a codec, I'd do something like GstH264Format *gst_h264_format_new_from_data > (GstBuffer *codec_data) and GstBuffer *gst_h264_format_to_data (GstH264Format > *format) in addition to the caps functions. > Then elements can use the format struct to seamlessly transform between data > and caps and modify the format using getters and setters. I'm not sure this is needed here, since we usually don't need to keep any of this stuff around. Also, it's basically a parse-stuff-into-helper-structures approach, which we tend not to do (and where we have done it in the past, we want to change it in 0.11), and we didn't chose it on purpose for e.g. the stuff in libgstvideo. > The current functions to me don't look like a very good public API. They seem > to be just a bunch of utility functions dumped into some file - I think this > can be done better with a bit of thought. I don't think this is bad API at all. Some more concrete suggestions / criticisms would be nice. > You want to have caps code that's as trivial as > http://cgit.freedesktop.org/~company/gst-plugins-cairo/tree/gst/cairo/gstcairocolorspace.c#n42 > (and that code does more than ffmpegcolorspace). As far as I can tell, there's not much overlap between your cairo stuff and the codec stuff here. You need to handle fixation, negotiation, expansion, etc. We need to parse and evaluate stuff here according to certain rules (e.g. to determine the level). Completely different use cases.
FWIW, the API looks fine/convenient. Having some caps setter wrapper is indeed likely also useful (rather than having this repeated all over the demuxers). Note that gst_codec_utils_caps_set_aac_profile is both nicely consistent with e.g. gst_caps_set_simple and shorter ;)
Created attachment 162332 [details] [review] pbutils: Add codec-specific utility functions This allows us to add generic codec-specific functionality, like extracting profile/level data from headers, without having to duplicate code across demuxers and typefindfunctions. As a starting point, this moves over AAC level extraction code from typefindfunctions, so it can be reused in qtdemux, etc.
Created attachment 162333 [details] [review] pbutils: Add AAC profile detection This moves AAC profile detection to pbutils, and uses this in typefindfunctions. This will also be used in qtdemux.
Created attachment 162334 [details] [review] pbutils: Add H.264 profile/level extraction functions This adds code to parse the first few bytes of H.264 sequence parameter set in order to extract the profile and level as const strings. This code was originally in both qtdemux and matroskademux.
Created attachment 162335 [details] [review] pbutils: Add MPEG-4 Video profile/level extraction This adds code to translate the profile_and_level indication from the MPEG-4 video (ISO/IEC 14496-2) headers to a string profile/level. The mappings are taken from the spec and Wireshark's code, and might need to be expanded on.
Updated patches to: * Use a gst_codec_utils_* prefix * Introduce setter functions prefixed as gst_codec_utils_<codec>_caps_set_{profile,level}
Looks (still) good to me.
Pushed most of these now. Sorry it took so long! I combined the _caps_set_profile() and _caps_set_level() functions into one single _caps_set_level_and_profile() function for each format, which is a bit bulky, but seems to reflect the usage better (no point in having two separate functions if both are always called together) Also did a bunch of minor changes: - renamed gst_codec_utils_aac_get_sample_rate() to _rate_from_index() - added some g_return_val_if_fail() guards - moved some static vars around / mucked with const arrays a bit - dereference data only after the size check - required the "mpegversion" field to be set on the aac caps for _set_level_and_profile() instead of having it as an argument commit 515a768dbc0b1b3ae1fc6265771b8bc8a776966a Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Thu Sep 30 00:19:29 2010 +0100 docs: add new codec utils API to docs commit def1cf727634698e31accbce259d565a20856673 Author: Arun Raghavan <arun.raghavan@collabora.co.uk> Date: Sat May 1 01:03:18 2010 +0530 pbutils: Add MPEG-4 Video profile/level extraction This adds code to translate the profile_and_level indication from the MPEG-4 video (ISO/IEC 14496-2) headers to a string profile/level. The mappings are taken from the spec and Wireshark's code, and might need to be expanded on. https://bugzilla.gnome.org/show_bug.cgi?id=617314 API: gst_codec_utils_mpeg4video_get_profile() API: gst_codec_utils_mpeg4video_get_level() API: gst_codec_utils_mpeg4video_caps_set_level_and_profile() commit e4bd09f0ed3f33a202eac94149feb8452548f1ff Author: Arun Raghavan <arun.raghavan@collabora.co.uk> Date: Fri Apr 30 20:50:09 2010 +0530 pbutils: add H.264 profile/level extraction functions to codec utils This adds code to parse the first few bytes of H.264 sequence parameter set in order to extract the profile and level as const strings. This code was originally in both qtdemux and matroskademux. https://bugzilla.gnome.org/show_bug.cgi?id=617314 API: gst_codec_utils_h264_get_level() API: gst_codec_utils_h264_get_profile() API: gst_codec_utils_h264_caps_set_level_and_profile() commit 0cf81938a160a9c16dc4903766dbea76f166ffe5 Author: Arun Raghavan <arun.raghavan@collabora.co.uk> Date: Fri Apr 30 15:12:04 2010 +0530 pbutils: add AAC profile detection to codec utils This moves AAC profile detection to pbutils, and uses this in typefindfunctions. This will also be used in qtdemux. https://bugzilla.gnome.org/show_bug.cgi?id=617314 API: gst_codec_utils_aac_get_profile() API: codec_utils_aac_caps_set_level_and_profile() commit c77f88cac675a1dbb89e40da8e3c28320523bfca Author: Arun Raghavan <arun.raghavan@collabora.co.uk> Date: Fri Apr 30 13:41:17 2010 +0530 pbutils: add codec-specific utility functions for AAC This allows us to add generic codec-specific functionality, like extracting profile/level data from headers, without having to duplicate code across demuxers and typefindfunctions. As a starting point, this moves over AAC level extraction code from typefindfunctions, so it can be reused in qtdemux, etc. https://bugzilla.gnome.org/show_bug.cgi?id=617314 API: gst_codec_utils_aac_get_sample_rate_from_index() API: gst_codec_utils_aac_get_level()