Chromecast: Play audio streams not supported by CMA via default renderer

Chromecast: Play audio streams not supported by CMA via default renderer

For Chromecast we want to choose which media renderer to use based on
the types of input content streams. We will use CMA media renderer for
media types that are supported by our hardware (H264, AAC, etc) and
will use the default media renderer for audio streams other than AAC
or Vorbis. This will allow us support software decoding of FLAC and
Opus via the default Chrome audio path.

BUG=457959
Committed: https://crrev.com/2e583073b996e66d62f709dd2ffde9e1ef58b276
Cr-Commit-Position: refs/heads/master@{#318941}

[servolk, 2015-03-02 23:22:59]

servolk@chromium.org changed reviewers:
+ dalecurtis@chromium.org, gunsch@chromium.org, lcwu@chromium.org

[gunsch, 2015-03-03 00:06:41]

+CC Eric

Pile of comments, mostly nits. I'm fine with the overall structure.

https://codereview.chromium.org/973633002/diff/20001/chromecast/media/base/me...
File chromecast/media/base/media_renderer.cc (right):

https://codereview.chromium.org/973633002/diff/20001/chromecast/media/base/me...
chromecast/media/base/media_renderer.cc:8: #include
"chromecast/media/cma/filters/cma_renderer.h"
chromecast/media/base should NOT be depending on chromecast/media/cma/filters.
Please move this file to chromecast/media/cma/filters. (Android build can link
in chromecast/media/base, but not chromecast/media/cma)

https://codereview.chromium.org/973633002/diff/20001/chromecast/media/base/me...
chromecast/media/base/media_renderer.cc:17: scoped_ptr<::media::Renderer>
default_renderer,
style nit: indentation

https://codereview.chromium.org/973633002/diff/20001/chromecast/media/base/me...
chromecast/media/base/media_renderer.cc:87: }
style nit: blank line

https://codereview.chromium.org/973633002/diff/20001/chromecast/media/base/me...
File chromecast/media/base/media_renderer.h (right):

https://codereview.chromium.org/973633002/diff/20001/chromecast/media/base/me...
chromecast/media/base/media_renderer.h:27: class MediaRenderer : public
::media::Renderer {
Can we give this a better name than just the interface it's implementing?
perhaps "SwitchingMediaRenderer" or something.

https://codereview.chromium.org/973633002/diff/20001/chromecast/media/base/me...
chromecast/media/base/media_renderer.h:31: scoped_ptr<CmaRenderer>
cma_renderer);
This API's kind of interesting... is it possible we'll ever need more renderers?
If so, what about an API that took pairs of "::media::Renderer,
base::Callback<bool(void)> use_renderer_if_this_returns_true" ?

Alternately, this could be conceptually simplified by just taking two
::media::Renderer instances. There's no reason to tie the API to CmaRenderer
specifically. If you did that, it'd probably be okay to leave this file in
chromecast/media/base/, since it no longer depends on chromecast/media/cma/.

https://codereview.chromium.org/973633002/diff/20001/chromecast/renderer/medi...
File chromecast/renderer/media/chromecast_media_renderer_factory.cc (right):

https://codereview.chromium.org/973633002/diff/20001/chromecast/renderer/medi...
chromecast/renderer/media/chromecast_media_renderer_factory.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
2015

https://codereview.chromium.org/973633002/diff/20001/chromecast/renderer/medi...
chromecast/renderer/media/chromecast_media_renderer_factory.cc:40: int
buffer_size = kDefaultSamplingRate*20*2/1000; // About 20ms of audio
style nit: spaces on sides of operators

https://codereview.chromium.org/973633002/diff/20001/chromecast/renderer/medi...
chromecast/renderer/media/chromecast_media_renderer_factory.cc:51: }
style nit: blank line after conditional block

https://codereview.chromium.org/973633002/diff/20001/chromecast/renderer/medi...
chromecast/renderer/media/chromecast_media_renderer_factory.cc:62:
scoped_ptr<::media::Renderer> default_chrome_render(
conceptual layering violation: "chrome" is not supposed to be referenced outside
of src/chrome/

[servolk, 2015-03-03 00:46:54]

https://codereview.chromium.org/973633002/diff/20001/chromecast/media/base/me...
File chromecast/media/base/media_renderer.cc (right):

https://codereview.chromium.org/973633002/diff/20001/chromecast/media/base/me...
chromecast/media/base/media_renderer.cc:8: #include
"chromecast/media/cma/filters/cma_renderer.h"
On 2015/03/03 00:06:40, gunsch wrote:
> chromecast/media/base should NOT be depending on chromecast/media/cma/filters.
> Please move this file to chromecast/media/cma/filters. (Android build can link
> in chromecast/media/base, but not chromecast/media/cma)

Luckily we don't really need CmaRenderer here, we can use media::Renderer
instead.

https://codereview.chromium.org/973633002/diff/20001/chromecast/media/base/me...
chromecast/media/base/media_renderer.cc:17: scoped_ptr<::media::Renderer>
default_renderer,
On 2015/03/03 00:06:40, gunsch wrote:
> style nit: indentation

Done.

https://codereview.chromium.org/973633002/diff/20001/chromecast/media/base/me...
chromecast/media/base/media_renderer.cc:87: }
On 2015/03/03 00:06:40, gunsch wrote:
> style nit: blank line

Done.

https://codereview.chromium.org/973633002/diff/20001/chromecast/media/base/me...
File chromecast/media/base/media_renderer.h (right):

https://codereview.chromium.org/973633002/diff/20001/chromecast/media/base/me...
chromecast/media/base/media_renderer.h:27: class MediaRenderer : public
::media::Renderer {
On 2015/03/03 00:06:40, gunsch wrote:
> Can we give this a better name than just the interface it's implementing?
> perhaps "SwitchingMediaRenderer" or something.

Done.

https://codereview.chromium.org/973633002/diff/20001/chromecast/media/base/me...
chromecast/media/base/media_renderer.h:31: scoped_ptr<CmaRenderer>
cma_renderer);
On 2015/03/03 00:06:40, gunsch wrote:
> This API's kind of interesting... is it possible we'll ever need more
renderers?
> If so, what about an API that took pairs of "::media::Renderer,
> base::Callback<bool(void)> use_renderer_if_this_returns_true" ?
> 
> Alternately, this could be conceptually simplified by just taking two
> ::media::Renderer instances. There's no reason to tie the API to CmaRenderer
> specifically. If you did that, it'd probably be okay to leave this file in
> chromecast/media/base/, since it no longer depends on chromecast/media/cma/.

Yes, the only reason I used CmaRenderer here explicitly was to get some minimal
type-checking and make it impossible to give the parameters in the wrong order.
But since this unnecessary dependency on CmaRenderer creates some issues, we can
switch to generic media::Renderer.
I think we should look into refactoring the whole media renderer creation model
in WebMediaPlayer, but that would need to be done in a separate CL.

https://codereview.chromium.org/973633002/diff/20001/chromecast/renderer/medi...
File chromecast/renderer/media/chromecast_media_renderer_factory.cc (right):

https://codereview.chromium.org/973633002/diff/20001/chromecast/renderer/medi...
chromecast/renderer/media/chromecast_media_renderer_factory.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2015/03/03 00:06:40, gunsch wrote:
> 2015

Done.

https://codereview.chromium.org/973633002/diff/20001/chromecast/renderer/medi...
chromecast/renderer/media/chromecast_media_renderer_factory.cc:40: int
buffer_size = kDefaultSamplingRate*20*2/1000; // About 20ms of audio
On 2015/03/03 00:06:40, gunsch wrote:
> style nit: spaces on sides of operators

Done.

https://codereview.chromium.org/973633002/diff/20001/chromecast/renderer/medi...
chromecast/renderer/media/chromecast_media_renderer_factory.cc:62:
scoped_ptr<::media::Renderer> default_chrome_render(
On 2015/03/03 00:06:41, gunsch wrote:
> conceptual layering violation: "chrome" is not supposed to be referenced
outside
> of src/chrome/

Wait, why is there a layering violation? "chrome" is only in the variable name.
I meant "chrome" as opposed to "chromecast", we could rename it to
default_media_renderer.

[gunsch, 2015-03-03 01:00:51]

couple more nits, otherwise lgtm on chromecast/ side

https://codereview.chromium.org/973633002/diff/20001/chromecast/renderer/medi...
File chromecast/renderer/media/chromecast_media_renderer_factory.cc (right):

https://codereview.chromium.org/973633002/diff/20001/chromecast/renderer/medi...
chromecast/renderer/media/chromecast_media_renderer_factory.cc:62:
scoped_ptr<::media::Renderer> default_chrome_render(
Hmm, I thought I saw somewhere that you weren't supposed to mention "chrome"
outside of the src/chrome/ directory, but I can't find it now. Carry on!

https://codereview.chromium.org/973633002/diff/40001/chromecast/renderer/medi...
File chromecast/renderer/media/chromecast_media_renderer_factory.cc (right):

https://codereview.chromium.org/973633002/diff/40001/chromecast/renderer/medi...
chromecast/renderer/media/chromecast_media_renderer_factory.cc:51: media_log_,
/*gpu_factories*/ NULL, audio_config));
nit: NULL -> nullptr

https://codereview.chromium.org/973633002/diff/40001/chromecast/renderer/medi...
File chromecast/renderer/media/chromecast_media_renderer_factory.h (right):

https://codereview.chromium.org/973633002/diff/40001/chromecast/renderer/medi...
chromecast/renderer/media/chromecast_media_renderer_factory.h:1: // Copyright
2014 The Chromium Authors. All rights reserved.
2015

https://codereview.chromium.org/973633002/diff/40001/chromecast/renderer/medi...
chromecast/renderer/media/chromecast_media_renderer_factory.h:28: scoped_ptr<
::media::Renderer> CreateRenderer(
nit: "< ::" --> "<::"

https://codereview.chromium.org/973633002/diff/40001/chromecast/renderer/medi...
chromecast/renderer/media/chromecast_media_renderer_factory.h:36:
DISALLOW_COPY_AND_ASSIGN(ChromecastMediaRendererFactory);
nit: blank line above

https://codereview.chromium.org/973633002/diff/40001/media/renderers/default_...
File media/renderers/default_renderer_factory.h (right):

https://codereview.chromium.org/973633002/diff/40001/media/renderers/default_...
media/renderers/default_renderer_factory.h:11: #include
"media/filters/gpu_video_accelerator_factories.h"
I agree with Dale's comment on the separate email thread: this should probably
be moved to media/base/

[servolk, 2015-03-03 01:06:10]

https://codereview.chromium.org/973633002/diff/20001/chromecast/renderer/medi...
File chromecast/renderer/media/chromecast_media_renderer_factory.cc (right):

https://codereview.chromium.org/973633002/diff/20001/chromecast/renderer/medi...
chromecast/renderer/media/chromecast_media_renderer_factory.cc:51: }
On 2015/03/03 00:06:40, gunsch wrote:
> style nit: blank line after conditional block

Done.

https://codereview.chromium.org/973633002/diff/40001/chromecast/renderer/medi...
File chromecast/renderer/media/chromecast_media_renderer_factory.cc (right):

https://codereview.chromium.org/973633002/diff/40001/chromecast/renderer/medi...
chromecast/renderer/media/chromecast_media_renderer_factory.cc:51: media_log_,
/*gpu_factories*/ NULL, audio_config));
On 2015/03/03 01:00:51, gunsch wrote:
> nit: NULL -> nullptr

Done.

https://codereview.chromium.org/973633002/diff/40001/chromecast/renderer/medi...
File chromecast/renderer/media/chromecast_media_renderer_factory.h (right):

https://codereview.chromium.org/973633002/diff/40001/chromecast/renderer/medi...
chromecast/renderer/media/chromecast_media_renderer_factory.h:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2015/03/03 01:00:51, gunsch wrote:
> 2015

Done.

https://codereview.chromium.org/973633002/diff/40001/chromecast/renderer/medi...
chromecast/renderer/media/chromecast_media_renderer_factory.h:28: scoped_ptr<
::media::Renderer> CreateRenderer(
On 2015/03/03 01:00:51, gunsch wrote:
> nit: "< ::" --> "<::"

Done.

https://codereview.chromium.org/973633002/diff/40001/chromecast/renderer/medi...
chromecast/renderer/media/chromecast_media_renderer_factory.h:36:
DISALLOW_COPY_AND_ASSIGN(ChromecastMediaRendererFactory);
On 2015/03/03 01:00:51, gunsch wrote:
> nit: blank line above

Done.

https://codereview.chromium.org/973633002/diff/40001/media/renderers/default_...
File media/renderers/default_renderer_factory.h (right):

https://codereview.chromium.org/973633002/diff/40001/media/renderers/default_...
media/renderers/default_renderer_factory.h:11: #include
"media/filters/gpu_video_accelerator_factories.h"
On 2015/03/03 01:00:51, gunsch wrote:
> I agree with Dale's comment on the separate email thread: this should probably
> be moved to media/base/ 

Ok, I'll do that as a separate CL

[servolk, 2015-03-03 20:03:12]

The CQ bit was checked by servolk@chromium.org

[servolk, 2015-03-03 20:03:12]

The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/973633002/#ps70001 (title:
"Reverted default_renderer_factory.h change")

[commit-bot: I haz the power, 2015-03-03 20:04:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/973633002/70001

[commit-bot: I haz the power, 2015-03-03 20:31:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-03 20:31:03]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[DaleCurtis, 2015-03-03 20:50:48]

deps lgtm

[servolk, 2015-03-03 21:12:26]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2015-03-03 21:13:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/973633002/70001

[commit-bot: I haz the power, 2015-03-03 21:17:37]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2015-03-03 21:18:34]

Patchset 5 (id:??) landed as
https://crrev.com/2e583073b996e66d62f709dd2ffde9e1ef58b276
Cr-Commit-Position: refs/heads/master@{#318941}

[Finnur, 2015-03-04 08:42:06]

A revert of this CL (patchset #5 id:70001) has been created in
https://codereview.chromium.org/981473003/ by finnur@chromium.org.

The reason for reverting is: Audio tests started failing consistently in the
next build after this was checked in. See:
https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests/builds/3762

Not sure if this CL is the culprit, but seems the most likely one, so attempting
a revert to see if it fixes the problem.

Error: 
[ RUN      ] AudioRecorderTest.BasicRecordAndStop
../../components/audio_modem/audio_recorder_unittest.cc:203: Failure
Value of: IsRecording()
Actual: false
Expected: true
../../components/audio_modem/audio_recorder_unittest.cc:208: Failure
Value of: IsRecording()
Actual: false
Expected: true
../../components/audio_modem/audio_recorder_unittest.cc:213: Failure
Value of: IsRecording()
Actual: false
Expected: true
[  FAILED  ] AudioRecorderTest.BasicRecordAndStop (1292 ms)
.