On 2016/12/15 7:13, Hendrik Leppkes wrote:
> On Thu, Dec 15, 2016 at 12:02 AM, Mark Thompson <s...@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.w...@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.z...@intel.com>
>>> Signed-off-by: Yi A Wang <yi.a.w...@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
>>>
>>> +@item hevc_main10
>>> +
>>> +@item h264_constrained_baseline
>>> +
>>> +@item h264_main
>>> +
>>> +@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.
>
Thanks for you clear explain, will move to private options.
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
