Change audio renderer to communicate with host using low latency codepath.

Change audio renderer to communicate with host using low latency codepath.
I.e. now host signals that it is ready to accept new data using sockets,
not IPC messages.

That should reduce audio latency.

Right now we are using high latency code path by default, low latency
would be used only when flag --enable-low-latency-audio is specified.

Changed unit tests to test low latency code path, as it should become
default soon.

http://codereview.chromium.org/7253003

BUG=http://code.google.com/p/chromium/issues/detail?id=87640
TEST=Everything should just work. I changed audio_renderer_impl_unittest.cc to explicitly force low latency code path.

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

[scherkus (not reviewing), 2011-06-24 21:09:25]

don't forget BUG= and TEST=

if a suitable doesn't exist feel free to create a new bug in Feature-Media that
tracks switching to low-latency mode

http://codereview.chromium.org/7253003/diff/5001/content/common/content_switc...
File content/common/content_switches.cc (right):

http://codereview.chromium.org/7253003/diff/5001/content/common/content_switc...
content/common/content_switches.cc:274: // That is temporary till we make it
default and remove old code path.
s/till/until/

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:40: if
(CommandLine::ForCurrentProcess()->HasSwitch(
instead of checking the command line here, do the check in
content/renderer/render_view.cc (see RenderView::createMediaPlayer)

we can also get rid of the static var and make it member var

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:189: DCHECK(latency_type_ ==
kHighLatency);
try to use DCHECK_EQ when possible

for pointers it's not always as interesting, but for values it can be nice for
debugging

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:201: DCHECK(latency_type_ ==
kLowLatency);
DCHECK_EQ

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:211: DCHECK(latency_type_ ==
kLowLatency);
DCHECK_EQ

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:221: 
nit: remove blank line

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:223: DCHECK(latency_type_ ==
kLowLatency);
DCHECK_EQ

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:247: DCHECK(latency_type_ ==
kHighLatency);
DCHECK_EQ

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:354: DCHECK(latency_type_ ==
kHighLatency);
DCHECK_EQ

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:413:
audio_thread_->SetThreadPriority(base::kThreadPriority_RealtimeAudio);
it's ok to remove the DCHECK and have it spew a NOTIMPLEMENTED() for now (it
does a LOG(ERROR))

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:428: if (playback_rate > 0.0f) {
nit: try to collapse nested ifs, so check for opposite condition and continue

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
File content/renderer/media/audio_renderer_impl_unittest.cc (right):

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl_unittest.cc:24: virtual void
CreateSocket(base::SyncSocket::Handle socket_handle) {
nit: for empty one-liner functions we typically close the braces {} assuming the
entire thing can fit on a single line

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl_unittest.cc:123: if
(renderer_->latency_type() == AudioRendererImpl::kHighLatency) {
if every test has its latency set as kLowLatency then will this ever get
executed?

[enal1, 2011-06-24 22:01:09]

Addressed everything, plus uploaded 2 files not uploaded earlier...

http://codereview.chromium.org/7253003/diff/5001/content/common/content_switc...
File content/common/content_switches.cc (right):

http://codereview.chromium.org/7253003/diff/5001/content/common/content_switc...
content/common/content_switches.cc:274: // That is temporary till we make it
default and remove old code path.
On 2011/06/24 21:09:26, scherkus wrote:
> s/till/until/

Done.

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:40: if
(CommandLine::ForCurrentProcess()->HasSwitch(
On 2011/06/24 21:09:26, scherkus wrote:
> instead of checking the command line here, do the check in
> content/renderer/render_view.cc (see RenderView::createMediaPlayer)
> 
> we can also get rid of the static var and make it member var

(1) I thought about moving check up the call tree, and decided against it. Upper
level code should not go into details exactly which protocols audio renderer
implementation is using. Other than audio renderer the only code that should be
interested is hoisting code that implements other half of the protocol, and I am
not touching it.

(2) I thought about member variable, too, but decided to use global -- I don't
see why we want to make only some instances low latency. Thank for pointing,
though -- I made method that sets mode static, and it should be called before
any instance is created. This way tests can be sure that all instances are the
"right" ones.

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:189: DCHECK(latency_type_ ==
kHighLatency);
On 2011/06/24 21:09:26, scherkus wrote:
> try to use DCHECK_EQ when possible
> 
> for pointers it's not always as interesting, but for values it can be nice for
> debugging

Done.

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:201: DCHECK(latency_type_ ==
kLowLatency);
On 2011/06/24 21:09:26, scherkus wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:211: DCHECK(latency_type_ ==
kLowLatency);
On 2011/06/24 21:09:26, scherkus wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:221: 
On 2011/06/24 21:09:26, scherkus wrote:
> nit: remove blank line

Done.

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:223: DCHECK(latency_type_ ==
kLowLatency);
On 2011/06/24 21:09:26, scherkus wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:247: DCHECK(latency_type_ ==
kHighLatency);
On 2011/06/24 21:09:26, scherkus wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:354: DCHECK(latency_type_ ==
kHighLatency);
On 2011/06/24 21:09:26, scherkus wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:413:
audio_thread_->SetThreadPriority(base::kThreadPriority_RealtimeAudio);
On 2011/06/24 21:09:26, scherkus wrote:
> it's ok to remove the DCHECK and have it spew a NOTIMPLEMENTED() for now (it
> does a LOG(ERROR))

It is exactly what it is doing, I just did not like that extra output... Removed
#ifdef here.

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:428: if (playback_rate > 0.0f) {
On 2011/06/24 21:09:26, scherkus wrote:
> nit: try to collapse nested ifs, so check for opposite condition and continue

Done, though personally I strongly prefer nested ifs: "continue" passes control
somewhere outside normal control flow. E.g. you cannot just move piece of code
with "continue" into nested loop...

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
File content/renderer/media/audio_renderer_impl_unittest.cc (right):

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl_unittest.cc:24: virtual void
CreateSocket(base::SyncSocket::Handle socket_handle) {
On 2011/06/24 21:09:26, scherkus wrote:
> nit: for empty one-liner functions we typically close the braces {} assuming
the
> entire thing can fit on a single line

Done.

http://codereview.chromium.org/7253003/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl_unittest.cc:123: if
(renderer_->latency_type() == AudioRendererImpl::kHighLatency) {
On 2011/06/24 21:09:26, scherkus wrote:
> if every test has its latency set as kLowLatency then will this ever get
> executed?

I prefer code that can be executed in both high and low latency modes. E.g. I
run those tests in high latency mode to verify I did not break it...

[scherkus (not reviewing), 2011-06-27 18:49:38]

LGTM

don't forget BUG= and TEST= before submitting

[enal1, 2011-06-27 19:12:30]

Added BUG= and TEST= to the description.

[commit-bot: I haz the power, 2011-06-28 18:15:17]

Presubmit check for 7253003-9 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/common/content_switches.cc,content/common/content_switches.h,content/browser/renderer_host/browser_render_process_host.cc

Presubmit checks took 1.4s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[enal1, 2011-06-28 19:11:07]

Adding brettw as reviewer for content/browser and content/common review...

[brettw, 2011-07-06 22:13:23]

LGTM (I didn't check the logic). Sorry I missed this before.

http://codereview.chromium.org/7253003/diff/9/content/common/content_switches.cc
File content/common/content_switches.cc (right):

http://codereview.chromium.org/7253003/diff/9/content/common/content_switches...
content/common/content_switches.cc:275: const char kLowLatencyAudio[] =
"enable-low-latency-audio";
Please indent the = to match the rest of the file.

http://codereview.chromium.org/7253003/diff/9/content/renderer/media/audio_re...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/7253003/diff/9/content/renderer/media/audio_re...
content/renderer/media/audio_renderer_impl.cc:421: if (bytes ==
media::AudioOutputController::kPauseMark) {
Be consistent about {} for single-line conditionals, since the rest of the file
doesn't use them, you shouldn't either.

http://codereview.chromium.org/7253003/diff/9/content/renderer/media/audio_re...
File content/renderer/media/audio_renderer_impl.h (right):

http://codereview.chromium.org/7253003/diff/9/content/renderer/media/audio_re...
content/renderer/media/audio_renderer_impl.h:44: #include
"base/threading/simple_thread.h"
This should be in order (moved below 's')

[commit-bot: I haz the power, 2011-07-07 18:17:45]

Change committed as 91727