Media bench add -hash, 64 bit IO, and some cleanup of dump

Media bench add -hash, 64 bit IO, and some cleanup of dump
BUG=21126
TEST=use media_bench -hash and it compare txt files instead of raw dump output.  Should be substantially faster.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=25563

[fbarchard, 2009-09-04 16:24:39]

hashing to speed up comparisons of large files

[Alpha Left Google, 2009-09-04 16:47:19]

LGTM

[scherkus (not reviewing), 2009-09-04 16:52:43]

http://codereview.chromium.org/195012/diff/1002/2003
File media/bench/bench.cc (right):

http://codereview.chromium.org/195012/diff/1002/2003#newcode40
Line 40: unsigned int hash_djb2(const uint8* s,
we already have hashing functions under /base

see /base/md5.h for example

http://codereview.chromium.org/195012/diff/1002/2003#newcode41
Line 41: size_t len, unsigned int hash = 5381u) {
default parameters are forbidden in style guide

http://codereview.chromium.org/195012/diff/1002/2004
File media/bench/file_protocol.cc (right):

http://codereview.chromium.org/195012/diff/1002/2004#newcode7
Line 7: #include <stdio.h>
don't think this is needed either

http://codereview.chromium.org/195012/diff/1002/2004#newcode40
Line 40: #if defined(_MSC_VER)
hmm.. any reason why we need to change this?

thought I had 64-bit already covered

nonetheless please use defined(OS_WIN)

[fbarchard, 2009-09-04 17:17:31]

http://codereview.chromium.org/195012/diff/1002/2003
File media/bench/bench.cc (right):

http://codereview.chromium.org/195012/diff/1002/2003#newcode40
Line 40: unsigned int hash_djb2(const uint8* s,
I don't see appropriate hash or CRC functions in base.  Should I add them there?

http://codereview.chromium.org/195012/diff/1002/2003#newcode41
Line 41: size_t len, unsigned int hash = 5381u) {
On 2009/09/04 16:52:46, scherkus wrote:
> default parameters are forbidden in style guide

Done.

http://codereview.chromium.org/195012/diff/1002/2004
File media/bench/file_protocol.cc (right):

http://codereview.chromium.org/195012/diff/1002/2004#newcode7
Line 7: #include <stdio.h>
needed for _fseeki64

http://codereview.chromium.org/195012/diff/1002/2004#newcode40
Line 40: #if defined(_MSC_VER)
Seems I was working on this at the same time as you.
Mine was showing casting warnings, so I fixed those.
_fseeki64 is Visual C specific, including on xbox and other platforms supported
by Visual C, and doesn't work on other windows compilers like gcc.

[awong, 2009-09-04 17:36:47]

http://codereview.chromium.org/195012/diff/1002/2003
File media/bench/bench.cc (right):

http://codereview.chromium.org/195012/diff/1002/2003#newcode40
Line 40: unsigned int hash_djb2(const uint8* s,
On 2009/09/04 17:17:31, fbarchard wrote:
> I don't see appropriate hash or CRC functions in base.  Should I add them
there?
> 

You could if  you want...on the otherhand, you could just overkill it and use
md5 or sha-something.  I'm not seeing any benefit to adding another function
(read added complexity) for something that isn't really perf critical.

[scherkus (not reviewing), 2009-09-04 20:43:43]

If you contibute to /base add deanm as a reviewer, if not that's ok.
I just recall seeing you use the same hashing function at some other
point in time (yuv unittests, maybe?).

We're not concerned with non-MSVC windows compilers. I'm fairly
certain OS_WINDOWS is a synonym for _MVC_VER anyway.

I'm losing Internet signal so deferring the review to hclam/ajwong

On Friday, September 4, 2009,  <ajwong@chromium.org> wrote:
>
> http://codereview.chromium.org/195012/diff/1002/2003
> File media/bench/bench.cc (right):
>
> http://codereview.chromium.org/195012/diff/1002/2003#newcode40
> Line 40: unsigned int hash_djb2(const uint8* s,
> On 2009/09/04 17:17:31, fbarchard wrote:
>
> I don't see appropriate hash or CRC functions in base. =A0Should I add
>
> them there?
>
>
> You could if =A0you want...on the otherhand, you could just overkill it
> and use md5 or sha-something. =A0I'm not seeing any benefit to adding
> another function (read added complexity) for something that isn't really
> perf critical.
>
> http://codereview.chromium.org/195012
>

[fbarchard, 2009-09-04 21:31:52]

I've prepared a base/djb2.h for review.  But it'll be a couple steps to get that
checked in, and then make this tool use it.  I'd suggest going ahead with this
tool asis.  If djb2 gets accepted into base, then I should go thru all the
unittests and switch over to that.  If it gets rejected, I'll keep the code asis
or switch to md5.
Note that at EA I was the hashing expert, reviewing a number of algorithms, and
this was the single most useful one.  I've implemented it in assembler several
times so it can be done in realtime.
Having familiarized myself with md5.h, I could easily use that.  But it would
take more code and run slower.  If you really want md5, I'm happy to add it. 
The main advantage would be that you can compare against the md5 tool as a
sanity check.
I'm adding hashing to speed up the regression testing, and you'd be surprised
how slow checksums can be.  By adding it, I could tell you how slow.
Extracting to YUV/PCM has the advantage that we could use psnr tool for lossy
compares.  Which is still an interesting thing thing to do, for testing changes
that sacrifice a small amount of quality, such as the flags2.

[fbarchard, 2009-09-04 22:06:08]

changed ifdef to OS_WINDOWS

[scherkus (not reviewing), 2009-09-04 22:12:51]

Cool!  My iPhone doesn't make the best code reviewing tool, so I can't
provide any more comments :P

Spliting up the reviews sounds like a good idea.  For tool development
I'd consider having reading/writing to std in/out modes so we can
chain tools together without hitting the disk.  I tried playing around
with mkfifo but msys/cygwin doesn't implement it :(

On Friday, September 4, 2009,  <fbarchard@chromium.org> wrote:
> I've prepared a base/djb2.h for review. =A0But it'll be a couple steps to
> get that checked in, and then make this tool use it. =A0I'd suggest going
> ahead with this tool asis. =A0If djb2 gets accepted into base, then I
> should go thru all the unittests and switch over to that. =A0If it gets
> rejected, I'll keep the code asis or switch to md5.
> Note that at EA I was the hashing expert, reviewing a number of
> algorithms, and this was the single most useful one. =A0I've implemented
> it in assembler several times so it can be done in realtime.
> Having familiarized myself with md5.h, I could easily use that. =A0But it
> would take more code and run slower. =A0If you really want md5, I'm happy
> to add it. =A0The main advantage would be that you can compare against th=
e
> md5 tool as a sanity check.
> I'm adding hashing to speed up the regression testing, and you'd be
> surprised how slow checksums can be. =A0By adding it, I could tell you ho=
w
> slow.
> Extracting to YUV/PCM has the advantage that we could use psnr tool for
> lossy compares. =A0Which is still an interesting thing thing to do, for
> testing changes that sacrifice a small amount of quality, such as the
> flags2.
>
> http://codereview.chromium.org/195012
>

[awong, 2009-09-04 23:33:25]

On 2009/09/04 22:12:51, scherkus wrote:
> Cool!  My iPhone doesn't make the best code reviewing tool, so I can't
> provide any more comments :P
> 
> Spliting up the reviews sounds like a good idea.  For tool development
> I'd consider having reading/writing to std in/out modes so we can
> chain tools together without hitting the disk.  I tried playing around
> with mkfifo but msys/cygwin doesn't implement it :(

Why do we want mkfifo?  Can you just to fwrite/fprintf (stdout..) or similar?

-A

> 
> On Friday, September 4, 2009,  <mailto:fbarchard@chromium.org> wrote:
> > I've prepared a base/djb2.h for review. =A0But it'll be a couple steps to
> > get that checked in, and then make this tool use it. =A0I'd suggest going
> > ahead with this tool asis. =A0If djb2 gets accepted into base, then I
> > should go thru all the unittests and switch over to that. =A0If it gets
> > rejected, I'll keep the code asis or switch to md5.
> > Note that at EA I was the hashing expert, reviewing a number of
> > algorithms, and this was the single most useful one. =A0I've implemented
> > it in assembler several times so it can be done in realtime.
> > Having familiarized myself with md5.h, I could easily use that. =A0But it
> > would take more code and run slower. =A0If you really want md5, I'm happy
> > to add it. =A0The main advantage would be that you can compare against th=
> e
> > md5 tool as a sanity check.
> > I'm adding hashing to speed up the regression testing, and you'd be
> > surprised how slow checksums can be. =A0By adding it, I could tell you ho=
> w
> > slow.
> > Extracting to YUV/PCM has the advantage that we could use psnr tool for
> > lossy compares. =A0Which is still an interesting thing thing to do, for
> > testing changes that sacrifice a small amount of quality, such as the
> > flags2.
> >
> > http://codereview.chromium.org/195012
> >

[awong, 2009-09-04 23:35:07]

http://codereview.chromium.org/195012/diff/1002/2003
File media/bench/bench.cc (right):

http://codereview.chromium.org/195012/diff/1002/2003#newcode318
Line 318: if (hash) {
Maybe collapse this to be one loop, and put the conditions for output or hash
inside the loop?

[fbarchard, 2009-09-04 23:58:48]

fixed default arg

[awong, 2009-09-05 00:02:24]

LGTM

With the one change listed below.

As to moving the loops, up to you.  There's only 2 so doesn't matter too much
IMO.

http://codereview.chromium.org/195012/diff/2013/2014
File media/bench/bench.cc (right):

http://codereview.chromium.org/195012/diff/2013/2014#newcode119
Line 119: unsigned int hash_value = 5381u;  // Seed for DJB2.
The 5381u should probably be a constant.

[fbarchard, 2009-09-05 00:03:43]

commenting on for loop

http://codereview.chromium.org/195012/diff/1002/2003
File media/bench/bench.cc (right):

http://codereview.chromium.org/195012/diff/1002/2003#newcode318
Line 318: if (hash) {
this way is more efficient and modular.  I just got in the habit of doing it the
efficient way.  I can switch it if you think its best practice doing it the
other way for some reason.

[awong, 2009-09-05 00:08:45]

>
> http://codereview.chromium.org/195012/diff/1002/2003#newcode318
> Line 318: if (hash) {
> this way is more efficient and modular.  I just got in the habit of
> doing it the efficient way.  I can switch it if you think its best
> practice doing it the other way for some reason.


I don't feel strongly.  As for efficiency, one way you avoid a branch.  The
other way you loop over everything twice.  If we were looping > 2 times,
maybe I'd start to care a bit more.

Whatever you feel like. :)

[fbarchard, 2009-09-05 00:42:04]

basics are working, but there is something wrong with --stream=audio on m4a
files.  But it works on video and audio for ogg and mp3.