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

From: "Martin Storsjö" <martin@martin.st>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Cc: Zhao Zhili <zhilizhao@tencent.com>, "J. Dekker" <jdek@itanimul.li>
Subject: Re: [FFmpeg-devel] [PATCH 2/2] avutil/macos_kperf: Fix assert which makes kperf failed to run
Date: Mon, 17 Jun 2024 14:10:39 +0300 (EEST)
Message-ID: <d54da2f7-4028-d7f-abf1-eb50923e32e8@martin.st> (raw)
In-Reply-To: <tencent_8C54698E039569F27C0AAD9A3979C1E1B50A@qq.com>

On Wed, 12 Jun 2024, Zhao Zhili wrote:

> From: Zhao Zhili <zhilizhao@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.

// Martin

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".