[FFmpeg-devel] [PATCH v2 1/3] lavc/h265_profile_level: Expand profile compatibility checking

[Mark Thompson]

On 06/05/2024 20:56, Andreas Rheinhardt wrote:
> Mark Thompson:
>> Replace existing get_profile() with find_profile(), which finds the
>> lowest compatible profile rather than requiring an exact match.
>> ---
>> Series changes since v1:
>> * Added H265_PROFILE_INVALID with value zero because it simplifies some code (and the values don't matter).
> 
> What code is simplified by using zero (instead of -1)? I only see that
> it leads to the addition of an unnecessary entry to h265_profiles.

The large number of empty entries in profile_table in the test work by being zero-initialised.  Filling them with -1 would not be fun.

(I originally put that table (and the bprint function) in h265_profile_level.c, but when no library caller was using it I moved it to the test.)

>> * Fixed the h265-levels test.
>> * Added a new h265-profiles test which tests the profile compatibility checking.
>> * Fixed missing VAAPI profiles.
>>
>>
>>  libavcodec/h265_profile_level.c | 78 +++++++++++++++++++++------------
>>  libavcodec/h265_profile_level.h | 71 +++++++++++++++++++++++++++++-
>>  libavcodec/tests/h265_levels.c  |  2 +-
>>  libavcodec/vaapi_hevc.c         |  2 +-
>>  libavcodec/vdpau_hevc.c         |  2 +-
>>  5 files changed, 123 insertions(+), 32 deletions(-)
>>
>> diff --git a/libavcodec/h265_profile_level.c b/libavcodec/h265_profile_level.c
>> index 7ff9681f65..2e4a1c88bf 100644
>> --- a/libavcodec/h265_profile_level.c
>> +++ b/libavcodec/h265_profile_level.c
>> @@ -40,6 +40,7 @@ static const H265LevelDescriptor h265_levels[] = {
>>  };
>>  
>>  static const H265ProfileDescriptor h265_profiles[] = {
>> +    { "Invalid" },
>>      // profile_idc   8bit       one-picture
>>      //   HT-profile  | 422chroma    | lower-bit-rate
>>      //   |  14bit    |  | 420chroma |  | CpbVclFactor     MinCrScaleFactor
>> @@ -119,41 +120,62 @@ static const H265ProfileDescriptor h265_profiles[] = {
>>        5, 1, 0, 0, 0, 0, 0, 0, 0, 1, 0, 2, 4000, 4400, 6.000, 0.5, 6 },
>>  };
>>  
>> +_Static_assert(H265_PROFILE_COUNT == FF_ARRAY_ELEMS(h265_profiles),
>> +               "Incorrect H.265 profiles table.");
>>  
>> -const H265ProfileDescriptor *ff_h265_get_profile(const H265RawProfileTierLevel *ptl)
>> +
>> +const int ff_h265_profile_compatible(const H265RawProfileTierLevel *ptl,
>> +                                     int profile)
> 
> Why don't you add a tag to this enum and pass a proper enum here?

No reason.  I can add an explicit type if you prefer that?

>>  {
>> -    int i;
>> +    const H265ProfileDescriptor *desc;
>> +
>> +    av_assert0(profile > H265_PROFILE_INVALID &&
>> +               profile < H265_PROFILE_COUNT);
>>  
>>      if (ptl->general_profile_space)
>> -        return NULL;
>> +        return 0;
>>  
>> -    for (i = 0; i < FF_ARRAY_ELEMS(h265_profiles); i++) {
>> -        const H265ProfileDescriptor *profile = &h265_profiles[i];
>> +    desc = &h265_profiles[profile];
>>  
>> -        if (ptl->general_profile_idc &&
>> -            ptl->general_profile_idc != profile->profile_idc)
>> -            continue;
>> -        if (!ptl->general_profile_compatibility_flag[profile->profile_idc])
>> -            continue;
>> +    if (ptl->general_profile_idc &&
>> +        ptl->general_profile_idc != desc->profile_idc)
>> +        return 0;
>> +    if (!ptl->general_profile_compatibility_flag[desc->profile_idc])
>> +        return 0;
>>  
>> -#define check_flag(name) \
>> -        if (profile->name < 2) { \
>> -            if (profile->name != ptl->general_ ## name ## _constraint_flag) \
>> -                continue; \
>> -        }
>> -        check_flag(max_14bit);
>> -        check_flag(max_12bit);
>> -        check_flag(max_10bit);
>> -        check_flag(max_8bit);
>> -        check_flag(max_422chroma);
>> -        check_flag(max_420chroma);
>> -        check_flag(max_monochrome);
>> -        check_flag(intra);
>> -        check_flag(one_picture_only);
>> -        check_flag(lower_bit_rate);
>> +#define check_flag(flag) \
>> +    if (desc->flag < 2 && \
>> +        desc->flag > ptl->general_ ## flag ## _constraint_flag) \
>> +        return 0;
>> +    check_flag(max_14bit);
>> +    check_flag(max_12bit);
>> +    check_flag(max_10bit);
>> +    check_flag(max_8bit);
>> +    check_flag(max_422chroma);
>> +    check_flag(max_420chroma);
>> +    check_flag(max_monochrome);
>> +    check_flag(intra);
>> +    check_flag(one_picture_only);
>> +    check_flag(lower_bit_rate);
>>  #undef check_flag
>>  
>> -        return profile;
>> +    return 1;
>> +}
>> +
>> ...

Thanks,

- Mark