[chromecast] Moves CastAudioOutputStream::Backend to CMA thread.

[chromecast] Moves CastAudioOutputStream::Backend to CMA thread.

crrev.com/2cbd4fcab300af5aa4ef76752673901d58da0f24 moved
CastAudioOutputStream along with CastAudioOutputStream::Backend to
AudioThread, which is wrong. The backend need to run on CMA thread.

In addition to fixing the threading model, this patch:
- Lets AudioManager use MediaPipelineBackendFactory instead of BackendManager
- Updates tests to use the threading model used in production
- Adds another basic CastAudioManager test that creates AudioOutputStreamProxy, which is used in production.

Review-Url: https://codereview.chromium.org/2879703003
Cr-Commit-Position: refs/heads/master@{#475726}
Committed: https://chromium.googlesource.com/chromium/src/+/2341e24bf7f4b05f38a2c59354163984d076e25b

[alokp, 2017-05-11 22:22:26]

Description was changed from

==========
[chromecast] Adds a basic unittest for CastAudioManager.

The test uses CastAudioManager in the same way it is used in production.
It uses real audio and media thread and ensures that various calls are
made on the correct thread.

In order to write a test, we had to plumb MediaPipelineBackendFactory
instead of MediaPipelineBackendManager.

Ensures that audio tasks cannot run after AudioManager is deleted.
==========

to

==========
[chromecast] Adds a basic unittest for CastAudioManager.

The test uses CastAudioManager in the same way it is used in production.
It uses real audio and media thread and ensures that various calls are
made on the correct thread.

In order to write a test, we had to plumb MediaPipelineBackendFactory
instead of MediaPipelineBackendManager.

Ensures that audio tasks cannot run after AudioManager is deleted.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[alokp, 2017-05-12 17:11:56]

Description was changed from

==========
[chromecast] Adds a basic unittest for CastAudioManager.

The test uses CastAudioManager in the same way it is used in production.
It uses real audio and media thread and ensures that various calls are
made on the correct thread.

In order to write a test, we had to plumb MediaPipelineBackendFactory
instead of MediaPipelineBackendManager.

Ensures that audio tasks cannot run after AudioManager is deleted.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
[chromecast] Adds a basic unittest for CastAudioManager.

The test uses CastAudioManager in the same way it is used in production.
It uses real audio and media thread and ensures that various calls are
made on the correct thread.

In order to write a test, we had to plumb MediaPipelineBackendFactory
instead of MediaPipelineBackendManager.
==========

[alokp, 2017-05-19 23:35:03]

The CQ bit was checked by alokp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 23:35:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 23:46:47]

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

[commit-bot: I haz the power, 2017-05-19 23:46:48]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[alokp, 2017-05-25 18:22:06]

Description was changed from

==========
[chromecast] Adds a basic unittest for CastAudioManager.

The test uses CastAudioManager in the same way it is used in production.
It uses real audio and media thread and ensures that various calls are
made on the correct thread.

In order to write a test, we had to plumb MediaPipelineBackendFactory
instead of MediaPipelineBackendManager.
==========

to

==========
[chromecast] Adds a basic unittest for CastAudioManager.

The test uses CastAudioManager in the same way it is used in production.
It uses real audio and media thread and ensures that various calls are
made on the correct thread.

In order to write a test, we had to plumb MediaPipelineBackendFactory
instead of MediaPipelineBackendManager.

BUG=
==========

[alokp, 2017-05-25 18:25:41]

Description was changed from

==========
[chromecast] Adds a basic unittest for CastAudioManager.

The test uses CastAudioManager in the same way it is used in production.
It uses real audio and media thread and ensures that various calls are
made on the correct thread.

In order to write a test, we had to plumb MediaPipelineBackendFactory
instead of MediaPipelineBackendManager.

BUG=
==========

to

==========
[chromecast] Moves CastAudioOutputStream::Backend to CMA thread.

crrev.com/2cbd4fcab300af5aa4ef76752673901d58da0f24 moved CastAudioOutputStream
along with CastAudioOutputStream::Backend to AudioThread, which is wrong. The
backend need to run on CMA thread.

In addition to fixing the threading model, this patch:
- Lets AudioManager use MediaPipelineBackendFactory instead of BackendManager
- Updates tests to use the threading model used in production
- Adds another basic CastAudioManager test that creates AudioOutputStreamProxy,
which is used in production.
==========

[alokp, 2017-05-25 18:26:57]

Description was changed from

==========
[chromecast] Moves CastAudioOutputStream::Backend to CMA thread.

crrev.com/2cbd4fcab300af5aa4ef76752673901d58da0f24 moved CastAudioOutputStream
along with CastAudioOutputStream::Backend to AudioThread, which is wrong. The
backend need to run on CMA thread.

In addition to fixing the threading model, this patch:
- Lets AudioManager use MediaPipelineBackendFactory instead of BackendManager
- Updates tests to use the threading model used in production
- Adds another basic CastAudioManager test that creates AudioOutputStreamProxy,
which is used in production.
==========

to

==========
[chromecast] Moves CastAudioOutputStream::Backend to CMA thread.

crrev.com/2cbd4fcab300af5aa4ef76752673901d58da0f24 moved
CastAudioOutputStream along with CastAudioOutputStream::Backend to
AudioThread, which is wrong. The backend need to run on CMA thread.

In addition to fixing the threading model, this patch:
- Lets AudioManager use MediaPipelineBackendFactory instead of BackendManager
- Updates tests to use the threading model used in production
- Adds another basic CastAudioManager test that creates AudioOutputStreamProxy,
which is used in production.
==========

[alokp, 2017-05-25 18:28:13]

The CQ bit was checked by alokp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 18:28:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 18:39:44]

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

[commit-bot: I haz the power, 2017-05-25 18:39:45]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[alokp, 2017-05-25 23:26:22]

The CQ bit was checked by alokp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 23:27:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alokp, 2017-05-25 23:50:08]

alokp@chromium.org changed reviewers:
+ slan@chromium.org

[commit-bot: I haz the power, 2017-05-26 04:02:12]

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

[commit-bot: I haz the power, 2017-05-26 04:02:13]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[slan, 2017-05-30 16:26:02]

Pretty much just nits. Need to step back and take a look at the threading model
one more time, but lgtm

https://codereview.chromium.org/2879703003/diff/160001/chromecast/browser/cas...
File chromecast/browser/cast_content_browser_client.cc (right):

https://codereview.chromium.org/2879703003/diff/160001/chromecast/browser/cas...
chromecast/browser/cast_content_browser_client.cc:200: // TODO(alokp): Consider
switching off the mixer on audio platforms
Is this comment useful? Aren't we blocked on dmix issues?

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/audio...
File chromecast/media/audio/cast_audio_mixer.h (right):

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_mixer.h:28: CastAudioMixer(CastAudioManager*
audio_manager);
nit: explicit

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_mixer.h:51: CastAudioManager* audio_manager_;
nit: const

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/audio...
File chromecast/media/audio/cast_audio_output_stream.cc (right):

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream.cc:30: void
SignalWaitableEvent(bool* success,
nit: DCHECKs on success and waitable_event

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream.cc:258: VLOG(1) << this << ": "
<< __func__;
rm

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/cma/b...
File chromecast/media/cma/backend/media_pipeline_backend_factory.h (right):

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/cma/b...
chromecast/media/cma/backend/media_pipeline_backend_factory.h:25: }  // media
nit: namespace here and below

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/cma/t...
File chromecast/media/cma/test/mock_media_pipeline_backend_factory.h (right):

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/cma/t...
chromecast/media/cma/test/mock_media_pipeline_backend_factory.h:9: #include
"chromecast/public/media/media_pipeline_backend.h"
needed?

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/cma/t...
chromecast/media/cma/test/mock_media_pipeline_backend_factory.h:10: #include
"chromecast/public/media/media_pipeline_device_params.h"
needed?

[alokp, 2017-05-30 23:19:34]

https://codereview.chromium.org/2879703003/diff/160001/chromecast/browser/cas...
File chromecast/browser/cast_content_browser_client.cc (right):

https://codereview.chromium.org/2879703003/diff/160001/chromecast/browser/cas...
chromecast/browser/cast_content_browser_client.cc:200: // TODO(alokp): Consider
switching off the mixer on audio platforms
On 2017/05/30 16:26:01, slan wrote:
> Is this comment useful? Aren't we blocked on dmix issues?

Not really - We can switch off the CastAudioManager mixer today on audio
platforms because mixer is implemented in CMA-Alsa wrapper, which sits upstream
of alsa/dmix.

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/audio...
File chromecast/media/audio/cast_audio_mixer.h (right):

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_mixer.h:28: CastAudioMixer(CastAudioManager*
audio_manager);
On 2017/05/30 16:26:01, slan wrote:
> nit: explicit

Done.

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_mixer.h:51: CastAudioManager* audio_manager_;
On 2017/05/30 16:26:01, slan wrote:
> nit: const

Done.

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/audio...
File chromecast/media/audio/cast_audio_output_stream.cc (right):

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream.cc:30: void
SignalWaitableEvent(bool* success,
On 2017/05/30 16:26:01, slan wrote:
> nit: DCHECKs on success and waitable_event

Is it worth DCHECK'ing every pointer, especially the ones defined internally? I
find DCHECK'ing public method arguments for documentation purposes. But I think
using it everywhere just adds noise.

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream.cc:258: VLOG(1) << this << ": "
<< __func__;
On 2017/05/30 16:26:01, slan wrote:
> rm

Actually I want to log this event.

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/cma/b...
File chromecast/media/cma/backend/media_pipeline_backend_factory.h (right):

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/cma/b...
chromecast/media/cma/backend/media_pipeline_backend_factory.h:25: }  // media
On 2017/05/30 16:26:01, slan wrote:
> nit: namespace here and below

Done.

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/cma/t...
File chromecast/media/cma/test/mock_media_pipeline_backend_factory.h (right):

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/cma/t...
chromecast/media/cma/test/mock_media_pipeline_backend_factory.h:9: #include
"chromecast/public/media/media_pipeline_backend.h"
On 2017/05/30 16:26:01, slan wrote:
> needed?

yes because otherwise the compiler will complain everytime this file is
included. The clients will need to include these files in order to use this
file.

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/cma/t...
chromecast/media/cma/test/mock_media_pipeline_backend_factory.h:10: #include
"chromecast/public/media/media_pipeline_device_params.h"
On 2017/05/30 16:26:01, slan wrote:
> needed?

ditto

[alokp, 2017-05-30 23:19:52]

The CQ bit was checked by alokp@chromium.org

[alokp, 2017-05-30 23:19:52]

The patchset sent to the CQ was uploaded after l-g-t-m from slan@chromium.org
Link to the patchset: https://codereview.chromium.org/2879703003/#ps180001
(title: "addressed comments")

[commit-bot: I haz the power, 2017-05-30 23:20:41]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[slan, 2017-05-30 23:44:39]

still lgtm

https://codereview.chromium.org/2879703003/diff/160001/chromecast/browser/cas...
File chromecast/browser/cast_content_browser_client.cc (right):

https://codereview.chromium.org/2879703003/diff/160001/chromecast/browser/cas...
chromecast/browser/cast_content_browser_client.cc:200: // TODO(alokp): Consider
switching off the mixer on audio platforms
On 2017/05/30 23:19:33, alokp wrote:
> On 2017/05/30 16:26:01, slan wrote:
> > Is this comment useful? Aren't we blocked on dmix issues?
> 
> Not really - We can switch off the CastAudioManager mixer today on audio
> platforms because mixer is implemented in CMA-Alsa wrapper, which sits
upstream
> of alsa/dmix.

Ah, I see what you mean - disable the mixer whenever the alsa CMA backend is
used. Vendors (who may or may not use the alsa CMA backend) should still get
this functionality.

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/audio...
File chromecast/media/audio/cast_audio_output_stream.cc (right):

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream.cc:30: void
SignalWaitableEvent(bool* success,
On 2017/05/30 23:19:33, alokp wrote:
> On 2017/05/30 16:26:01, slan wrote:
> > nit: DCHECKs on success and waitable_event
> 
> Is it worth DCHECK'ing every pointer, especially the ones defined internally?
I
> find DCHECK'ing public method arguments for documentation purposes. But I
think
> using it everywhere just adds noise.

SGTM

https://codereview.chromium.org/2879703003/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream.cc:258: VLOG(1) << this << ": "
<< __func__;
On 2017/05/30 23:19:33, alokp wrote:
> On 2017/05/30 16:26:01, slan wrote:
> > rm
> 
> Actually I want to log this event.

Acknowledged.

[commit-bot: I haz the power, 2017-05-31 01:39:27]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1496186392198660,
"parent_rev": "6b0a494122ebe2fc48099a9549b0acbf0b28571d", "commit_rev":
"580bb06c25290d8f9717812749d5a4cadb61460b"}

[commit-bot: I haz the power, 2017-05-31 01:39:52]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1496186392198660,
"parent_rev": "0b923fff5b586cbbc9665526340bf4305634ee1a", "commit_rev":
"2341e24bf7f4b05f38a2c59354163984d076e25b"}

[commit-bot: I haz the power, 2017-05-31 01:40:02]

Description was changed from

==========
[chromecast] Moves CastAudioOutputStream::Backend to CMA thread.

crrev.com/2cbd4fcab300af5aa4ef76752673901d58da0f24 moved
CastAudioOutputStream along with CastAudioOutputStream::Backend to
AudioThread, which is wrong. The backend need to run on CMA thread.

In addition to fixing the threading model, this patch:
- Lets AudioManager use MediaPipelineBackendFactory instead of BackendManager
- Updates tests to use the threading model used in production
- Adds another basic CastAudioManager test that creates AudioOutputStreamProxy,
which is used in production.
==========

to

==========
[chromecast] Moves CastAudioOutputStream::Backend to CMA thread.

crrev.com/2cbd4fcab300af5aa4ef76752673901d58da0f24 moved
CastAudioOutputStream along with CastAudioOutputStream::Backend to
AudioThread, which is wrong. The backend need to run on CMA thread.

In addition to fixing the threading model, this patch:
- Lets AudioManager use MediaPipelineBackendFactory instead of BackendManager
- Updates tests to use the threading model used in production
- Adds another basic CastAudioManager test that creates AudioOutputStreamProxy,
which is used in production.

Review-Url: https://codereview.chromium.org/2879703003
Cr-Commit-Position: refs/heads/master@{#475726}
Committed:
https://chromium.googlesource.com/chromium/src/+/2341e24bf7f4b05f38a2c5935416...
==========

[commit-bot: I haz the power, 2017-05-31 01:40:04]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/2341e24bf7f4b05f38a2c5935416...