Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
199049:
Media Bench file IO redux to address mp4 parsing issue. (Closed)
|
Created:
11 years, 3 months ago by fbarchard Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com, scherkus (not reviewing), awong Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionMedia Bench file IO redux to address mp4 parsing issue.
BUG=21322
TEST=run media_bench on regression corpus. Old version crashes/fails. New version should report errors and/or work.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=26072
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 8
Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 10
Patch Set 7 : '' #
Total comments: 12
Patch Set 8 : '' #Patch Set 9 : '' #Patch Set 10 : '' #
Total comments: 22
Patch Set 11 : '' #Messages
Total messages: 19 (0 generated)
fixed the file io and added exception handler
file_protocol.cc might be problematic -- getting some advice rest looks sane, but media bench should be refactored at some point.. it's getting long!! http://codereview.chromium.org/199049/diff/6001/7001 File media/bench/bench.cc (right): http://codereview.chromium.org/199049/diff/6001/7001#newcode10 Line 10: // This tool requires FFMPeg DLL's built with --enable-protocol=file. could you also remove this comment? http://codereview.chromium.org/199049/diff/6001/7001#newcode31 Line 31: #pragma warning(disable:4509) where is the actual warning generated? base\compiler_specific.h includes macros for disabling warnings for MSVC http://codereview.chromium.org/199049/diff/6001/7001#newcode138 Line 138: #ifdef _MSC_VER #if defined(OS_WIN) http://codereview.chromium.org/199049/diff/6001/7001#newcode400 Line 400: #ifdef _MSC_VER #if defined(OS_WIN)
done http://codereview.chromium.org/199049/diff/6001/7001 File media/bench/bench.cc (right): http://codereview.chromium.org/199049/diff/6001/7001#newcode10 Line 10: // This tool requires FFMPeg DLL's built with --enable-protocol=file. On 2009/09/09 01:11:52, scherkus wrote: > could you also remove this comment? Done. http://codereview.chromium.org/199049/diff/6001/7001#newcode31 Line 31: #pragma warning(disable:4509) FYI I took this verbatim from Chrome's exception handler. I tried the compiler_specific.h but it still produced the warning. It doesnt seem to be commonly used. The errors I got when using the macro instead are 2>bench.cc 2>.\bench\bench.cc(405) : error C2220: warning treated as error - no 'object' file generated 2>.\bench\bench.cc(405) : warning C4509: nonstandard extension used: 'main' uses SEH and 'threads' has destructor 2> .\bench\bench.cc(104) : see declaration of 'threads' 2>.\bench\bench.cc(405) : warning C4509: nonstandard extension used: 'main' uses SEH and 'stream' has destructor 2> .\bench\bench.cc(91) : see declaration of 'stream' 2>.\bench\bench.cc(405) : warning C4509: nonstandard extension used: 'main' uses SEH and 'out_path' has destructor 2> .\bench\bench.cc(83) : see declaration of 'out_path' 2>.\bench\bench.cc(405) : warning C4509: nonstandard extension used: 'main' uses SEH and 'in_path' has destructor 2> .\bench\bench.cc(82) : see declaration of 'in_path' 2>.\bench\bench.cc(405) : warning C4509: nonstandard extension used: 'main' uses SEH and 'filenames' has destructor 2> .\bench\bench.cc(55) : see declaration of 'filenames' 2>.\bench\bench.cc(405) : warning C4509: nonstandard extension used: 'main' uses SEH and 'exit_manager' has destructor 2> .\bench\bench.cc(50) : see declaration of 'exit_manager' 2>.\bench\bench.cc(406) : warning C4509: nonstandard extension used: 'main' uses SEH and 'threads' has destructor 2> .\bench\bench.cc(104) : see declaration of 'threads' 2>.\bench\bench.cc(406) : warning C4509: nonstandard extension used: 'main' uses SEH and 'stream' has destructor 2> .\bench\bench.cc(91) : see declaration of 'stream' 2>.\bench\bench.cc(406) : warning C4509: nonstandard extension used: 'main' uses SEH and 'out_path' has destructor 2> .\bench\bench.cc(83) : see declaration of 'out_path' 2>.\bench\bench.cc(406) : warning C4509: nonstandard extension used: 'main' uses SEH and 'in_path' has destructor 2> .\bench\bench.cc(82) : see declaration of 'in_path' 2>.\bench\bench.cc(406) : warning C4509: nonstandard extension used: 'main' uses SEH and 'filenames' has destructor 2> .\bench\bench.cc(55) : see declaration of 'filenames' 2>.\bench\bench.cc(406) : warning C4509: nonstandard extension used: 'main' uses SEH and 'exit_manager' has destructor 2> .\bench\bench.cc(50) : see declaration of 'exit_manager' 2>Build log was saved at "file://C:\chrome-svn\src\media\Release\obj\media_bench\BuildLog.htm" 2>media_bench - 1 error(s), 12 warning(s) ========== Build: 1 succeeded, 1 failed, 7 up-to-date, 0 skipped ========== The errors I get if I dont use the pragma at all are: The warning is almost every line 2>.\bench\bench.cc(150) : error C2220: warning treated as error - no 'object' file generated 2>.\bench\bench.cc(150) : warning C4509: nonstandard extension used: 'main' uses SEH and 'threads' has destructor 2> .\bench\bench.cc(108) : see declaration of 'threads' 2>.\bench\bench.cc(150) : warning C4509: nonstandard extension used: 'main' uses SEH and 'stream' has destructor 2> .\bench\bench.cc(95) : see declaration of 'stream' 2>.\bench\bench.cc(150) : warning C4509: nonstandard extension used: 'main' uses SEH and 'out_path' has destructor 2> .\bench\bench.cc(87) : see declaration of 'out_path' 2>.\bench\bench.cc(150) : warning C4509: nonstandard extension used: 'main' uses SEH and 'in_path' has destructor 2> .\bench\bench.cc(86) : see declaration of 'in_path' 2>.\bench\bench.cc(150) : warning C4509: nonstandard extension used: 'main' uses SEH and 'filenames' has destructor 2> .\bench\bench.cc(59) : see declaration of 'filenames' 2>.\bench\bench.cc(150) : warning C4509: nonstandard extension used: 'main' uses SEH and 'exit_manager' has destructor 2> .\bench\bench.cc(54) : see declaration of 'exit_manager' 2>.\bench\bench.cc(159) : warning C4509: nonstandard extension used: 'main' uses SEH and 'threads' has destructor 2> .\bench\bench.cc(108) : see declaration of 'threads' 2>.\bench\bench.cc(159) : warning C4509: nonstandard extension used: 'main' uses SEH and 'stream' has destructor 2> .\bench\bench.cc(95) : see declaration of 'stream' 2>.\bench\bench.cc(159) : warning C4509: nonstandard extension used: 'main' uses SEH and 'out_path' has destructor 2> .\bench\bench.cc(87) : see declaration of 'out_path' 2>.\bench\bench.cc(159) : warning C4509: nonstandard extension used: 'main' uses SEH and 'in_path' has destructor 2> .\bench\bench.cc(86) : see declaration of 'in_path' 2>.\bench\bench.cc(159) : warning C4509: nonstandard extension used: 'main' uses SEH and 'filenames' has destructor 2> .\bench\bench.cc(59) : see declaration of 'filenames' 2>.\bench\bench.cc(159) : warning C4509: nonstandard extension used: 'main' uses SEH and 'exit_manager' has destructor 2> .\bench\bench.cc(54) : see declaration of 'exit_manager' 2>.\bench\bench.cc(166) : warning C4509: nonstandard extension used: 'main' uses SEH and 'threads' has destructor 2> .\bench\bench.cc(108) : see declaration of 'threads' 2>.\bench\bench.cc(166) : warning C4509: nonstandard extension used: 'main' uses SEH and 'stream' has destructor 2> .\bench\bench.cc(95) : see declaration of 'stream' 2>.\bench\bench.cc(166) : warning C4509: nonstandard extension used: 'main' uses SEH and 'out_path' has destructor 2> .\bench\bench.cc(87) : see declaration of 'out_path' 2>.\bench\bench.cc(166) : warning C4509: nonstandard extension used: 'main' uses SEH and 'in_path' has destructor 2> .\bench\bench.cc(86) : see declaration of 'in_path' 2>.\bench\bench.cc(166) : warning C4509: nonstandard extension used: 'main' uses SEH and 'filenames' has destructor 2> .\bench\bench.cc(59) : see declaration of 'filenames' 2>.\bench\bench.cc(166) : warning C4509: nonstandard extension used: 'main' uses SEH and 'exit_manager' has destructor 2> .\bench\bench.cc(54) : see declaration of 'exit_manager' 2>.\bench\bench.cc(194) : warning C4509: nonstandard extension used: 'main' uses SEH and 'threads' has destructor 2> .\bench\bench.cc(108) : see declaration of 'threads' 2>.\bench\bench.cc(194) : warning C4509: nonstandard extension used: 'main' uses SEH and 'stream' has destructor 2> .\bench\bench.cc(95) : see declaration of 'stream' 2>.\bench\bench.cc(194) : warning C4509: nonstandard extension used: 'main' uses SEH and 'out_path' has destructor 2> .\bench\bench.cc(87) : see declaration of 'out_path' 2>.\bench\bench.cc(194) : warning C4509: nonstandard extension used: 'main' uses SEH and 'in_path' has destructor 2> .\bench\bench.cc(86) : see declaration of 'in_path' 2>.\bench\bench.cc(194) : warning C4509: nonstandard extension used: 'main' uses SEH and 'filenames' has destructor 2> .\bench\bench.cc(59) : see declaration of 'filenames' 2>.\bench\bench.cc(194) : warning C4509: nonstandard extension used: 'main' uses SEH and 'exit_manager' has destructor 2> .\bench\bench.cc(54) : see declaration of 'exit_manager' 2>.\bench\bench.cc(226) : warning C4509: nonstandard extension used: 'main' uses SEH and 'threads' has destructor 2> .\bench\bench.cc(108) : see declaration of 'threads' 2>.\bench\bench.cc(226) : warning C4509: nonstandard extension used: 'main' uses SEH and 'stream' has destructor 2> .\bench\bench.cc(95) : see declaration of 'stream' 2>.\bench\bench.cc(226) : warning C4509: nonstandard extension used: 'main' uses SEH and 'out_path' has destructor 2> .\bench\bench.cc(87) : see declaration of 'out_path' 2>.\bench\bench.cc(226) : warning C4509: nonstandard extension used: 'main' uses SEH and 'in_path' has destructor 2> .\bench\bench.cc(86) : see declaration of 'in_path' 2>.\bench\bench.cc(226) : warning C4509: nonstandard extension used: 'main' uses SEH and 'filenames' has destructor 2> .\bench\bench.cc(59) : see declaration of 'filenames' 2>.\bench\bench.cc(226) : warning C4509: nonstandard extension used: 'main' uses SEH and 'exit_manager' has destructor 2> .\bench\bench.cc(54) : see declaration of 'exit_manager' 2>.\bench\bench.cc(237) : warning C4509: nonstandard extension used: 'main' uses SEH and 'threads' has destructor 2> .\bench\bench.cc(108) : see declaration of 'threads' 2>.\bench\bench.cc(237) : warning C4509: nonstandard extension used: 'main' uses SEH and 'stream' has destructor 2> .\bench\bench.cc(95) : see declaration of 'stream' 2>.\bench\bench.cc(237) : warning C4509: nonstandard extension used: 'main' uses SEH and 'out_path' has destructor 2> .\bench\bench.cc(87) : see declaration of 'out_path' 2>.\bench\bench.cc(237) : warning C4509: nonstandard extension used: 'main' uses SEH and 'in_path' has destructor 2> .\bench\bench.cc(86) : see declaration of 'in_path' 2>.\bench\bench.cc(237) : warning C4509: nonstandard extension used: 'main' uses SEH and 'filenames' has destructor 2> .\bench\bench.cc(59) : see declaration of 'filenames' 2>.\bench\bench.cc(237) : warning C4509: nonstandard extension used: 'main' uses SEH and 'exit_manager' has destructor 2> .\bench\bench.cc(54) : see declaration of 'exit_manager' 2>.\bench\bench.cc(275) : warning C4509: nonstandard extension used: 'main' uses SEH and 'decode_times' has destructor 2> .\bench\bench.cc(241) : see declaration of 'decode_times' 2>.\bench\bench.cc(275) : warning C4509: nonstandard extension used: 'main' uses SEH and 'threads' has destructor 2> .\bench\bench.cc(108) : see declaration of 'threads' 2>.\bench\bench.cc(275) : warning C4509: nonstandard extension used: 'main' uses SEH and 'stream' has destructor 2> .\bench\bench.cc(95) : see declaration of 'stream' 2>.\bench\bench.cc(275) : warning C4509: nonstandard extension used: 'main' uses SEH and 'out_path' has destructor 2> .\bench\bench.cc(87) : see declaration of 'out_path' 2>.\bench\bench.cc(275) : warning C4509: nonstandard extension used: 'main' uses SEH and 'in_path' has destructor 2> .\bench\bench.cc(86) : see declaration of 'in_path' 2>.\bench\bench.cc(275) : warning C4509: nonstandard extension used: 'main' uses SEH and 'filenames' has destructor 2> .\bench\bench.cc(59) : see declaration of 'filenames' 2>.\bench\bench.cc(275) : warning C4509: nonstandard extension used: 'main' uses SEH and 'exit_manager' has destructor 2> .\bench\bench.cc(54) : see declaration of 'exit_manager' 2>.\bench\bench.cc(314) : warning C4509: nonstandard extension used: 'main' uses SEH and 'decode_times' has destructor 2> .\bench\bench.cc(241) : see declaration of 'decode_times' 2>.\bench\bench.cc(314) : warning C4509: nonstandard extension used: 'main' uses SEH and 'threads' has destructor 2> .\bench\bench.cc(108) : see declaration of 'threads' 2>.\bench\bench.cc(314) : warning C4509: nonstandard extension used: 'main' uses SEH and 'stream' has destructor 2> .\bench\bench.cc(95) : see declaration of 'stream' 2>.\bench\bench.cc(314) : warning C4509: nonstandard extension used: 'main' uses SEH and 'out_path' has destructor 2> .\bench\bench.cc(87) : see declaration of 'out_path' 2>.\bench\bench.cc(314) : warning C4509: nonstandard extension used: 'main' uses SEH and 'in_path' has destructor 2> .\bench\bench.cc(86) : see declaration of 'in_path' 2>.\bench\bench.cc(314) : warning C4509: nonstandard extension used: 'main' uses SEH and 'filenames' has destructor 2> .\bench\bench.cc(59) : see declaration of 'filenames' 2>.\bench\bench.cc(314) : warning C4509: nonstandard extension used: 'main' uses SEH and 'exit_manager' has destructor 2> .\bench\bench.cc(54) : see declaration of 'exit_manager' 2>.\bench\bench.cc(323) : warning C4509: nonstandard extension used: 'main' uses SEH and 'decode_times' has destructor 2> .\bench\bench.cc(241) : see declaration of 'decode_times' 2>.\bench\bench.cc(323) : warning C4509: nonstandard extension used: 'main' uses SEH and 'threads' has destructor 2> .\bench\bench.cc(108) : see declaration of 'threads' 2>.\bench\bench.cc(323) : warning C4509: nonstandard extension used: 'main' uses SEH and 'stream' has destructor 2> .\bench\bench.cc(95) : see declaration of 'stream' 2>.\bench\bench.cc(323) : warning C4509: nonstandard extension used: 'main' uses SEH and 'out_path' has destructor 2> .\bench\bench.cc(87) : see declaration of 'out_path' 2>.\bench\bench.cc(323) : warning C4509: nonstandard extension used: 'main' uses SEH and 'in_path' has destructor 2> .\bench\bench.cc(86) : see declaration of 'in_path' 2>.\bench\bench.cc(323) : warning C4509: nonstandard extension used: 'main' uses SEH and 'filenames' has destructor 2> .\bench\bench.cc(59) : see declaration of 'filenames' 2>.\bench\bench.cc(323) : warning C4509: nonstandard extension used: 'main' uses SEH and 'exit_manager' has destructor 2> .\bench\bench.cc(54) : see declaration of 'exit_manager' 2>.\bench\bench.cc(347) : warning C4509: nonstandard extension used: 'main' uses SEH and 'decode_times' has destructor 2> .\bench\bench.cc(241) : see declaration of 'decode_times' 2>.\bench\bench.cc(347) : warning C4509: nonstandard extension used: 'main' uses SEH and 'threads' has destructor 2> .\bench\bench.cc(108) : see declaration of 'threads' 2>.\bench\bench.cc(347) : warning C4509: nonstandard extension used: 'main' uses SEH and 'stream' has destructor 2> .\bench\bench.cc(95) : see declaration of 'stream' 2>.\bench\bench.cc(347) : warning C4509: nonstandard extension used: 'main' uses SEH and 'out_path' has destructor 2> .\bench\bench.cc(87) : see declaration of 'out_path' 2>.\bench\bench.cc(347) : warning C4509: nonstandard extension used: 'main' uses SEH and 'in_path' has destructor 2> .\bench\bench.cc(86) : see declaration of 'in_path' 2>.\bench\bench.cc(347) : warning C4509: nonstandard extension used: 'main' uses SEH and 'filenames' has destructor 2> .\bench\bench.cc(59) : see declaration of 'filenames' 2>.\bench\bench.cc(347) : warning C4509: nonstandard extension used: 'main' uses SEH and 'exit_manager' has destructor 2> .\bench\bench.cc(54) : see declaration of 'exit_manager' 2>.\bench\bench.cc(401) : warning C4509: nonstandard extension used: 'main' uses SEH and 'decode_times' has destructor 2> .\bench\bench.cc(241) : see declaration of 'decode_times' 2>.\bench\bench.cc(404) : warning C4509: nonstandard extension used: 'main' uses SEH and 'threads' has destructor 2> .\bench\bench.cc(108) : see declaration of 'threads' 2>.\bench\bench.cc(404) : warning C4509: nonstandard extension used: 'main' uses SEH and 'stream' has destructor 2> .\bench\bench.cc(95) : see declaration of 'stream' 2>.\bench\bench.cc(404) : warning C4509: nonstandard extension used: 'main' uses SEH and 'out_path' has destructor 2> .\bench\bench.cc(87) : see declaration of 'out_path' 2>.\bench\bench.cc(404) : warning C4509: nonstandard extension used: 'main' uses SEH and 'in_path' has destructor 2> .\bench\bench.cc(86) : see declaration of 'in_path' 2>.\bench\bench.cc(404) : warning C4509: nonstandard extension used: 'main' uses SEH and 'filenames' has destructor 2> .\bench\bench.cc(59) : see declaration of 'filenames' 2>.\bench\bench.cc(404) : warning C4509: nonstandard extension used: 'main' uses SEH and 'exit_manager' has destructor 2> .\bench\bench.cc(54) : see declaration of 'exit_manager' 2>.\bench\bench.cc(407) : warning C4509: nonstandard extension used: 'main' uses SEH and 'threads' has destructor 2> .\bench\bench.cc(108) : see declaration of 'threads' 2>.\bench\bench.cc(407) : warning C4509: nonstandard extension used: 'main' uses SEH and 'stream' has destructor 2> .\bench\bench.cc(95) : see declaration of 'stream' 2>.\bench\bench.cc(407) : warning C4509: nonstandard extension used: 'main' uses SEH and 'out_path' has destructor 2> .\bench\bench.cc(87) : see declaration of 'out_path' 2>.\bench\bench.cc(407) : warning C4509: nonstandard extension used: 'main' uses SEH and 'in_path' has destructor 2> .\bench\bench.cc(86) : see declaration of 'in_path' 2>.\bench\bench.cc(407) : warning C4509: nonstandard extension used: 'main' uses SEH and 'filenames' has destructor 2> .\bench\bench.cc(59) : see declaration of 'filenames' 2>.\bench\bench.cc(407) : warning C4509: nonstandard extension used: 'main' uses SEH and 'exit_manager' has destructor 2> .\bench\bench.cc(54) : see declaration of 'exit_manager' 2>.\bench\bench.cc(408) : warning C4509: nonstandard extension used: 'main' uses SEH and 'threads' has destructor 2> .\bench\bench.cc(108) : see declaration of 'threads' 2>.\bench\bench.cc(408) : warning C4509: nonstandard extension used: 'main' uses SEH and 'stream' has destructor 2> .\bench\bench.cc(95) : see declaration of 'stream' 2>.\bench\bench.cc(408) : warning C4509: nonstandard extension used: 'main' uses SEH and 'out_path' has destructor 2> .\bench\bench.cc(87) : see declaration of 'out_path' 2>.\bench\bench.cc(408) : warning C4509: nonstandard extension used: 'main' uses SEH and 'in_path' has destructor 2> .\bench\bench.cc(86) : see declaration of 'in_path' 2>.\bench\bench.cc(408) : warning C4509: nonstandard extension used: 'main' uses SEH and 'filenames' has destructor 2> .\bench\bench.cc(59) : see declaration of 'filenames' 2>.\bench\bench.cc(408) : warning C4509: nonstandard extension used: 'main' uses SEH and 'exit_manager' has destructor 2> .\bench\bench.cc(54) : see declaration of 'exit_manager' 2>Build log was saved at "file://C:\chrome-svn\src\media\Debug\obj\media_bench\BuildLog.htm" http://codereview.chromium.org/199049/diff/6001/7001#newcode138 Line 138: #ifdef _MSC_VER I had to try :-) http://codereview.chromium.org/199049/diff/6001/7001#newcode400 Line 400: #ifdef _MSC_VER On 2009/09/09 01:11:52, scherkus wrote: > #if defined(OS_WIN) Done.
added null codec check for pcm audio in mp4 files.
Added another error check for unknown codec, and --frames option to do quick
tests, such as a pruning script to remove all unsupported files.
C:\chrome-svn\src\media\Release>media_bench.exe --stream=video
e:\mediatests\seattle.mp4 --frames=100
* Stream #0: h264 (H.264 / AVC / MPEG-4 AVC / MPEG-4 part 10)
Stream #1: aac (Advanced Audio Coding)
Frames: 100
Total: 1904.67 ms
Summation: 1894.94 ms
Average: 18.95 ms
StdDev: 4.57 ms
Code looks good to me. Have you tried building it on linux and mac?
linux is tested.
http://codereview.chromium.org/199049/diff/2003/8006 File media/bench/bench.cc (right): http://codereview.chromium.org/199049/diff/2003/8006#newcode78 Line 78: void _disable() { The function names are a bit misleading, can you give them a better name? Other than that, the code is looking good.
_disable function name explained http://codereview.chromium.org/199049/diff/2003/8006 File media/bench/bench.cc (right): http://codereview.chromium.org/199049/diff/2003/8006#newcode78 Line 78: void _disable() { These names came from older Microsoft C libs where they did asm(di) and asm(ei) to disable and enable interupts. For windows I reimplement them with thread priority, which is more effective. On consoles (or cell phones), I tend to implement them with interupts or 'critical sections'. Its not something you'd want to do for production code, but its valuable for accurate and reproducible benchmarking. Off hand I can't think of a better name without being misleading. At least this name can be 'googled' for and find public documentation on what it does. ie http://www.users.pjwstk.edu.pl/~jms/qnx/help/watcom/clibref/src/_disable.html
http://codereview.chromium.org/199049/diff/2003/8006 File media/bench/bench.cc (right): http://codereview.chromium.org/199049/diff/2003/8006#newcode63 Line 63: #if defined(USE_PIPE) does this have to be a define? Tools such as wget/curl use a filename of "-" (single dash) to represent reading from stdin or writing to stdout.. you may want to consider that here if I'm understanding the code correctly, if USE_PIPE is defined you still have to specify a filename (that doesn't get created) to trigger writing to stdout -- so might as well use a token filename Also instead of macros you can declare an std::ostream reference. For example: std::ostream* log_out = NULL; if (write_to_stdout) { log_out = &std::cerr; } else { log_out = &std::cout; } *log_out << "Foo" << std::endl; http://codereview.chromium.org/199049/diff/2003/8006#newcode120 Line 120: << " --hash " shouldn't this be called --djb2 instead? http://codereview.chromium.org/199049/diff/2003/8006#newcode185 Line 185: bool hash = false; if you're having multiple hashes, go hash_djb2 and djb2_value http://codereview.chromium.org/199049/diff/2003/8006#newcode509 Line 509: bench_cout << " Hash:" << std::setw(11) << hash_value nit: Hash -> DJB2
from my interpretation of the benchmarks, if we don't want to use MD5 hashing because it's super slow, then you can decide whether or not you want to actually keep it in media_bench
fixed output streams http://codereview.chromium.org/199049/diff/2003/8006 File media/bench/bench.cc (right): http://codereview.chromium.org/199049/diff/2003/8006#newcode63 Line 63: #if defined(USE_PIPE) Done. Thanks! I was trying to figure out how to point to cout vs cerr. The ifdef isnt necessary... I like to do it on code that I'm not sure we should keep long term... makes it easy to spot. Or if its problematic on some new platform, it can be disabled. http://codereview.chromium.org/199049/diff/2003/8006#newcode120 Line 120: << " --hash " Done. http://codereview.chromium.org/199049/diff/2003/8006#newcode185 Line 185: bool hash = false; On 2009/09/11 21:57:25, scherkus wrote: > if you're having multiple hashes, go hash_djb2 and djb2_value Done. http://codereview.chromium.org/199049/diff/2003/8006#newcode509 Line 509: bench_cout << " Hash:" << std::setw(11) << hash_value On 2009/09/11 21:57:25, scherkus wrote: > nit: Hash -> DJB2 Done.
may as well keep md5. it saves disc space and is a more robust checksum, for cases where performance isnt the primary concern.
http://codereview.chromium.org/199049/diff/7005/7006 File media/bench/bench.cc (right): http://codereview.chromium.org/199049/diff/7005/7006#newcode11 Line 11: #define USE_PIPE 1 we can always enable writing to stdout mode http://codereview.chromium.org/199049/diff/7005/7006#newcode64 Line 64: #define EXCEPTION_HANDLER 1 might as well always have this enabled for windows http://codereview.chromium.org/199049/diff/7005/7006#newcode71 Line 71: void _disable() { I'm with alpha on this one... these function names are not very clear (especially considering you call disable() before ever calling enable()) http://codereview.chromium.org/199049/diff/7005/7006#newcode265 Line 265: << codec->long_name << ")" << std::endl; alignment http://codereview.chromium.org/199049/diff/7005/7006#newcode479 Line 479: << std::endl; alignment http://codereview.chromium.org/199049/diff/7005/7006#newcode481 Line 481: << " ms" << std::endl; alignment http://codereview.chromium.org/199049/diff/7005/7006#newcode483 Line 483: << " ms" << std::endl; alignment http://codereview.chromium.org/199049/diff/7005/7006#newcode502 Line 502: << " ms" << std::endl; alignment http://codereview.chromium.org/199049/diff/7005/7006#newcode504 Line 504: << " ms" << std::endl; alignment http://codereview.chromium.org/199049/diff/7005/7006#newcode508 Line 508: << " " << in_path << std::endl; alignment http://codereview.chromium.org/199049/diff/7005/7006#newcode514 Line 514: << " " << in_path << std::endl; alignment http://codereview.chromium.org/199049/diff/7005/7006#newcode519 Line 519: << " " << in_path << std::endl; alignment
done
LG yet?
more style nits http://codereview.chromium.org/199049/diff/6008/7009 File media/bench/bench.cc (right): http://codereview.chromium.org/199049/diff/6008/7009#newcode13 Line 13: #include "build/build_config.h" nit on header include order (based off other examples in source): put build_config first then blank line then platform-specific includes then blank line then the rest of the system includes then blank line then the rest http://codereview.chromium.org/199049/diff/6008/7009#newcode57 Line 57: // Remove the following macro if you want to debug into exceptions. remove this comment http://codereview.chromium.org/199049/diff/6008/7009#newcode64 Line 64: void EnterCriticalSection() { warning: this is a Win32 function name http://codereview.chromium.org/199049/diff/6008/7009#newcode65 Line 65: SetThreadPriority(GetCurrentThread(),THREAD_PRIORITY_ABOVE_NORMAL); space after , http://codereview.chromium.org/199049/diff/6008/7009#newcode69 Line 69: SetThreadPriority(GetCurrentThread(),THREAD_PRIORITY_NORMAL); space after , http://codereview.chromium.org/199049/diff/6008/7010 File media/bench/file_protocol.cc (right): http://codereview.chromium.org/199049/diff/6008/7010#newcode8 Line 8: #include <fcntl.h> nit on header include order (based off other examples in source): keep file_protocol.h first then blank line then build_config then blank line then platform-specific includes then blank line then the rest of the system includes then blank line then the rest http://codereview.chromium.org/199049/diff/6008/7010#newcode24 Line 24: nit: extra blank line http://codereview.chromium.org/199049/diff/6008/7010#newcode43 Line 43: h->priv_data = reinterpret_cast<void *>(static_cast<intptr_t>(f)); nit: void * -> void* http://codereview.chromium.org/199049/diff/6008/7010#newcode58 Line 58: return lseek(GetHandle(h), static_cast<long>(offset), whence); indentation http://codereview.chromium.org/199049/diff/6008/7010#newcode60 Line 60: return lseek(GetHandle(h), offset, whence); indentation http://codereview.chromium.org/199049/diff/6008/7010#newcode84 Line 84: delete trailing newlines
Done http://codereview.chromium.org/199049/diff/6008/7009 File media/bench/bench.cc (right): http://codereview.chromium.org/199049/diff/6008/7009#newcode13 Line 13: #include "build/build_config.h" On 2009/09/12 02:06:41, scherkus wrote: > nit on header include order (based off other examples in source): > > put build_config first > then blank line > then platform-specific includes > then blank line > then the rest of the system includes > then blank line > then the rest Done. http://codereview.chromium.org/199049/diff/6008/7009#newcode57 Line 57: // Remove the following macro if you want to debug into exceptions. On 2009/09/12 02:06:41, scherkus wrote: > remove this comment Done. http://codereview.chromium.org/199049/diff/6008/7009#newcode64 Line 64: void EnterCriticalSection() { So was _disable okay, I'll call it a 'Timing' section http://codereview.chromium.org/199049/diff/6008/7009#newcode65 Line 65: SetThreadPriority(GetCurrentThread(),THREAD_PRIORITY_ABOVE_NORMAL); On 2009/09/12 02:06:41, scherkus wrote: > space after , Done. http://codereview.chromium.org/199049/diff/6008/7009#newcode69 Line 69: SetThreadPriority(GetCurrentThread(),THREAD_PRIORITY_NORMAL); On 2009/09/12 02:06:41, scherkus wrote: > space after , Done. http://codereview.chromium.org/199049/diff/6008/7010 File media/bench/file_protocol.cc (right): http://codereview.chromium.org/199049/diff/6008/7010#newcode8 Line 8: #include <fcntl.h> On 2009/09/12 02:06:41, scherkus wrote: > nit on header include order (based off other examples in source): > > keep file_protocol.h first > then blank line > then build_config > then blank line > then platform-specific includes > then blank line > then the rest of the system includes > then blank line > then the rest Done. http://codereview.chromium.org/199049/diff/6008/7010#newcode24 Line 24: On 2009/09/12 02:06:41, scherkus wrote: > nit: extra blank line Done. http://codereview.chromium.org/199049/diff/6008/7010#newcode43 Line 43: h->priv_data = reinterpret_cast<void *>(static_cast<intptr_t>(f)); On 2009/09/12 02:06:41, scherkus wrote: > nit: void * -> void* Done. http://codereview.chromium.org/199049/diff/6008/7010#newcode58 Line 58: return lseek(GetHandle(h), static_cast<long>(offset), whence); On 2009/09/12 02:06:41, scherkus wrote: > indentation Done. http://codereview.chromium.org/199049/diff/6008/7010#newcode60 Line 60: return lseek(GetHandle(h), offset, whence); On 2009/09/12 02:06:41, scherkus wrote: > indentation Done. http://codereview.chromium.org/199049/diff/6008/7010#newcode84 Line 84: On 2009/09/12 02:06:41, scherkus wrote: > delete trailing newlines Done.
LGTM sorry for being a stickler for style :) |
