Enable three threads for video decoding.

Change default to three threads for video decoding and add command line --video-threads.
BUG=44128
TEST=player_wtl.exe --video-threads=6 d:\mediatests\crowd\crowd2.mp4


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

[fbarchard, 2010-05-16 15:22:21]

Decoding performance improvement using 3 thread decoder

[scherkus (not reviewing), 2010-05-18 06:19:51]

http://codereview.chromium.org/2080008/diff/32001/33001
File media/base/media_switches.cc (right):

http://codereview.chromium.org/2080008/diff/32001/33001#newcode1
media/base/media_switches.cc:1: // Copyright (c) 2009 The Chromium Authors. All
rights reserved.
copyright year

http://codereview.chromium.org/2080008/diff/32001/33001#newcode14
media/base/media_switches.cc:14: const char kEnableOpenMax[] = "enable-openmax";
while you're editing this file could you add a tiny comment to this?

http://codereview.chromium.org/2080008/diff/32001/33001#newcode17
media/base/media_switches.cc:17: const char kVideoThreads[] = "video-threads";
You'll need to pass this from the browser process to the renderer process.  Take
a look at:
/chrome/browser/renderer_host/browser_render_process_host.cc

http://codereview.chromium.org/2080008/diff/32001/33002
File media/base/media_switches.h (right):

http://codereview.chromium.org/2080008/diff/32001/33002#newcode1
media/base/media_switches.h:1: // Copyright (c) 2009 The Chromium Authors. All
rights reserved.
copyright year

http://codereview.chromium.org/2080008/diff/32001/33003
File media/filters/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/2080008/diff/32001/33003#newcode9
media/filters/ffmpeg_video_decode_engine.cc:9: #include "media/base/buffers.h"
alphabetize includes

http://codereview.chromium.org/2080008/diff/32001/33003#newcode47
media/filters/ffmpeg_video_decode_engine.cc:47: !StringToInt(threads,
&video_threads)) {
can we detangle this logic a bit?  we've got three separate conditionals and a
bunch of similarly named variables

how about we move the command line stuff into a static helper function "bool
GetVideoThreadsFromCommandLine(int* threads_out)" + move the sanitization there
(where we check for the value to be between 0 and 16)

then we can simply override decode_threads below if
GetVideoThreadsFromCommandLine returns true

[fbarchard, 2010-05-18 07:02:06]

fixed andrews concerns

http://codereview.chromium.org/2080008/diff/32001/33001
File media/base/media_switches.cc (right):

http://codereview.chromium.org/2080008/diff/32001/33001#newcode1
media/base/media_switches.cc:1: // Copyright (c) 2009 The Chromium Authors. All
rights reserved.
On 2010/05/18 06:19:51, scherkus wrote:
> copyright year

Done.

http://codereview.chromium.org/2080008/diff/32001/33001#newcode14
media/base/media_switches.cc:14: const char kEnableOpenMax[] = "enable-openmax";
On 2010/05/18 06:19:51, scherkus wrote:
> while you're editing this file could you add a tiny comment to this?

Done.

http://codereview.chromium.org/2080008/diff/32001/33001#newcode17
media/base/media_switches.cc:17: const char kVideoThreads[] = "video-threads";
added as todo, as this was enough for experimental player_wtl.

http://codereview.chromium.org/2080008/diff/32001/33003
File media/filters/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/2080008/diff/32001/33003#newcode9
media/filters/ffmpeg_video_decode_engine.cc:9: #include "media/base/buffers.h"
On 2010/05/18 06:19:51, scherkus wrote:
> alphabetize includes

Done.

http://codereview.chromium.org/2080008/diff/32001/33003#newcode47
media/filters/ffmpeg_video_decode_engine.cc:47: !StringToInt(threads,
&video_threads)) {
okay, kept constant and decode threads

[scherkus (not reviewing), 2010-05-18 07:11:41]

LGTM w/ one optional comment

http://codereview.chromium.org/2080008/diff/37001/38001
File media/base/media_switches.cc (right):

http://codereview.chromium.org/2080008/diff/37001/38001#newcode19
media/base/media_switches.cc:19: // TODO(fbarchard): pass from browser process.
I'd really recommend also adding it to the file I mentioned, just search for
kEnableOpenMax (one line change)

your call

[fbarchard, 2010-05-19 00:26:47]

added --video-threads to list propagated from browser to renderer

[scherkus (not reviewing), 2010-05-19 00:33:22]

http://codereview.chromium.org/2080008/diff/51001/6
File media/base/media_switches.cc (right):

http://codereview.chromium.org/2080008/diff/51001/6#newcode19
media/base/media_switches.cc:19: // TODO(fbarchard): pass from browser process.
remove this TODO

http://codereview.chromium.org/2080008/diff/51001/7
File media/base/media_switches.h (right):

http://codereview.chromium.org/2080008/diff/51001/7#newcode7
media/base/media_switches.h:7: #ifndef MEDIA_BASE_MEDIA_SWITCHES_H_
nice catch :)

http://codereview.chromium.org/2080008/diff/51001/8
File media/filters/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/2080008/diff/51001/8#newcode63
media/filters/ffmpeg_video_decode_engine.cc:63: if (decode_threads < 0 ||
decode_threads > kMaxDecodeThreads) {
just lump this condition into the previous one

short circuit eval will help us out here

[fbarchard, 2010-05-19 06:33:23]

http://codereview.chromium.org/2080008/diff/51001/6
File media/base/media_switches.cc (right):

http://codereview.chromium.org/2080008/diff/51001/6#newcode19
media/base/media_switches.cc:19: // TODO(fbarchard): pass from browser process.
On 2010/05/19 00:33:22, scherkus wrote:
> remove this TODO

Done.

http://codereview.chromium.org/2080008/diff/51001/7
File media/base/media_switches.h (right):

http://codereview.chromium.org/2080008/diff/51001/7#newcode7
media/base/media_switches.h:7: #ifndef MEDIA_BASE_MEDIA_SWITCHES_H_
On 2010/05/19 00:33:22, scherkus wrote:
> nice catch :)

lint is your friend.

http://codereview.chromium.org/2080008/diff/51001/8
File media/filters/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/2080008/diff/51001/8#newcode63
media/filters/ffmpeg_video_decode_engine.cc:63: if (decode_threads < 0 ||
decode_threads > kMaxDecodeThreads) {
On 2010/05/19 00:33:22, scherkus wrote:
> just lump this condition into the previous one
> 
> short circuit eval will help us out here

Done.

[scherkus (not reviewing), 2010-05-19 15:04:37]

LGTM