Re: [FFmpeg-devel] [PATCH v2 1/3] lavc/h265_profile_level: Expand profile compatibility checking
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 = _profiles[i];
>> +desc = _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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2 1/3] lavc/h265_profile_level: Expand profile compatibility checking
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 = _profiles[i];
> +desc = _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 _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 _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 = _profiles[4];
> +profile = _profiles[H265_PROFILE_MAIN];
> }
>
>
[FFmpeg-devel] [PATCH 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(-)
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)
{
-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 = _profiles[i];
+desc = _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 _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 _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 = _profiles[4];
+profile = _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,
+
