[FFmpeg-devel] [PATCH 2/2] avutil/macos_kperf: Fix assert which makes kperf failed to run

[J. Dekker]

On Mon, Jun 17, 2024, at 13:10, Martin Storsjö wrote:
> On Wed, 12 Jun 2024, Zhao Zhili wrote:
>
>> From: Zhao Zhili <zhilizhao at tencent.com>
>>
>> On m1, kpc_get_counter_count(KPC_MASK) return 8. The exact value
>> doesn't matter in our case.
>
> This is somewhat unexpected, I had expected that this API was originally 
> tested on an m1. I guess it might depend on what OS version you're using 
> as well?
>
>> ---
>> libavutil/macos_kperf.c | 15 +++++++++------
>> 1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/libavutil/macos_kperf.c b/libavutil/macos_kperf.c
>> index a0bc845fd3..906b276a34 100644
>> --- a/libavutil/macos_kperf.c
>> +++ b/libavutil/macos_kperf.c
>> @@ -67,14 +67,15 @@ KPERF_LIST
>> #define KPC_CLASS_POWER_MASK        (1 << 2)
>> #define KPC_CLASS_RAWPMU_MASK       (1 << 3)
>>
>> -#define COUNTERS_COUNT 10
>> +#define KPC_MAX_COUNTERS 32
>> #define CONFIG_COUNT 8
>> #define KPC_MASK (KPC_CLASS_CONFIGURABLE_MASK | KPC_CLASS_FIXED_MASK)
>>
>> static void kperf_init(void)
>> {
>> -    uint64_t config[COUNTERS_COUNT] = {0};
>> +    uint64_t config[CONFIG_COUNT] = {0};
>
> Hmm, this changes the array from 10 to 8 elements. While the change looks 
> reasonable based on the variable names, I just wanted to doublecheck that 
> we have some clues that this is right?
>
>>     void *kperf = NULL;
>> +    uint32_t n;
>>
>>     av_assert0(kperf = dlopen("/System/Library/PrivateFrameworks/kperf.framework/Versions/A/kperf", RTLD_LAZY));
>>
>> @@ -82,8 +83,10 @@ static void kperf_init(void)
>>     KPERF_LIST
>> #undef F
>>
>> -    av_assert0(kpc_get_counter_count(KPC_MASK) == COUNTERS_COUNT);
>> -    av_assert0(kpc_get_config_count(KPC_MASK) == CONFIG_COUNT);
>> +    n = kpc_get_counter_count(KPC_MASK);
>> +    av_assert0(n <= KPC_MAX_COUNTERS);
>> +    n = kpc_get_config_count(KPC_MASK);
>> +    av_assert0(n <= CONFIG_COUNT);
>
> I guess this is the actual functional change here, I think this seems 
> right.
>
> I CC's Josh on this change too, in case he has something to add here, but 
> it looks mostly reasonable to me.

It’s a private kernel API there are no guarantees about it working at any point. I just know that it used to work. If it needs changes to work currently and this fixes it for you then that’s fine, you should probably also port this change to dav1d. I’m personally unable to test this as I am just running Linux on my M1 machine now.

-- 
jd