[FFmpeg-devel] a new benchmarking option for ffmpeg - patch 1 of 2

"René J.V. Bertin" rjvbertin at gmail.com
Wed Mar 6 18:06:58 CET 2013


On Mar 06, 2013, at 16:55, Nicolas George wrote:

> Is it really the name you want to appear as the author of the patch?

Since my full name appears in the sign-off, yeah, why not? Not against an (undocumented) ffmpeg guideline I hope? :)

> Wrong format for the commit message. The first line should be a short
> summary of what the commit does; then an empty line, then the details. The
> Singned-off pseudo-headers come at the end.

And I did follow the instructions by using git format-patch ...

> 
>> +//RJVB
> 
> Leftovers?

Not really, I like to keep trace of who changed what.

> The _time suffix seems redundant.

I'd prefer to keep them.

>> +    size_t numSamples, numObs;
> 
> size_t does not seem like the correct type at all.

IMHO size_t is perfect for counters that cannot go negative.

> And camelCase is ugly.

No, it's just not liked by everyone. I just forgot these 2 instances.

>> 
>> +static int64_t getmaxrss(void);
> 
> Why did you need to move the forward declaration of getmaxrss()?

To get my compiler (gcc 4.7.2) to accept the function's implicit declaration in the function just below the static declaration. It wasn't used there in the version in which I developed my patch, and I don't recall adding it.

I admit it's a patch that has nothing to do with my edits, but since it's so minor I thought it'd pass with the rest.

> 
> This kind of spacing is really not ffmpeg style: else on the same line as
> the closing brace; space between the keyword and the parentheses, and
> between the parentheses, but not inside the parentheses.

I noticed - I also noticed patcheck left me free to ignore this. Which I gladly did because *I* happen to find that kind of spacing ugly, less readable and more than annoying when using vi for editing.

> Aligning the "+=" and the - xT0 would make this much more readable.

Is that obligatory?

>> +        if( cum_bin && cum_bin == bin ){
>> +          char tots[32];
>> +            bin->user_cum_time /= bin->numSamples;
> 
> Strange indentation.

A must-change? I find this makes locating local variable definitions much easier...

>> +            snprintf( tots, sizeof(tots), "%g/%lu", (double) bin->numSamples, bin->numObs );
>> +            fprintf( fp, "%s\t%10s\t%10gs\t%10gs\t%10gs\t%10g%%\n", header,
>> +                    tots, bin->user_cum_time, bin->system_cum_time, bin->total_cum_time,
>> +                    cpu );
> 
> What use is the indirect print in the "tots" buffer?

Do you know another way to ensure that the fraction is printed in 10 characters, without internal spaces?
> 
>> +#include "timing.c"
> 
> Including a whole source file is usually not a good sign. Can you explain
> why you do it?

I don't use an external file to allow inlining, and timing contains 4 conditional blocks making it a bit awkward to inline it completely. In this case the functions are used only in ffmpeg.c, so I could declare them static, and rename timing.c to timing.h ...

>>     GetProcessTimes(proc, &c, &e, &k, &u);
>> +    if( bin ){
>> +      ULARGE_INTEGER uT, kT;
> 
> Indentation is strange. And what is ULARGE_INTEGER? It does not appear
> elsewhere in your patch nor in ffmpeg.
>> +        uT.LowPart = u.dwLowDateTime, uT.HighPart = u.dwHighDateTime;
>> +        kT.LowPart = k.dwLowDateTime, kT.HighPart = k.dwHighDateTime;
>> +        bin->user_time = uT.QuadPart * 100e-9;

As you can see, it's a Microsoft type (defined through windows.h) for converting the values returned by GetProcessTimes to seconds. This is the only place where the type is required, apparently.

>> +    { int dbm = do_benchmark_most;
>> +        do_benchmark_most = 1;
>> +        update_benchmark( &ffmpeg_bench, NULL );
>> +        do_benchmark_most = dbm;
>> +    }
>> +#endif
> 
> It looks like a hack. Can you add a comment to explain what is going on?

It's a hack indeed, to initialise the overall benchmarking bin before do_benchmark_most is set (by the user) and without introducing an additional argument that's required only here. Storing the initial value is a bit redundant as it only protects the probably few people who want do_benchmark_most to default to 1 and force that in the code...
>> 
>> +        fprintf( stdout, "Detailed benchmark results:\n" );
>> +        fprintf( stdout, "               \t%10s\t%10s\t%10s\t%10s\t%10s\n", "samples", "user t", "kernel t", "real t", "CPU %" );
> 
> Why fprintf(stdout) instead of just printf?

Personal preference; at least the destination file is explicit, and one cannot forget to add an f upon deciding to send the output elsewhere than to stdout.

You may have noticed that I also use curly braces systematically, even where they're not obligatory, and for much the same reason.

>> 
> 
>> +/*!
>> + *  @file QTImage2Mov/source/timing.c
>> + *
>> + *  (C) RenE J.V. Bertin on 20080926.
>> + *
>> + */
> 
> Missing license boilerplate.

Anything specific I'm supposed to put there? I don't care about the type of license for timing.[ch] - in fact, the code is so simple that I'm not even sure I am adamant on claiming a copyright on it...

> I can not comment on this file, lacking the knowledge of most of the
> platforms you endeavour to handle. But it seems pretty complicated for a
> minor feature.

It's complicated only because of the conditional blocks, but for benchmarking I wouldn't call the functions a minor feature. Apart from the fallback version using gettimeofday, they give access to (very) high resolution timers that are not expensive computationally speaking. Which means they cause about as little perturbation of the measured process as is probably possible using library functions.
A moot point for sure for the overall timer, but not for the ones that are called before and after each individual operation.

>> \ No newline at end of file

Aargh, I took those for unwanted trailing whitespace ...


> Please squash both patches into one (git rebase -i): there is really no need
> to add a file with unusual coding style only to fix it immediately.

Sorry, I'm not enough of a git expert just yet ... and I didn't find a way to use git format-patch *before* committing the files I modified...




More information about the ffmpeg-devel mailing list