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

[Zhao Zhili <quinkblack@foxmail.com>]

> On Jun 17, 2024, at 19:15, Martin Storsjö <martin@martin.st> wrote:
> 
> On Wed, 12 Jun 2024, Zhao Zhili wrote:
> 
>> From: Zhao Zhili <zhilizhao@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.

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.

Will add missing include in v2.

> 
> // 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".

_______________________________________________
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".