[FFmpeg-devel] [PATCH 2/2] vaapi_h265: Add named options for setting profile and level

[Hendrik Leppkes]

On Thu, Nov 30, 2017 at 10:40 AM, Li, Zhong <zhong.li at intel.com> wrote:
>> -----Original Message-----
>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
>> Of Mark Thompson
>> Sent: Thursday, November 30, 2017 7:30 AM
>> To: ffmpeg-devel at ffmpeg.org
>> Subject: [FFmpeg-devel] [PATCH 2/2] vaapi_h265: Add named options for
>> setting profile and level
>>
>> Also fixes the default, which previously contained a nonsense value.
>> ---
>> On 29/11/17 03:51, Li, Zhong wrote:
>> >> On 28/11/17 07:46, Ruiling Song wrote:
>> >>> Signed-off-by: Ruiling Song <ruiling.song at intel.com>
>> >>> ---
>> >>>  libavcodec/vaapi_encode_h265.c | 2 +-
>> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>>
>> >>> diff --git a/libavcodec/vaapi_encode_h265.c
>> >>> b/libavcodec/vaapi_encode_h265.c index 3ae92a7..32b8bc6 100644
>> >>> --- a/libavcodec/vaapi_encode_h265.c
>> >>> +++ b/libavcodec/vaapi_encode_h265.c
>> >>> @@ -219,7 +219,7 @@ static int
>> >> vaapi_encode_h265_init_sequence_params(AVCodecContext *avctx)
>> >>>          .general_non_packed_constraint_flag = 1,
>> >>>          .general_frame_only_constraint_flag = 1,
>> >>>
>> >>> -        .general_level_idc     = avctx->level,
>> >>> +        .general_level_idc     = avctx->level * 3,
>> >>>      };
>> >>>
>> >>> vps->profile_tier_level.general_profile_compatibility_flag[avctx->pr
>> >>> vps->of
>> >>> ile & 31] = 1;
>> >>>
>> >>>
>> >> The documentation has always said "profile and level set the values
>> >> of general_profile_idc and general_level_idc respectively"
>> >> (<http://ffmpeg.org/ffmpeg-codecs.html#VAAPI-encoders>) - the code
>> >> disagreed for a while, but that was made consistent in
>> >> 00179664bccd1dd6fa0d1c40db453528757bf6f7.
>> >>
>> >> Why do you want to change it to be general_level_idc / 3 instead?
>> >
>> > As HEVC spec, "general_level_idc and sub_layer_level_idc[ i ] shall be
>> > set equal to a value of 30 times the level number specified in Table A.4."
>> > And use the tool "mediainfo" to check the encoded bitstream, it show the
>> level is 1.7 if set the option "-level" to be 51.
>> >
>> > Maybe the documentation
>> ((<http://ffmpeg.org/ffmpeg-codecs.html#VAAPI-encoders>)) also needs to
>> be changed?
>>
>> Hmm, right - the default is wrong, so you get a nonsense value there.
>>
>> How about we fix that and add named constants to make it clearer, like this?
>
> I think it is a good idea, except one situation：
> Suppose user set the level to be 41, they may want an encoded bitstream with level 4.1, right?
> But actually the output level is 1.4 (using mediainfo to check this), currently we are forcing them to set the option to be 4.1 or 123, right?
> Haven't verify your patch, just infer from the code.


But 41 is never 4.1 for HEVC, so using a scheme that was basically
"invented" doesn't seem right (probably as a carry-over from H264).
When decoding, avctx->level contains 123, not 41, for example, or for
a closer match, NVENC accepts -profile:v 123 or -profile:v 4.1, but
not 41.
I think its more important to stay consistent here (both to the HEVC
spec and other HEVC encoders), instead of catering to an imaginary
value that is never actually used in relation to HEVC.

- Hendrik