[FFmpeg-devel] [PATCH 1/2] avutil/timer: define macos kperf as AV_READ_TIME

[Martin Storsjö]

On Mon, 17 Jun 2024, Zhao Zhili wrote:

>
>
>> On Jun 17, 2024, at 19:15, Martin Storsjö <martin at martin.st> wrote:
>> 
>> On Wed, 12 Jun 2024, Zhao Zhili wrote:
>> 
>>> From: Zhao Zhili <zhilizhao at tencent.com>
>>> 
>>> Firstly, make ff_kperf_cycles as an implementation of AV_READ_TIME
>>> avoids code duplication.
>>> 
>>> Secondly, fix compilation error since 6a18c0bc87e when macos-kperf
>>> is enabled. mach_time.h is included only when CONFIG_MACOS_KPERF
>>> is 0. The error happened due to define mach_absolute_time as
>>> AV_READ_TIME but missing include mach_time.h. Define macos kperf
>>> as AV_READ_TIME fixed the issue.
>> 
>> Can you elaborate on what your actual goal is here? We have relatively little use of AV_READ_TIME (mostly START/STOP_TIMER), while most benchmarking these days is done via checkasm. Do you have a real case where you want to do benchmarking with this api, outside of checkasm?
>> 
>> Or do you just want to fix the compilation error? In that case I guess it's possible to fix differently by adding the missing includes.
>> 
>> By doing this change, we'd be adding one call to ff_thread_once to every single invocation of the timers - which seems suboptimal (even if it probably is quite quick). We don't use Linux perf for AV_READ_TIME either, we only use it in checkasm. So I'd prefer not to do this change, especially unless you have a concrete case where you actively desire to use START/STOP_TIMER benchmarking with macOS kperf?
>
> I’m trying to fix the missing include header file first. Then I saw 
> ff_kperf_init() is called each time by START_TIMER, which can be 
> simplified by merge ff_kperf_init into ff_kperf_cycles.
>
> #define START_TIMER                                                         \
>    uint64_t tperf;                                                         \
>    ff_kperf_init();                                                        \
>    tperf = ff_kperf_cycles();
>
> Now I think I have chose the wrong example. checkasm bench_init_kperf is 
> the right one.

Oh, right, I had entirely missed that we already do this - so both Linux 
perf and macOS kperf are used for START/STOP_TIMER, they're just not used 
for AV_READ_TIME so far. I see...

> We can remove the ff_thread_once in ff_kperf_init, and let caller make 
> guarantee to only call it once. But kperf is only for test, so not 
> urgent to do such change.

In any case, I much rather have ff_thread_once in an _init function, than 
in every single timer invocation. Not sure if it's worth trying to get rid 
of the ff_thread_once from there.

// Martin