media_bench optionally dump raw output to a file....

media_bench optionally dump raw output to a file.
BUG=20709
TEST=use ffmpeg to dump raw yuv and compare to media_bench output.


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

[scherkus (not reviewing), 2009-09-02 22:58:23]

http://codereview.chromium.org/171113/diff/1/2
File media/bench/bench.cc (right):

http://codereview.chromium.org/171113/diff/1/2#newcode20
Line 20: #include "base/file_util.h"
if file_util::ScopedFILE isn't being used, do we need this?

http://codereview.chromium.org/171113/diff/1/2#newcode27
Line 27: using file_util::ScopedFILE;
this doesn't seem used

http://codereview.chromium.org/171113/diff/1/2#newcode39
Line 39: bool dump_output = false;
you can determine this by checking for a non-empty out_path

http://codereview.chromium.org/171113/diff/1/2#newcode46
Line 46: std::cerr << "Usage: media_bench [OPTIONS] FILE [DUMPFILE]\n"
curious.. reason why it's not an --option ?

http://codereview.chromium.org/171113/diff/1/2#newcode124
Line 124: 
remove blank line

http://codereview.chromium.org/171113/diff/1/2#newcode127
Line 127: FILE *output = NULL;
hmm.. ScopedFILE doesn't work here?

http://codereview.chromium.org/171113/diff/1/2#newcode131
Line 131: LOG(ERROR) << "could not open output";
the rest if this file is using std::cerr as opposed to LOG(ERROR)

http://codereview.chromium.org/171113/diff/1/2#newcode244
Line 244: if (dump_output) {
nit: check output instead and you && the operation together

http://codereview.chromium.org/171113/diff/1/2#newcode246
Line 246: LOG(ERROR) << "could not write data after " << size_out;
should we return 1; here?

http://codereview.chromium.org/171113/diff/1/2#newcode258
Line 258: if (dump_output) {
nit: check output instead and you && the operation together

http://codereview.chromium.org/171113/diff/1/2#newcode260
Line 260: LOG(ERROR) << "could not write data after " << got_picture;
should we return 1; here?

[fbarchard, 2009-09-03 02:06:09]

yuv dumping tested on windows.
pcm should work, but not tested

http://codereview.chromium.org/171113/diff/1/2
File media/bench/bench.cc (right):

http://codereview.chromium.org/171113/diff/1/2#newcode20
Line 20: #include "base/file_util.h"
On 2009/09/02 22:58:23, scherkus wrote:
> if file_util::ScopedFILE isn't being used, do we need this?
seems to be needed
2>bench.cc
2>.\bench\bench.cc(123) : error C2653: 'file_util' : is not a class or namespace
name
2>.\bench\bench.cc(123) : error C2660: 'OpenFile' : function does not take 2
arguments
2>.\bench\bench.cc(233) : error C2664: 'avcodec_decode_audio3' : cannot convert
parameter 3 from 'size_t *' to 'int *'
2>        Types pointed to are unrelated; conversion requires reinterpret_cast,
C-style cast or function-style cast
2>.\bench\bench.cc(295) : error C2653: 'file_util' : is not a class or namespace
name
2>.\bench\bench.cc(295) : error C3861: 'CloseFile': identifier not found
2>Build log was saved at
"file://C:\chrome-svn\src\media\Release\obj\media_bench\BuildLog.htm"
2>media_bench - 5 error(s), 0 warning(s)
========== Build: 1 succeeded, 1 failed, 7 up-to-date, 0 skipped ==========

http://codereview.chromium.org/171113/diff/1/2#newcode27
Line 27: using file_util::ScopedFILE;
On 2009/09/02 22:58:23, scherkus wrote:
> this doesn't seem used

Done.

http://codereview.chromium.org/171113/diff/1/2#newcode39
Line 39: bool dump_output = false;
Its not initialized... not sure what the behavior will be, but okay

http://codereview.chromium.org/171113/diff/1/2#newcode46
Line 46: std::cerr << "Usage: media_bench [OPTIONS] FILE [DUMPFILE]\n"
Its fairly conventional to have
tool in out
with filenames not as options.

http://codereview.chromium.org/171113/diff/1/2#newcode124
Line 124: 
On 2009/09/02 22:58:23, scherkus wrote:
> remove blank line

Done.

http://codereview.chromium.org/171113/diff/1/2#newcode127
Line 127: FILE *output = NULL;
no, it closed the file as soon as it leaves the if scope.

http://codereview.chromium.org/171113/diff/1/2#newcode131
Line 131: LOG(ERROR) << "could not open output";
On 2009/09/02 22:58:23, scherkus wrote:
> the rest if this file is using std::cerr as opposed to LOG(ERROR)

Done.

http://codereview.chromium.org/171113/diff/1/2#newcode244
Line 244: if (dump_output) {
changed to just output

http://codereview.chromium.org/171113/diff/1/2#newcode246
Line 246: LOG(ERROR) << "could not write data after " << size_out;
early outs are problematic, but okay

http://codereview.chromium.org/171113/diff/1/2#newcode258
Line 258: if (dump_output) {
On 2009/09/02 22:58:23, scherkus wrote:
> nit: check output instead and you && the operation together

Done.

http://codereview.chromium.org/171113/diff/1/2#newcode260
Line 260: LOG(ERROR) << "could not write data after " << got_picture;
On 2009/09/02 22:58:23, scherkus wrote:
> should we return 1; here?

Done.

[scherkus (not reviewing), 2009-09-03 18:00:55]

few more nits but we're looking good!

http://codereview.chromium.org/171113/diff/1/2
File media/bench/bench.cc (right):

http://codereview.chromium.org/171113/diff/1/2#newcode39
Line 39: bool dump_output = false;
On 2009/09/03 02:06:09, fbarchard wrote:
> Its not initialized... not sure what the behavior will be, but okay

std::[w]strings are initialized to empty, so we're safe here (part of the reason
why we don't allow global static std::strings)

http://codereview.chromium.org/171113/diff/1/2#newcode127
Line 127: FILE *output = NULL;
On 2009/09/03 02:06:09, fbarchard wrote:
> no, it closed the file as soon as it leaves the if scope.
> 

that doesn't seem right... ScopedFILE is a specialized version of
scoped_ptr_malloc<>, which should only release itself when it goes completely
out of scope

seeing as you've declared this at the highest level scope it should live for the
rest of main {}

http://codereview.chromium.org/171113/diff/8001/4002#newcode126
Line 126: // Open output file.
std::cerr

http://codereview.chromium.org/171113/diff/8001/4002#newcode255
Line 255: ++frames;
you should do a CHECK or NOTIMPLEMENTED on codec_context->pix_fmt instead of
unknown behaviour

http://codereview.chromium.org/171113/diff/8001/4002#newcode270
Line 270: 
move the << to the next line and align it at the << (this is the one exception
for formatting when doing printing or logging messages)

[Alpha Left Google, 2009-09-03 18:08:58]

LGTM after style issues by scherkus are addressed.

[scherkus (not reviewing), 2009-09-03 22:03:35]

woah some stuff slipped through.. create a new CL and TBR if you want

http://codereview.chromium.org/171113/diff/8004/8005
File media/bench/bench.cc (right):

http://codereview.chromium.org/171113/diff/8004/8005#newcode26
Line 26: #include "media/filters/ffmpeg_video_decoder.h"
hmm.. don't think you need to include this

http://codereview.chromium.org/171113/diff/8004/8005#newcode123
Line 123: FILE *output = NULL;
pointer goes with typename

http://codereview.chromium.org/171113/diff/8004/8005#newcode256
Line 256: // TODO(fbarchard): support formats other than YV12.
this comment is out of date

http://codereview.chromium.org/171113/diff/8004/8005#newcode273
Line 273: copy_lines = copy_lines;
redundant?

http://codereview.chromium.org/171113/diff/8004/8005#newcode277
Line 277: copy_lines = copy_lines;
redundant?