Chromecast: adds a media pipeline feeding data to CMA device backends.

Chromecast: adds a media pipeline feeding data to CMA device backends.

The pipeline state transitions are based closely on the interface
of ::media::Renderer. The pipeline interfaces themselves are intended
to be used both renderer- and browser-side, with future renderer-side
implementations using the CMA IPC code for data feeding.

R=lcwu@chromium.org,damienv@chromium.org,servolk@chromium.org
BUG=408189
Committed: https://crrev.com/bd08add63918abae4ef8fb10b515bef9ce153d14
Cr-Commit-Position: refs/heads/master@{#305081}

[gunsch, 2014-11-19 23:05:30]

Patchset #2 (id:20001) has been deleted

[gunsch, 2014-11-19 23:05:46]

Patchset #1 (id:1) has been deleted

[damienv1, 2014-11-20 17:08:26]

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
File chromecast/media/cma/pipeline/av_pipeline_impl.cc (right):

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
chromecast/media/cma/pipeline/av_pipeline_impl.cc:60: if (media_keys_ &&
media_keys_callback_id_ != kNoCallbackId) {
nit: remove curly brace

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
chromecast/media/cma/pipeline/av_pipeline_impl.cc:131: return false;
StartPlayingFrom used to be 2 different actions (Preroll + Play) where Play was
done *only* if Preroll was successful.

I think we should move this state transition before starting feeding the
pipeline (if there is an error, feeding the pipeline should not be done).

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
chromecast/media/cma/pipeline/av_pipeline_impl.cc:141:
media_component_device_->GetState(), MediaComponentDevice::kStateRunning);
Can you add a comment for this block of code to mention that returning to the
idle state aborts any pending frame push operation.

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
chromecast/media/cma/pipeline/av_pipeline_impl.cc:193: if (media_keys_ &&
media_keys_callback_id_ != kNoCallbackId) {
nit: remove curly braces.

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
chromecast/media/cma/pipeline/av_pipeline_impl.cc:233: if
(audio_config.IsValidConfig() || video_config.IsValidConfig()) {
nit: remove curly braces.

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
File chromecast/media/cma/pipeline/av_pipeline_impl.h (right):

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
chromecast/media/cma/pipeline/av_pipeline_impl.h:48: // |buffering_controller|
can be NULL if no buffering strategy is needed.
Comment could be removed (the controller is not given to AvPipelineImpl anymore)

[damienv1, 2014-11-20 17:28:28]

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right):

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
chromecast/media/cma/pipeline/media_pipeline_impl.cc:259: if (has_audio_) {
nit: remove curly braces (here and below)

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
chromecast/media/cma/pipeline/media_pipeline_impl.cc:271: if
(!buffering_controller_ || !buffering_controller_->IsBuffering()) {
nit: curly brace not needed.

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
File chromecast/media/cma/pipeline/video_pipeline_impl.cc (right):

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
chromecast/media/cma/pipeline/video_pipeline_impl.cc:138:
!video_client_.av_pipeline_client.playback_error_cb.is_null())
nit: multiple line "if" statement => should have curly braces (update also
AudioPipelineImpl)

[damienv1, 2014-11-20 17:33:39]

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
File chromecast/media/cma/pipeline/av_pipeline_impl.cc (right):

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
chromecast/media/cma/pipeline/av_pipeline_impl.cc:151: void
AvPipelineImpl::Flush(const base::Closure& done_cb) {
Could you merge Pause and Flush (always used together in
Audio/VideoPipelineImpl) ?

[gunsch, 2014-11-20 19:50:27]

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
File chromecast/media/cma/pipeline/av_pipeline_impl.cc (right):

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
chromecast/media/cma/pipeline/av_pipeline_impl.cc:60: if (media_keys_ &&
media_keys_callback_id_ != kNoCallbackId) {
On 2014/11/20 17:08:26, damienv1 wrote:
> nit: remove curly brace

Done.

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
chromecast/media/cma/pipeline/av_pipeline_impl.cc:131: return false;
On 2014/11/20 17:08:26, damienv1 wrote:
> StartPlayingFrom used to be 2 different actions (Preroll + Play) where Play
was
> done *only* if Preroll was successful.
> 
> I think we should move this state transition before starting feeding the
> pipeline (if there is an error, feeding the pipeline should not be done).

Strictly speaking, this code is in order from when Preroll/Play were separate,
but that SGTM. Done.

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
chromecast/media/cma/pipeline/av_pipeline_impl.cc:141:
media_component_device_->GetState(), MediaComponentDevice::kStateRunning);
On 2014/11/20 17:08:26, damienv1 wrote:
> Can you add a comment for this block of code to mention that returning to the
> idle state aborts any pending frame push operation.

Done.

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
chromecast/media/cma/pipeline/av_pipeline_impl.cc:151: void
AvPipelineImpl::Flush(const base::Closure& done_cb) {
On 2014/11/20 17:33:39, damienv1 wrote:
> Could you merge Pause and Flush (always used together in
> Audio/VideoPipelineImpl) ?

Done.

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
chromecast/media/cma/pipeline/av_pipeline_impl.cc:193: if (media_keys_ &&
media_keys_callback_id_ != kNoCallbackId) {
On 2014/11/20 17:08:26, damienv1 wrote:
> nit: remove curly braces.

Done.

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
chromecast/media/cma/pipeline/av_pipeline_impl.cc:233: if
(audio_config.IsValidConfig() || video_config.IsValidConfig()) {
On 2014/11/20 17:08:26, damienv1 wrote:
> nit: remove curly braces.

Done.

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
File chromecast/media/cma/pipeline/av_pipeline_impl.h (right):

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
chromecast/media/cma/pipeline/av_pipeline_impl.h:48: // |buffering_controller|
can be NULL if no buffering strategy is needed.
On 2014/11/20 17:08:26, damienv1 wrote:
> Comment could be removed (the controller is not given to AvPipelineImpl
anymore)

Done.

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right):

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
chromecast/media/cma/pipeline/media_pipeline_impl.cc:259: if (has_audio_) {
On 2014/11/20 17:28:28, damienv1 wrote:
> nit: remove curly braces (here and below)

Done.

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
chromecast/media/cma/pipeline/media_pipeline_impl.cc:271: if
(!buffering_controller_ || !buffering_controller_->IsBuffering()) {
On 2014/11/20 17:28:28, damienv1 wrote:
> nit: curly brace not needed.

Done.

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
File chromecast/media/cma/pipeline/video_pipeline_impl.cc (right):

https://codereview.chromium.org/741863002/diff/40001/chromecast/media/cma/pip...
chromecast/media/cma/pipeline/video_pipeline_impl.cc:138:
!video_client_.av_pipeline_client.playback_error_cb.is_null())
On 2014/11/20 17:28:28, damienv1 wrote:
> nit: multiple line "if" statement => should have curly braces (update also
> AudioPipelineImpl)

Done. (not in AudioPipelineImpl, it's a single line there)

[damienv1, 2014-11-20 20:14:30]

lgtm

[gunsch, 2014-11-20 20:45:11]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-11-20 20:46:53]

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

[commit-bot: I haz the power, 2014-11-20 21:31:24]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2014-11-20 21:32:04]

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