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

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).
* 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(-)

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.

> * 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?

>  {
> -    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;
> +}
> +
> +
> +const H265ProfileDescriptor *ff_h265_get_profile(int profile)
> +{
> +    av_assert0(profile > H265_PROFILE_INVALID &&
> +               profile < H265_PROFILE_COUNT);
> +
> +    return &h265_profiles[profile];
> +}
> +
> +const H265ProfileDescriptor *ff_h265_find_profile(const H265RawProfileTierLevel *ptl)
> +{
> +    for (int p = 1; p < H265_PROFILE_COUNT; p++) {
> +        if (ff_h265_profile_compatible(ptl, p))
> +            return &h265_profiles[p];
>      }
>  
>      return NULL;
> @@ -171,12 +193,12 @@ const H265LevelDescriptor *ff_h265_guess_level(const H265RawProfileTierLevel *pt
>      int i;
>  
>      if (ptl)
> -        profile = ff_h265_get_profile(ptl);
> +        profile = ff_h265_find_profile(ptl);
>      else
>          profile = NULL;
>      if (!profile) {
>          // Default to using multiplication factors for Main profile.
> -        profile = &h265_profiles[4];
> +        profile = &h265_profiles[H265_PROFILE_MAIN];
>      }
>  
>      pic_size = width * height;
> diff --git a/libavcodec/h265_profile_level.h b/libavcodec/h265_profile_level.h
> index cd30ac5c50..e4e63dfef1 100644
> --- a/libavcodec/h265_profile_level.h
> +++ b/libavcodec/h265_profile_level.h
> @@ -24,6 +24,50 @@
>  #include "cbs_h265.h"
>  
>  
> +// Enumeration of all H.265 profiles.
> +// The list is ordered to match table A.10; numeric values are an index
> +// into the latest version of this table and have no codec meaning.
> +enum {
> +    H265_PROFILE_INVALID,
> +    H265_PROFILE_MONOCHROME,
> +    H265_PROFILE_MONOCHROME_10,
> +    H265_PROFILE_MONOCHROME_12,
> +    H265_PROFILE_MONOCHROME_16,
> +    H265_PROFILE_MAIN,
> +    H265_PROFILE_SCREEN_EXTENDED_MAIN,
> +    H265_PROFILE_MAIN_10,
> +    H265_PROFILE_SCREEN_EXTENDED_MAIN_10,
> +    H265_PROFILE_MAIN_12,
> +    H265_PROFILE_MAIN_STILL_PICTURE,
> +    H265_PROFILE_MAIN_10_STILL_PICTURE,
> +    H265_PROFILE_MAIN_422_10,
> +    H265_PROFILE_MAIN_422_12,
> +    H265_PROFILE_MAIN_444,
> +    H265_PROFILE_HIGH_THROUGHPUT_444,
> +    H265_PROFILE_SCREEN_EXTENDED_MAIN_444,
> +    H265_PROFILE_SCREEN_EXTENDED_HIGH_THROUGHPUT_444,
> +    H265_PROFILE_MAIN_444_10,
> +    H265_PROFILE_HIGH_THROUGHPUT_444_10,
> +    H265_PROFILE_SCREEN_EXTENDED_MAIN_444_10,
> +    H265_PROFILE_SCREEN_EXTENDED_HIGH_THROUGHPUT_444_10,
> +    H265_PROFILE_MAIN_444_12,
> +    H265_PROFILE_HIGH_THROUGHPUT_444_14,
> +    H265_PROFILE_SCREEN_EXTENDED_HIGH_THROUGHPUT_444_14,
> +    H265_PROFILE_MAIN_INTRA,
> +    H265_PROFILE_MAIN_10_INTRA,
> +    H265_PROFILE_MAIN_12_INTRA,
> +    H265_PROFILE_MAIN_422_10_INTRA,
> +    H265_PROFILE_MAIN_422_12_INTRA,
> +    H265_PROFILE_MAIN_444_INTRA,
> +    H265_PROFILE_MAIN_444_10_INTRA,
> +    H265_PROFILE_MAIN_444_12_INTRA,
> +    H265_PROFILE_MAIN_444_16_INTRA,
> +    H265_PROFILE_MAIN_444_STILL_PICTURE,
> +    H265_PROFILE_MAIN_444_16_STILL_PICTURE,
> +    H265_PROFILE_HIGH_THROUGHPUT_444_16_INTRA,
> +    H265_PROFILE_COUNT,
> +};
> +
>  typedef struct H265LevelDescriptor {
>      char        name[4];    // Large enough for all current levels like "4.1"
>      uint8_t     level_idc;
> @@ -70,7 +114,32 @@ typedef struct H265ProfileDescriptor {
>  } H265ProfileDescriptor;
>  
>  
> -const H265ProfileDescriptor *ff_h265_get_profile(const H265RawProfileTierLevel *ptl);
> +/**
> + * Test whether the PTL structure is compatible with the given profile.
> + *
> + * @param ptl      PTL structure to check.
> + * @param profile  Profile to test compatibility with.  Must be a valid
> + *                 H265_PROFILE_* value.
> + * @return  Nonzero if compatible, zero if incompatible.
> + */
> +const int ff_h265_profile_compatible(const H265RawProfileTierLevel *ptl,
> +                                     int profile);
> +
> +/**
> + * Return profile descriptor for the given profile.
> + *
> + * @param profile  Profile number; must be a valid H265_PROFILE_* value.
> + * @return  Profile descriptor.
> + */
> +const H265ProfileDescriptor *ff_h265_get_profile(int profile);
> +
> +/**
> + * Find the first profile compatible with the given PTL structure.
> + *
> + * @param ptl  PTL structure to search for.
> + * @return  First compatible profile, or NULL if no compatible profiles.
> + */
> +const H265ProfileDescriptor *ff_h265_find_profile(const H265RawProfileTierLevel *ptl);
>  
>  
>  /**
> diff --git a/libavcodec/tests/h265_levels.c b/libavcodec/tests/h265_levels.c
> index 66d72c63a3..9923f346f7 100644
> --- a/libavcodec/tests/h265_levels.c
> +++ b/libavcodec/tests/h265_levels.c
> @@ -275,7 +275,7 @@ int main(void)
>      }
>  
>      for (i = 0; i < FF_ARRAY_ELEMS(test_bitrate); i++) {
> -        profile = ff_h265_get_profile(test_bitrate[i].ptl);
> +        profile = ff_h265_find_profile(test_bitrate[i].ptl);
>          level = ff_h265_guess_level(test_bitrate[i].ptl,
>                                      test_bitrate[i].bitrate,
>                                      0, 0, 0, 0, 0, 0);
> diff --git a/libavcodec/vaapi_hevc.c b/libavcodec/vaapi_hevc.c
> index 3bdd2dd1b8..77f55ff8b1 100644
> --- a/libavcodec/vaapi_hevc.c
> +++ b/libavcodec/vaapi_hevc.c
> @@ -604,7 +604,7 @@ VAProfile ff_vaapi_parse_hevc_rext_scc_profile(AVCodecContext *avctx)
>      /* convert PTLCommon to H265RawProfileTierLevel */
>      ptl_convert(general_ptl, &h265_raw_ptl);
>  
> -    profile = ff_h265_get_profile(&h265_raw_ptl);
> +    profile = ff_h265_find_profile(&h265_raw_ptl);
>      if (!profile) {
>          av_log(avctx, AV_LOG_WARNING, "HEVC profile is not found.\n");
>          goto end;
> diff --git a/libavcodec/vdpau_hevc.c b/libavcodec/vdpau_hevc.c
> index 4cd7ce5621..a59a030b1c 100644
> --- a/libavcodec/vdpau_hevc.c
> +++ b/libavcodec/vdpau_hevc.c
> @@ -478,7 +478,7 @@ static int vdpau_hevc_parse_rext_profile(AVCodecContext *avctx, VdpDecoderProfil
>      /* convert PTLCommon to H265RawProfileTierLevel */
>      ptl_convert(general_ptl, &h265_raw_ptl);
>  
> -    profile = ff_h265_get_profile(&h265_raw_ptl);
> +    profile = ff_h265_find_profile(&h265_raw_ptl);
>      if (!profile) {
>          av_log(avctx, AV_LOG_WARNING, "HEVC profile is not found.\n");
>          if (avctx->hwaccel_flags & AV_HWACCEL_FLAG_ALLOW_PROFILE_MISMATCH) {

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