[FFmpeg-devel] [PATCH] lavc/vaapi_encode_h264: add option to indicate the h264 encode profile

[Hendrik Leppkes]

On Thu, Dec 15, 2016 at 12:02 AM, Mark Thompson <sw at jkqxz.net> wrote:
> On 14/12/16 01:55, Jun Zhao wrote:
>> From 03030392ec2458679cdfb14802b80cbb196eae40 Mon Sep 17 00:00:00 2001
>> From: Yi A Wang <yi.a.wang at intel.com>
>> Date: Tue, 13 Dec 2016 10:50:54 +0800
>> Subject: [PATCH] lavc/vaapi_encode_h264: add option to indicate the h264
>>  encode profile
>>
>> add h264 encode profile option and update the docs, for avc
>> constrained baseline, disable B frames base on H.264 spec Annex A.2.1
>>
>> Signed-off-by: Jun Zhao <jun.zhao at intel.com>
>> Signed-off-by: Yi A Wang <yi.a.wang at intel.com>
>> ---
>>  doc/codecs.texi                | 8 ++++++++
>>  libavcodec/options_table.h     | 5 ++++-
>>  libavcodec/vaapi_encode_h264.c | 5 +++++
>>  3 files changed, 17 insertions(+), 1 deletion(-)
>
> Notwithstanding the below, this should probably be two patches (one for options/docs, one for VAAPI).
>
>
>> diff --git a/doc/codecs.texi b/doc/codecs.texi
>> index 9a3a56d..9ee9061 100644
>> --- a/doc/codecs.texi
>> +++ b/doc/codecs.texi
>> @@ -893,6 +893,14 @@ Possible values:
>>
>>  @item dts_hd_ma
>>
>> + at item hevc_main10
>> +
>> + at item h264_constrained_baseline
>> +
>> + at item h264_main
>> +
>> + at item h264_high
>> +
>>  @end table
>>
>>  @item level @var{integer} (@emph{encoding,audio,video})
>> diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h
>> index 3fe7925..94b2d9b 100644
>> --- a/libavcodec/options_table.h
>> +++ b/libavcodec/options_table.h
>> @@ -395,7 +395,10 @@ static const AVOption avcodec_options[] = {
>>  {"mpeg4_core", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_MPEG4_CORE }, INT_MIN, INT_MAX, V|E, "profile"},
>>  {"mpeg4_main", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_MPEG4_MAIN }, INT_MIN, INT_MAX, V|E, "profile"},
>>  {"mpeg4_asp",  NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_MPEG4_ADVANCED_SIMPLE }, INT_MIN, INT_MAX, V|E, "profile"},
>> -{"main10",  NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_HEVC_MAIN_10 }, INT_MIN, INT_MAX, V|E, "profile"},
>
> This table is part of the public API of libavcodec, you can't remove things from it like this.
>
>> +{"hevc_main10",  NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_HEVC_MAIN_10 }, INT_MIN, INT_MAX, V|E, "profile"},
>> +{"h264_constrained_baseline", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_H264_CONSTRAINED_BASELINE}, INT_MIN, INT_MAX, V|E, "profile"},
>> +{"h264_main", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_H264_MAIN}, INT_MIN, INT_MAX, V|E, "profile"},
>> +{"h264_high", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_H264_HIGH}, INT_MIN, INT_MAX, V|E, "profile"},
>
> This seems reasonable, but I'd like to hear from other people why there is only a small subset of video profiles there already?  Confusingly, the libx264, nvenc and videotoolbox encoders all have private options (also called "profile") which implement exactly the same thing where they could be using the generic one (were it to support suitable options).
>
> If the change is desirable, it should probably get the full set of profiles corresponding to FF_PROFILE_H264_* values rather than just a restricted set coming from what VAAPI exposes.  (Also deprecate the anomalous "main10" and add the H.265 profiles properly as well, I guess?)
>

The reason for private profiles is quite simply that you don't want to
type "hevc_main10", but just "main10", for example. On top of that,
how far do you take this, do you put every single codecs profiles into
the shared table? Or just the ones where we have multiple encoders?
What if they don't support the same profiles? You wouldn't have any
easy way to get a list of thigns it does support (which you could do
with private options)
It gets complicated fast.

I would argue that vaapi should just follow the trend and use its own
private profiles, unless we decide to change all of them one day.
Until that point, if someone wanted, they could factor the profile
lists out of current code and share them in a header/c file somewhere
to be able to be reused.

Unfortunately I didn't pay attention when main10 was added, or that
might have been avoided, as its inconsistent with any other options we
have in there and the only hevc profile.

- Hendrik