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

[Nicolas George]

Le sextidi 16 ventôse, an CCXXI, "René J.V. Bertin" a écrit :
> Since my full name appears in the sign-off, yeah, why not? Not against an
> (undocumented) ffmpeg guideline I hope? :)

I believe this is your decision.

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

These instruction do not cover the guidelines to write the commit messages.

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

There is git blame for that.

> I'd prefer to keep them.

Your choice.

> IMHO size_t is perfect for counters that cannot go negative.

For counters that can not go negative, you need an unsigned type; size_t is
unsigned all right, but it is far from being the only one.

The problem with size_t is that it has a different size depending on the
architecture: your program will seem to work great on your x86_64 and may
fail on a x86_32.

size_t is for the size of objects in memory, because it is directly linked
to the limitations of the architectures.

For integers that need to handle large values, ffmpeg use (u)int64_t.

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

thanksForFixingItButIAssureYouThatCamelCaseIsInherentlyUglyJustConsider
HowPainfulItWasToReadThisSentenceSpacesWereInventedForAGoodReason.

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

patcheck is not exhaustive. Having a different coding style in a separate
file maintained regularly by someone is probably acceptable, but a lot of
people work on ffmpeg.c specifically, different coding styles within the
file is a very bad idea.

> Is that obligatory?

IMHO not, but anything that makes your code more readable will help it being
reviewed and eventually integrated.

> Do you know another way to ensure that the fraction is printed in 10
> characters, without internal spaces?

Right.

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

Yes, "static inline" is exactly made for this kind of situation.

> As you can see, it's a Microsoft type

I missed that, forget my comment.

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

Thanks for the explanation. A comment saying the same thing near the code
would help the reader.

> >> 
> >> +        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.

No problem about that.

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

No problem either.

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

You can copy-paste the license boilerplate from any file in ffmpeg; your
choice to put your name on it or not.

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

I see the point. If these functions are useful, they may have their place in
libavutil, so that they can be used elsewhere. This reminds me that
libavutil already has hi-res timers, for the START/STOP_TIMER macros: see
libavutil/timer.h. Having this kind of redundant declarations is definitely
a bad idea.

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

See my other mail.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130306/1f43f199/attachment.asc>