[Chromecast] Upgrade to new CMA backend API

Upgrade to new CMA backend API

Committed: https://crrev.com/13e7a879298533b9fb111495c8d7dd1b954aae03
Cr-Commit-Position: refs/heads/master@{#354307}

[kmackay, 2015-10-06 04:24:47]

kmackay@chromium.org changed reviewers:
+ alokp@chromium.org, erickung@chromium.org, halliwell@chromium.org,
servolk@chromium.org, slan@chromium.org

[halliwell, 2015-10-06 05:01:44]

On 2015/10/06 04:25:00, kmackay wrote:

Context on cma_end_to_end_test in case you're working on that and didn't know:
* Previously, the pipeline on both renderer and browser process side implemented
the same base interface
* I decided to break the (browser pipeline) -> base interface inheritance, in
order to change the browser implementation without affecting renderer code
* Unfortunately, I later realised cma_end_to_end_test took advantage of the
common interface to drive the pipeline "end-to-end" by directly plugging the
browser backend into cma_renderer, using the shared interface in place of IPC. 
So ... while end-to-end, the middle is technically missing :)

There are obviously a variety of general solution ideas ... e.g. 
* Update the renderer side to match the browser and have them share a base
interface again.
* Implement a little shim into cma_end_to_end_test to adapt the browser side to
the renderer interface.

I was probably going to try out the second of those first ... but take a look
and do whatever you think's best :)

[slan, 2015-10-06 19:34:01]

Okay, I did a pretty coarse pass to try to understand things at a high level
before diving in fully. My takeaways and questions:

1) I did not look at any tests or build files. Also, changes to renderer/ and
browser/ dirs seem pretty cosmetic. Renames, etc. I will examine all of these
changes more closely on the next pass. 

2) I need clarification on the expectation of Create*Decoder(). It says that any
impl must support 3 audio and 1 A/V sync decoder. How does a consumer of the
MediaPipelineBackend class know how many times it can safely call those methods?
Can we add a query method or document that a nullptr shall be returned if the
maximum amount is reached?

3) It is my understanding that a WebAudio element, for example, can create an
AudioOutputStream while other renderering is already occurring through another
media element. This would cause the CastAudioOutputStream to attempt to create a
decoder while the backend is in the incorrect state. How are we handling this
case?

All of these questions are addressed in individual comments. Feel free to
address them there. I am a big fan of this change. Nice work!

https://codereview.chromium.org/1372393007/diff/1/chromecast/browser/media/cm...
File chromecast/browser/media/cma_message_filter_host.h (right):

https://codereview.chromium.org/1372393007/diff/1/chromecast/browser/media/cm...
chromecast/browser/media/cma_message_filter_host.h:117: // Factory function for
media pipeline backend
nit: end with '.'

https://codereview.chromium.org/1372393007/diff/1/chromecast/browser/media/me...
File chromecast/browser/media/media_pipeline_host.cc (right):

https://codereview.chromium.org/1372393007/diff/1/chromecast/browser/media/me...
chromecast/browser/media/media_pipeline_host.cc:103: audio_frame_provider_ =
frame_provider.Pass();
Remove line 101, then replace this with:

audio_frame_provider_ = frame_provider_host.Pass();

same for line 105

https://codereview.chromium.org/1372393007/diff/1/chromecast/browser/media/me...
chromecast/browser/media/media_pipeline_host.cc:124: const std::vector<
::media::VideoDecoderConfig>& configs,
1-char nit: remove char.

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/audio/cast...
File chromecast/media/audio/cast_audio_output_stream.cc (right):

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/audio/cast...
chromecast/media/audio/cast_audio_output_stream.cc:76: bool* success,
nit: DCHECK all raw input

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/audio/cast...
chromecast/media/audio/cast_audio_output_stream.cc:81: backend_ =
audio_manager->CreateMediaPipelineBackend();
Can't an AudioOuput stream be created and opened at any time (by, for example, a
WebAudio node?) What if we want to start playing media, then a sound effect
Opens this stream when the |backend_| is already Initialized()?

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/audio/cast...
chromecast/media/audio/cast_audio_output_stream.cc:100: backend_->Start(0);
DCHECK backend_ before use to be defensive. Here and below.

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/audio/cast...
chromecast/media/audio/cast_audio_output_stream.cc:108: backend_->Pause();
DCHECK backend_ before use

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/audio/cast...
chromecast/media/audio/cast_audio_output_stream.cc:136:
decoder_->SetVolume(volume);
DCHECK(decoder_) before use

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/cma/backen...
File chromecast/media/cma/backend/media_pipeline_backend_default.cc (right):

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/media_pipeline_backend_default.cc:80: delete
audio_decoder_;
1) Since we already have //base deps, why not used scoped_ptr here?

2) If no scoped_ptr, please check that these are not nullptr before attempting
to delete them.

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/media_pipeline_backend_default.cc:85:
MediaPipelineBackendDefault::CreateAudioDecoder() {
Don't the requirements state that the backend must provide 3+1 A/V decoders?
Shouldn't we be able to call this function 3+1 times before it DCHECKs or
something?

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/media_pipeline_backend_default.cc:117: start_pts_ =
base::TimeDelta::FromMicroseconds(GetCurrentPts());
You want to call this before setting running_ to false. Otherwise start_pts_
will not increment. In Pause() too.

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/media_pipeline_backend_default.cc:146: DCHECK(rate
!= 0);
nit: rate > 0

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/cma/pipeli...
File chromecast/media/cma/pipeline/av_pipeline_client.cc (right):

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/cma/pipeli...
chromecast/media/cma/pipeline/av_pipeline_client.cc:12:
RendererAvPipelineClient::~RendererAvPipelineClient() {
nit: I know Ken is splitting these out, but let's add a space if possible.

https://codereview.chromium.org/1372393007/diff/1/chromecast/public/cast_medi...
File chromecast/public/cast_media_shlib.h (right):

https://codereview.chromium.org/1372393007/diff/1/chromecast/public/cast_medi...
chromecast/public/cast_media_shlib.h:46: // media pipeline and raw audio stream.
Even though each MediaPipelineBackend must be able to return 3+1 A/V decoders,
this interface returns a single global instance, and ownership is retained. Can
we add these details to the comment?

https://codereview.chromium.org/1372393007/diff/1/chromecast/public/media/med...
File chromecast/public/media/media_pipeline_backend.h (right):

https://codereview.chromium.org/1372393007/diff/1/chromecast/public/media/med...
chromecast/public/media/media_pipeline_backend.h:19: // decoders, they must be
synchronized.  Platforms must support 1
We should be able to query the backend for the number of decoders it can
provide. Otherwise, we must assume only the bare minimum.

https://codereview.chromium.org/1372393007/diff/1/chromecast/public/media/med...
chromecast/public/media/media_pipeline_backend.h:118: virtual void
OnVideoResolutionChanged(Decoder* decoder,
Why not VideoDecoder* here? Does a Delegate for an AudioDecoder need to know
this as well?

https://codereview.chromium.org/1372393007/diff/1/chromecast/public/media/med...
chromecast/public/media/media_pipeline_backend.h:125: // Must be called after an
end-of-stream buffer has been rendered.
Define "rendered" here: decoded?

[kmackay, 2015-10-06 21:44:55]

https://codereview.chromium.org/1372393007/diff/1/chromecast/browser/media/cm...
File chromecast/browser/media/cma_message_filter_host.h (right):

https://codereview.chromium.org/1372393007/diff/1/chromecast/browser/media/cm...
chromecast/browser/media/cma_message_filter_host.h:117: // Factory function for
media pipeline backend
On 2015/10/06 19:33:59, slan wrote:
> nit: end with '.'

Done.

https://codereview.chromium.org/1372393007/diff/1/chromecast/browser/media/me...
File chromecast/browser/media/media_pipeline_host.cc (right):

https://codereview.chromium.org/1372393007/diff/1/chromecast/browser/media/me...
chromecast/browser/media/media_pipeline_host.cc:103: audio_frame_provider_ =
frame_provider.Pass();
On 2015/10/06 19:33:59, slan wrote:
> Remove line 101, then replace this with:
> 
> audio_frame_provider_ = frame_provider_host.Pass();
> 
> same for line 105

Done.

https://codereview.chromium.org/1372393007/diff/1/chromecast/browser/media/me...
chromecast/browser/media/media_pipeline_host.cc:124: const std::vector<
::media::VideoDecoderConfig>& configs,
On 2015/10/06 19:33:59, slan wrote:
> 1-char nit: remove char.

clang-format likes it with the space.

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/audio/cast...
File chromecast/media/audio/cast_audio_output_stream.cc (right):

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/audio/cast...
chromecast/media/audio/cast_audio_output_stream.cc:76: bool* success,
On 2015/10/06 19:33:59, slan wrote:
> nit: DCHECK all raw input

Done.

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/audio/cast...
chromecast/media/audio/cast_audio_output_stream.cc:81: backend_ =
audio_manager->CreateMediaPipelineBackend();
On 2015/10/06 19:34:00, slan wrote:
> Can't an AudioOuput stream be created and opened at any time (by, for example,
a
> WebAudio node?) What if we want to start playing media, then a sound effect
> Opens this stream when the |backend_| is already Initialized()?

A whole new backend is created for each stream.

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/audio/cast...
chromecast/media/audio/cast_audio_output_stream.cc:100: backend_->Start(0);
On 2015/10/06 19:34:00, slan wrote:
> DCHECK backend_ before use to be defensive. Here and below.

Done.

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/audio/cast...
chromecast/media/audio/cast_audio_output_stream.cc:108: backend_->Pause();
On 2015/10/06 19:34:00, slan wrote:
> DCHECK backend_ before use

Done.

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/audio/cast...
chromecast/media/audio/cast_audio_output_stream.cc:136:
decoder_->SetVolume(volume);
On 2015/10/06 19:34:00, slan wrote:
> DCHECK(decoder_) before use

Done.

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/cma/backen...
File chromecast/media/cma/backend/media_pipeline_backend_default.cc (right):

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/media_pipeline_backend_default.cc:80: delete
audio_decoder_;
On 2015/10/06 19:34:00, slan wrote:
> 1) Since we already have //base deps, why not used scoped_ptr here?
> 
> 2) If no scoped_ptr, please check that these are not nullptr before attempting
> to delete them.

deleting nullptr is extremely valid.

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/media_pipeline_backend_default.cc:85:
MediaPipelineBackendDefault::CreateAudioDecoder() {
On 2015/10/06 19:34:00, slan wrote:
> Don't the requirements state that the backend must provide 3+1 A/V decoders?
> Shouldn't we be able to call this function 3+1 times before it DCHECKs or
> something?

The requirement is that the vendor media shlib is able to create 3+1
MediaPipelineBackend instances (one for synchronized audio+video, and 3 for just
audio). Maybe the header comments are poorly worded; I'll try to clarify.

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/media_pipeline_backend_default.cc:117: start_pts_ =
base::TimeDelta::FromMicroseconds(GetCurrentPts());
On 2015/10/06 19:34:00, slan wrote:
> You want to call this before setting running_ to false. Otherwise start_pts_
> will not increment. In Pause() too.

Done.

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/media_pipeline_backend_default.cc:146: DCHECK(rate
!= 0);
On 2015/10/06 19:34:00, slan wrote:
> nit: rate > 0

0 is potentially valid here.

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/cma/pipeli...
File chromecast/media/cma/pipeline/av_pipeline_client.cc (right):

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/cma/pipeli...
chromecast/media/cma/pipeline/av_pipeline_client.cc:12:
RendererAvPipelineClient::~RendererAvPipelineClient() {
On 2015/10/06 19:34:00, slan wrote:
> nit: I know Ken is splitting these out, but let's add a space if possible.

Done.

https://codereview.chromium.org/1372393007/diff/1/chromecast/public/cast_medi...
File chromecast/public/cast_media_shlib.h (right):

https://codereview.chromium.org/1372393007/diff/1/chromecast/public/cast_medi...
chromecast/public/cast_media_shlib.h:46: // media pipeline and raw audio stream.
On 2015/10/06 19:34:00, slan wrote:
> Even though each MediaPipelineBackend must be able to return 3+1 A/V decoders,
> this interface returns a single global instance, and ownership is retained.
Can
> we add these details to the comment?

Updated the comments. This function is called once to create a new backend
instance for each new media playback stream.

https://codereview.chromium.org/1372393007/diff/1/chromecast/public/media/med...
File chromecast/public/media/media_pipeline_backend.h (right):

https://codereview.chromium.org/1372393007/diff/1/chromecast/public/media/med...
chromecast/public/media/media_pipeline_backend.h:19: // decoders, they must be
synchronized.  Platforms must support 1
On 2015/10/06 19:34:00, slan wrote:
> We should be able to query the backend for the number of decoders it can
> provide. Otherwise, we must assume only the bare minimum.

Added comment that the backend can return nullptr for CreateVideoDecoder() /
CreateAudioDecoder().

https://codereview.chromium.org/1372393007/diff/1/chromecast/public/media/med...
chromecast/public/media/media_pipeline_backend.h:118: virtual void
OnVideoResolutionChanged(Decoder* decoder,
On 2015/10/06 19:34:00, slan wrote:
> Why not VideoDecoder* here? Does a Delegate for an AudioDecoder need to know
> this as well?

Done.

https://codereview.chromium.org/1372393007/diff/1/chromecast/public/media/med...
chromecast/public/media/media_pipeline_backend.h:125: // Must be called after an
end-of-stream buffer has been rendered.
On 2015/10/06 19:34:00, slan wrote:
> Define "rendered" here: decoded?

Rendered to output (ie, sent to the output hardware).

[slan, 2015-10-06 22:15:06]

Thanks for the clarification.

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/audio/cast...
File chromecast/media/audio/cast_audio_output_stream.cc (right):

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/audio/cast...
chromecast/media/audio/cast_audio_output_stream.cc:81: backend_ =
audio_manager->CreateMediaPipelineBackend();
On 2015/10/06 21:44:55, kmackay wrote:
> On 2015/10/06 19:34:00, slan wrote:
> > Can't an AudioOuput stream be created and opened at any time (by, for
example,
> a
> > WebAudio node?) What if we want to start playing media, then a sound effect
> > Opens this stream when the |backend_| is already Initialized()?
> 
> A whole new backend is created for each stream.

Duh, thank you.

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/cma/backen...
File chromecast/media/cma/backend/media_pipeline_backend_default.cc (right):

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/media_pipeline_backend_default.cc:80: delete
audio_decoder_;
On 2015/10/06 21:44:55, kmackay wrote:
> On 2015/10/06 19:34:00, slan wrote:
> > 1) Since we already have //base deps, why not used scoped_ptr here?
> > 
> > 2) If no scoped_ptr, please check that these are not nullptr before
attempting
> > to delete them.
> 
> deleting nullptr is extremely valid.

TIL.

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/media_pipeline_backend_default.cc:85:
MediaPipelineBackendDefault::CreateAudioDecoder() {
On 2015/10/06 21:44:55, kmackay wrote:
> On 2015/10/06 19:34:00, slan wrote:
> > Don't the requirements state that the backend must provide 3+1 A/V decoders?
> > Shouldn't we be able to call this function 3+1 times before it DCHECKs or
> > something?
> 
> The requirement is that the vendor media shlib is able to create 3+1
> MediaPipelineBackend instances (one for synchronized audio+video, and 3 for
just
> audio). Maybe the header comments are poorly worded; I'll try to clarify.

Ah much clearer, thanks!

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/media_pipeline_backend_default.cc:146: DCHECK(rate
!= 0);
On 2015/10/06 21:44:55, kmackay wrote:
> On 2015/10/06 19:34:00, slan wrote:
> > nit: rate > 0
> 
> 0 is potentially valid here.

The comments in the header (pasted below) hold that it must be positive. Is
there a good use case for 0? I would think that pause should be used instead. 

Comment:
 // Sets the playback rate.  |rate| > 0.

https://codereview.chromium.org/1372393007/diff/1/chromecast/public/cast_medi...
File chromecast/public/cast_media_shlib.h (right):

https://codereview.chromium.org/1372393007/diff/1/chromecast/public/cast_medi...
chromecast/public/cast_media_shlib.h:46: // media pipeline and raw audio stream.
On 2015/10/06 21:44:55, kmackay wrote:
> On 2015/10/06 19:34:00, slan wrote:
> > Even though each MediaPipelineBackend must be able to return 3+1 A/V
decoders,
> > this interface returns a single global instance, and ownership is retained.
> Can
> > we add these details to the comment?
> 
> Updated the comments. This function is called once to create a new backend
> instance for each new media playback stream.

Thanks, much clearer.

https://codereview.chromium.org/1372393007/diff/1/chromecast/public/media/med...
File chromecast/public/media/media_pipeline_backend.h (right):

https://codereview.chromium.org/1372393007/diff/1/chromecast/public/media/med...
chromecast/public/media/media_pipeline_backend.h:19: // decoders, they must be
synchronized.  Platforms must support 1
On 2015/10/06 21:44:55, kmackay wrote:
> On 2015/10/06 19:34:00, slan wrote:
> > We should be able to query the backend for the number of decoders it can
> > provide. Otherwise, we must assume only the bare minimum.
> 
> Added comment that the backend can return nullptr for CreateVideoDecoder() /
> CreateAudioDecoder().

Thanks.

[slan, 2015-10-07 02:06:47]

This sweep regards reducing the renderer-side complexity

https://codereview.chromium.org/1372393007/diff/20001/chromecast/browser/medi...
File chromecast/browser/media/cma_media_pipeline_client.h (right):

https://codereview.chromium.org/1372393007/diff/20001/chromecast/browser/medi...
chromecast/browser/media/cma_media_pipeline_client.h:31: // cast::CastResource
implementations
nit: // cast::CastResource implementation:

https://codereview.chromium.org/1372393007/diff/20001/chromecast/public/cast_...
File chromecast/public/cast_media_shlib.h (right):

https://codereview.chromium.org/1372393007/diff/20001/chromecast/public/cast_...
chromecast/public/cast_media_shlib.h:46: // media pipeline and raw audio stream.
The caller owns the returned instance correct? Please explicitly state this in
the comments.

https://codereview.chromium.org/1372393007/diff/20001/chromecast/renderer/med...
File chromecast/renderer/media/audio_pipeline.h (right):

https://codereview.chromium.org/1372393007/diff/20001/chromecast/renderer/med...
chromecast/renderer/media/audio_pipeline.h:14: class AudioPipeline {
I don't think this class needs to exist anymore. Since AudioPipelineProxy no
longer inherits from the same class as the browser-side AudioPipeline, this is
now pretty useless. IMO, we have two options:

1) AudioPipelineProxy does not inherit from any class, and its methods are made
virtual for mocking in unittests.

2) AudioPipelineProxy becomes a pure virtual interface, and we rename the
existing implemented class to AudioPipelineProxyImpl.

I think I prefer #1 for simplicity, and to reduce the complexity of this stack.
The exact same thing applies to VideoPipeline.

https://codereview.chromium.org/1372393007/diff/20001/chromecast/renderer/med...
File chromecast/renderer/media/renderer_media_pipeline.h (right):

https://codereview.chromium.org/1372393007/diff/20001/chromecast/renderer/med...
chromecast/renderer/media/renderer_media_pipeline.h:29: RendererMediaPipeline()
{}
This class can also be simplified in the way that AudioPipeline and
VideoPipeline can.

[kmackay, 2015-10-07 16:37:58]

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/cma/backen...
File chromecast/media/cma/backend/media_pipeline_backend_default.cc (right):

https://codereview.chromium.org/1372393007/diff/1/chromecast/media/cma/backen...
chromecast/media/cma/backend/media_pipeline_backend_default.cc:146: DCHECK(rate
!= 0);
On 2015/10/06 22:15:05, slan wrote:
> On 2015/10/06 21:44:55, kmackay wrote:
> > On 2015/10/06 19:34:00, slan wrote:
> > > nit: rate > 0
> > 
> > 0 is potentially valid here.
> 
> The comments in the header (pasted below) hold that it must be positive. Is
> there a good use case for 0? I would think that pause should be used instead. 
> 
> Comment:
>  // Sets the playback rate.  |rate| > 0. 

Done.

https://codereview.chromium.org/1372393007/diff/20001/chromecast/browser/medi...
File chromecast/browser/media/cma_media_pipeline_client.h (right):

https://codereview.chromium.org/1372393007/diff/20001/chromecast/browser/medi...
chromecast/browser/media/cma_media_pipeline_client.h:31: // cast::CastResource
implementations
On 2015/10/07 02:06:47, slan wrote:
> nit: // cast::CastResource implementation:

Done.

https://codereview.chromium.org/1372393007/diff/20001/chromecast/public/cast_...
File chromecast/public/cast_media_shlib.h (right):

https://codereview.chromium.org/1372393007/diff/20001/chromecast/public/cast_...
chromecast/public/cast_media_shlib.h:46: // media pipeline and raw audio stream.
On 2015/10/07 02:06:47, slan wrote:
> The caller owns the returned instance correct? Please explicitly state this in
> the comments.

Done.

https://codereview.chromium.org/1372393007/diff/20001/chromecast/renderer/med...
File chromecast/renderer/media/audio_pipeline.h (right):

https://codereview.chromium.org/1372393007/diff/20001/chromecast/renderer/med...
chromecast/renderer/media/audio_pipeline.h:14: class AudioPipeline {
On 2015/10/07 02:06:47, slan wrote:
> I don't think this class needs to exist anymore. Since AudioPipelineProxy no
> longer inherits from the same class as the browser-side AudioPipeline, this is
> now pretty useless. IMO, we have two options:
> 
> 1) AudioPipelineProxy does not inherit from any class, and its methods are
made
> virtual for mocking in unittests.
> 
> 2) AudioPipelineProxy becomes a pure virtual interface, and we rename the
> existing implemented class to AudioPipelineProxyImpl.
> 
> I think I prefer #1 for simplicity, and to reduce the complexity of this
stack.
> The exact same thing applies to VideoPipeline.

Done.

https://codereview.chromium.org/1372393007/diff/20001/chromecast/renderer/med...
File chromecast/renderer/media/renderer_media_pipeline.h (right):

https://codereview.chromium.org/1372393007/diff/20001/chromecast/renderer/med...
chromecast/renderer/media/renderer_media_pipeline.h:29: RendererMediaPipeline()
{}
On 2015/10/07 02:06:47, slan wrote:
> This class can also be simplified in the way that AudioPipeline and
> VideoPipeline can.

Done.

[slan, 2015-10-07 20:56:17]

Please see this comment first:
https://codereview.chromium.org/1372393007/diff/60001/chromecast/media/cma/pi...

Then address all other comments.

https://codereview.chromium.org/1372393007/diff/40001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/audio_pipeline_impl.h (right):

https://codereview.chromium.org/1372393007/diff/40001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/audio_pipeline_impl.h:29: class AudioPipelineImpl
{
Can this just be called AudioPipeline now that it does not implement anything?
This applies to:

AudioPipelineImpl
VideoPipelineImpl
MediaPipelineImpl
AvPipelineImpl

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

https://codereview.chromium.org/1372393007/diff/40001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/av_pipeline_impl.h:82: // Callback invoked when
playback has reached the end of stream.
nit: rm

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

https://codereview.chromium.org/1372393007/diff/40001/chromecast/renderer/med...
chromecast/renderer/media/chromecast_media_renderer_factory.cc:68:
scoped_ptr<::media::Renderer> default_media_render(
nit: default_media_renderer

https://codereview.chromium.org/1372393007/diff/40001/chromecast/renderer/med...
chromecast/renderer/media/chromecast_media_renderer_factory.cc:69:
default_render_factory_->CreateRenderer(
nit: default_renderer_factory_

https://codereview.chromium.org/1372393007/diff/60001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/audio_pipeline_impl.cc (right):

https://codereview.chromium.org/1372393007/diff/60001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/audio_pipeline_impl.cc:79: CMALOG(kLogControl) <<
"AudioPipelineImpl::OnFlushDone";
The __FUNCTION__ macros should be used here, similar to what we have done with
TRACE_EVENT in the internal ALSA CMA impl.

Here + everywhere.

https://codereview.chromium.org/1372393007/diff/60001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/audio_pipeline_impl.h (right):

https://codereview.chromium.org/1372393007/diff/60001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/audio_pipeline_impl.h:29: class AudioPipelineImpl
{
Both this class and VideoPipelineImpl are thin, stateless control managers of an
owned AvPipelineImpl instance, which basically gets the IPC data and passes it
to a decoder, and manages state. These  manager classes share massive amounts of
code (copy-paste), both in interface and implementation. The only differences:
 - The type of client passed in the ctor.
 - Different statistics are picked up.
 - Config types.
 - Audio* cares about volume.

I would love if we could converge these into one class (maybe called
AvPipelineManager), and isolate all audio/video specific logic to MediaPipeline.
MediaPipeline would create an audio and a video instance, but the class used for
each would be the same. 

If this is not be possible, we should, at the very least, make a base class
AvPipelineManagerBase, from which AudioAvPipelineManager and
VideoAvPipelineManager inherit, and which implement the differences listed
above. This would eliminate the copy-pasta.

WDYT? I perceive these layers to be unnecessarily complex, but I could be
oversimplifying things.

https://codereview.chromium.org/1372393007/diff/60001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/av_pipeline_impl.cc (right):

https://codereview.chromium.org/1372393007/diff/60001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/av_pipeline_impl.cc:273: void
AvPipelineImpl::OnFrameBuffered(
This should be OnBufferBuffered() to maintain consistency...

[erickung1, 2015-10-09 17:53:29]

https://codereview.chromium.org/1372393007/diff/60001/chromecast/public/media...
File chromecast/public/media/media_pipeline_backend.h (right):

https://codereview.chromium.org/1372393007/diff/60001/chromecast/public/media...
chromecast/public/media/media_pipeline_backend.h:11: struct Size;
nit: a blank line between #10 and #11

https://codereview.chromium.org/1372393007/diff/60001/chromecast/public/media...
chromecast/public/media/media_pipeline_backend.h:13: class CastDecoderBuffer;
nit: ditto between #12 and #13

https://codereview.chromium.org/1372393007/diff/60001/chromecast/public/media...
chromecast/public/media/media_pipeline_backend.h:20: // synchronized backend and
>= 3 backends with audio only simultaneously. If
Is this the restriction in Chromecast + Chromium restriction or in OEM partner?
We don't want to put such description for the later. 

We should put something like "currently cast platform should be able to
support.... blablabla.."

https://codereview.chromium.org/1372393007/diff/60001/chromecast/public/media...
chromecast/public/media/media_pipeline_backend.h:39: class Decoder {
should we hide destructor of Decoder/VideoDecoder/AudioDecoder to private and
only be accessible by MEdiaPipelineBackend? In that case our front end code
won't be able to do stupid things like deleting the decoder by accidents.

https://codereview.chromium.org/1372393007/diff/60001/chromecast/public/media...
chromecast/public/media/media_pipeline_backend.h:178: // Gets the current
playback time.
May be better to provide the unit of the value it returns

[erickung1, 2015-10-09 18:00:14]

https://codereview.chromium.org/1372393007/diff/60001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/audio_pipeline_impl.h (right):

https://codereview.chromium.org/1372393007/diff/60001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/audio_pipeline_impl.h:29: class AudioPipelineImpl
{
On 2015/10/07 20:56:16, slan wrote:
> Both this class and VideoPipelineImpl are thin, stateless control managers of
an
> owned AvPipelineImpl instance, which basically gets the IPC data and passes it
> to a decoder, and manages state. These  manager classes share massive amounts
of
> code (copy-paste), both in interface and implementation. The only differences:
>  - The type of client passed in the ctor.
>  - Different statistics are picked up.
>  - Config types.
>  - Audio* cares about volume.
> 
> I would love if we could converge these into one class (maybe called
> AvPipelineManager), and isolate all audio/video specific logic to
MediaPipeline.
> MediaPipeline would create an audio and a video instance, but the class used
for
> each would be the same. 
> 
> If this is not be possible, we should, at the very least, make a base class
> AvPipelineManagerBase, from which AudioAvPipelineManager and
> VideoAvPipelineManager inherit, and which implement the differences listed
> above. This would eliminate the copy-pasta.
> 
> WDYT? I perceive these layers to be unnecessarily complex, but I could be
> oversimplifying things.
(Maybe my understanding on your comment is not correct, please feel free to
correct me)

This is another aspect of refactoring the code. The original code is to have
common code in av_pipeline_impl, and leave AudioPipelineImpl and
VideoPipelineImpl here due to the different structure of Backend interface. What
you're suggesting is the other way around? to offload more common code base to
av_pipeline_impl like implementation make it as root.

I'm not sure if we want to make such code change in this CL, given the fact that
we touch so many interfaces which shouldn't change the code structure to create
unexpected regression given the short time frame.

[kmackay, 2015-10-09 20:35:40]

https://codereview.chromium.org/1372393007/diff/40001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/audio_pipeline_impl.h (right):

https://codereview.chromium.org/1372393007/diff/40001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/audio_pipeline_impl.h:29: class AudioPipelineImpl
{
On 2015/10/07 20:56:16, slan wrote:
> Can this just be called AudioPipeline now that it does not implement anything?
> This applies to:
> 
> AudioPipelineImpl
> VideoPipelineImpl
> MediaPipelineImpl
> AvPipelineImpl

I will rename things in a separate CL.

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

https://codereview.chromium.org/1372393007/diff/40001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/av_pipeline_impl.h:82: // Callback invoked when
playback has reached the end of stream.
On 2015/10/07 20:56:16, slan wrote:
> nit: rm

Done.

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

https://codereview.chromium.org/1372393007/diff/40001/chromecast/renderer/med...
chromecast/renderer/media/chromecast_media_renderer_factory.cc:68:
scoped_ptr<::media::Renderer> default_media_render(
On 2015/10/07 20:56:16, slan wrote:
> nit: default_media_renderer

Done.

https://codereview.chromium.org/1372393007/diff/40001/chromecast/renderer/med...
chromecast/renderer/media/chromecast_media_renderer_factory.cc:69:
default_render_factory_->CreateRenderer(
On 2015/10/07 20:56:16, slan wrote:
> nit: default_renderer_factory_

Done.

https://codereview.chromium.org/1372393007/diff/60001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/audio_pipeline_impl.cc (right):

https://codereview.chromium.org/1372393007/diff/60001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/audio_pipeline_impl.cc:79: CMALOG(kLogControl) <<
"AudioPipelineImpl::OnFlushDone";
On 2015/10/07 20:56:16, slan wrote:
> The __FUNCTION__ macros should be used here, similar to what we have done with
> TRACE_EVENT in the internal ALSA CMA impl.
> 
> Here + everywhere.

Done.

https://codereview.chromium.org/1372393007/diff/60001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/audio_pipeline_impl.h (right):

https://codereview.chromium.org/1372393007/diff/60001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/audio_pipeline_impl.h:29: class AudioPipelineImpl
{
On 2015/10/07 20:56:16, slan wrote:
> Both this class and VideoPipelineImpl are thin, stateless control managers of
an
> owned AvPipelineImpl instance, which basically gets the IPC data and passes it
> to a decoder, and manages state. These  manager classes share massive amounts
of
> code (copy-paste), both in interface and implementation. The only differences:
>  - The type of client passed in the ctor.
>  - Different statistics are picked up.
>  - Config types.
>  - Audio* cares about volume.
> 
> I would love if we could converge these into one class (maybe called
> AvPipelineManager), and isolate all audio/video specific logic to
MediaPipeline.
> MediaPipeline would create an audio and a video instance, but the class used
for
> each would be the same. 
> 
> If this is not be possible, we should, at the very least, make a base class
> AvPipelineManagerBase, from which AudioAvPipelineManager and
> VideoAvPipelineManager inherit, and which implement the differences listed
> above. This would eliminate the copy-pasta.
> 
> WDYT? I perceive these layers to be unnecessarily complex, but I could be
> oversimplifying things.

I think this would be a good change, but I will do it in a separate CL.

https://codereview.chromium.org/1372393007/diff/60001/chromecast/media/cma/pi...
File chromecast/media/cma/pipeline/av_pipeline_impl.cc (right):

https://codereview.chromium.org/1372393007/diff/60001/chromecast/media/cma/pi...
chromecast/media/cma/pipeline/av_pipeline_impl.cc:273: void
AvPipelineImpl::OnFrameBuffered(
On 2015/10/07 20:56:17, slan wrote:
> This should be OnBufferBuffered() to maintain consistency...

Changed to OnDataBuffered().

https://codereview.chromium.org/1372393007/diff/60001/chromecast/public/media...
File chromecast/public/media/media_pipeline_backend.h (right):

https://codereview.chromium.org/1372393007/diff/60001/chromecast/public/media...
chromecast/public/media/media_pipeline_backend.h:11: struct Size;
On 2015/10/09 17:53:29, erickung1 wrote:
> nit: a blank line between #10 and #11

I put a blank line before the media namespace. I usually don't add a blank line
before predeclared classes.

https://codereview.chromium.org/1372393007/diff/60001/chromecast/public/media...
chromecast/public/media/media_pipeline_backend.h:20: // synchronized backend and
>= 3 backends with audio only simultaneously. If
On 2015/10/09 17:53:29, erickung1 wrote:
> Is this the restriction in Chromecast + Chromium restriction or in OEM
partner?
> We don't want to put such description for the later. 
> 
> We should put something like "currently cast platform should be able to
> support.... blablabla.."
> 

I removed the comment, but we need to have some guidelines for partners.

https://codereview.chromium.org/1372393007/diff/60001/chromecast/public/media...
chromecast/public/media/media_pipeline_backend.h:39: class Decoder {
On 2015/10/09 17:53:29, erickung1 wrote:
> should we hide destructor of Decoder/VideoDecoder/AudioDecoder to private and
> only be accessible by MEdiaPipelineBackend? In that case our front end code
> won't be able to do stupid things like deleting the decoder by accidents.

Done.

https://codereview.chromium.org/1372393007/diff/60001/chromecast/public/media...
chromecast/public/media/media_pipeline_backend.h:178: // Gets the current
playback time.
On 2015/10/09 17:53:29, erickung1 wrote:
> May be better to provide the unit of the value it returns

Done.

[gunsch, 2015-10-12 16:05:11]

gunsch@chromium.org changed reviewers:
+ gunsch@chromium.org

[gunsch, 2015-10-12 16:05:13]

not lgtm

High-level drive-by: breaking the common browser/renderer interfaces makes me
pretty sad. There are a few advantages to that structure:

(1) Allows conceptually running the entire pipeline in-process, from CmaRenderer
on. The CMA end-to-end test takes advantage of this.

(2) It's not tied so much to the browser/renderer pairing specifically. This
allows a clean separation of "media code" from "content code". The
browser/renderer code then allows content to only exist as "wiring up the
processes" as opposed to being a critical piece of the pipeline.

If you're going to insist on separating these and tying CmaRenderer to the
renderer process (via chromecast/renderer/), I would *strongly* object to having
chromecast/media/ depend on chromecast/renderer/ and would instead advise
raising CmaRenderer to live in chromecast/renderer/media/.

https://codereview.chromium.org/1372393007/diff/100001/chromecast/media/cma/f...
File chromecast/media/cma/filters/DEPS (right):

https://codereview.chromium.org/1372393007/diff/100001/chromecast/media/cma/f...
chromecast/media/cma/filters/DEPS:2: "+chromecast/renderer/media",
definitely no

https://codereview.chromium.org/1372393007/diff/100001/chromecast/media/cma/f...
File chromecast/media/cma/filters/cma_renderer.cc (right):

https://codereview.chromium.org/1372393007/diff/100001/chromecast/media/cma/f...
chromecast/media/cma/filters/cma_renderer.cc:19: #include
"chromecast/renderer/media/audio_pipeline_proxy.h"
no no no no no

[gunsch, 2015-10-12 16:06:38]

https://codereview.chromium.org/1372393007/diff/100001/chromecast/media/cma/t...
File chromecast/media/cma/test/DEPS (right):

https://codereview.chromium.org/1372393007/diff/100001/chromecast/media/cma/t...
chromecast/media/cma/test/DEPS:2: "+chromecast/renderer/media",
also no

chromecast/media/ should never depend on content (e.g. chromecast/renderer/)

[kmackay, 2015-10-13 18:21:05]

On 2015/10/12 16:05:13, gunsch wrote:
> not lgtm
> 
> High-level drive-by: breaking the common browser/renderer interfaces makes me
> pretty sad. There are a few advantages to that structure:
> 
> (1) Allows conceptually running the entire pipeline in-process, from
CmaRenderer
> on. The CMA end-to-end test takes advantage of this.
> 
> (2) It's not tied so much to the browser/renderer pairing specifically. This
> allows a clean separation of "media code" from "content code". The
> browser/renderer code then allows content to only exist as "wiring up the
> processes" as opposed to being a critical piece of the pipeline.
> 
> If you're going to insist on separating these and tying CmaRenderer to the
> renderer process (via chromecast/renderer/), I would *strongly* object to
having
> chromecast/media/ depend on chromecast/renderer/ and would instead advise
> raising CmaRenderer to live in chromecast/renderer/media/.
> 
>
https://codereview.chromium.org/1372393007/diff/100001/chromecast/media/cma/f...
> File chromecast/media/cma/filters/DEPS (right):
> 
>
https://codereview.chromium.org/1372393007/diff/100001/chromecast/media/cma/f...
> chromecast/media/cma/filters/DEPS:2: "+chromecast/renderer/media",
> definitely no
> 
>
https://codereview.chromium.org/1372393007/diff/100001/chromecast/media/cma/f...
> File chromecast/media/cma/filters/cma_renderer.cc (right):
> 
>
https://codereview.chromium.org/1372393007/diff/100001/chromecast/media/cma/f...
> chromecast/media/cma/filters/cma_renderer.cc:19: #include
> "chromecast/renderer/media/audio_pipeline_proxy.h"
> no no no no no

I moved the cma_renderer.cc and friends into renderer/media.

[gunsch, 2015-10-13 23:26:39]

Thanks for clearing up the DEPS.

You should update the description of this CL. (add [Chromecast], fix the TODOs).

We discussed offline about removing the CMA end to end test. I still object
enough to maintain my Not-LG but I won't block this (based on offline
discussion). But you'll need to get Luke to approve :)

[halliwell, 2015-10-14 15:02:13]

On 2015/10/13 23:26:39, gunsch wrote:
> Thanks for clearing up the DEPS.
> 
> You should update the description of this CL. (add [Chromecast], fix the
TODOs).
> 
> We discussed offline about removing the CMA end to end test. I still object
> enough to maintain my Not-LG but I won't block this (based on offline
> discussion). But you'll need to get Luke to approve :)

Overall lgtm % nits and updating CL description.

Looking at the CMA end to end test again, it's hard to feel too sad about it. 
That said, I suspect CMA unit test coverage may be poor overall - please file a
ticket to address this shortly, I can help out with that.

[halliwell, 2015-10-14 15:02:37]

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

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream.cc:136:
backend_buffer_.reset(new CastDecoderBufferImpl(decoder_buffer));
This would be slightly cleaner if we added a no-args constructor for
CastDecoderBufferImpl, then we could just initialize it in the constructor
(possibly hold it by value not scoped_ptr), and this chunk of code would just
call set_buffer every time.

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

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream_unittest.cc:100: unsigned
pushed_buffer_count_;
nit: just use int (see
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types)

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/cma/b...
File chromecast/media/cma/backend/audio_video_pipeline_device_unittest.cc
(right):

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/cma/b...
chromecast/media/cma/backend/audio_video_pipeline_device_unittest.cc:252:
backend_audio_buffer_.reset(new CastDecoderBufferImpl(buffer));
same as other file: a no-args constructor for CastDecoderBufferImpl would allow
getting rid of this conditional.

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/cma/b...
chromecast/media/cma/backend/audio_video_pipeline_device_unittest.cc:286:
backend_video_buffer_.reset(new CastDecoderBufferImpl(buffer));
ditto

https://codereview.chromium.org/1372393007/diff/160001/chromecast/renderer/me...
File chromecast/renderer/media/hole_frame_factory.h (right):

https://codereview.chromium.org/1372393007/diff/160001/chromecast/renderer/me...
chromecast/renderer/media/hole_frame_factory.h:6: #define
CHROMECAST_MEDIA_CMA_FILTERS_HOLE_FRAME_FACTORY_H_
nit: can you check all the moved files include guards?

[halliwell, 2015-10-14 15:15:49]

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

[commit-bot: I haz the power, 2015-10-14 15:16:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372393007/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372393007/160001

[commit-bot: I haz the power, 2015-10-14 15:24:50]

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

[commit-bot: I haz the power, 2015-10-14 15:24:51]

Dry run: Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)

[gunsch, 2015-10-14 16:48:43]

Note: cma_end_to_end_test was written as a regression test for b/19868692. It
tests an actual failure that we've had in the past. I realize it looks sort of
silly without that context.

[kmackay, 2015-10-14 21:27:26]

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

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream.cc:136:
backend_buffer_.reset(new CastDecoderBufferImpl(decoder_buffer));
On 2015/10/14 15:02:37, halliwell wrote:
> This would be slightly cleaner if we added a no-args constructor for
> CastDecoderBufferImpl, then we could just initialize it in the constructor
> (possibly hold it by value not scoped_ptr), and this chunk of code would just
> call set_buffer every time.

Done.

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

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream_unittest.cc:100: unsigned
pushed_buffer_count_;
On 2015/10/14 15:02:37, halliwell wrote:
> nit: just use int (see
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types)

Done.

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/cma/b...
File chromecast/media/cma/backend/audio_video_pipeline_device_unittest.cc
(right):

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/cma/b...
chromecast/media/cma/backend/audio_video_pipeline_device_unittest.cc:252:
backend_audio_buffer_.reset(new CastDecoderBufferImpl(buffer));
On 2015/10/14 15:02:37, halliwell wrote:
> same as other file: a no-args constructor for CastDecoderBufferImpl would
allow
> getting rid of this conditional.

Done.

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/cma/b...
chromecast/media/cma/backend/audio_video_pipeline_device_unittest.cc:286:
backend_video_buffer_.reset(new CastDecoderBufferImpl(buffer));
On 2015/10/14 15:02:37, halliwell wrote:
> ditto

Done.

https://codereview.chromium.org/1372393007/diff/160001/chromecast/renderer/me...
File chromecast/renderer/media/hole_frame_factory.h (right):

https://codereview.chromium.org/1372393007/diff/160001/chromecast/renderer/me...
chromecast/renderer/media/hole_frame_factory.h:6: #define
CHROMECAST_MEDIA_CMA_FILTERS_HOLE_FRAME_FACTORY_H_
On 2015/10/14 15:02:37, halliwell wrote:
> nit: can you check all the moved files include guards?

Done.

[kmackay, 2015-10-14 21:27:46]

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

[commit-bot: I haz the power, 2015-10-14 21:28:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372393007/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372393007/180001

[commit-bot: I haz the power, 2015-10-14 21:28:46]

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

[commit-bot: I haz the power, 2015-10-14 21:28:47]

Dry run: A disapproval has been posted by following reviewers:
gunsch@chromium.org.
Please make sure to get positive review before another CQ attempt.

[halliwell, 2015-10-14 21:30:51]

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

[commit-bot: I haz the power, 2015-10-14 21:33:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372393007/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372393007/180001

[commit-bot: I haz the power, 2015-10-14 21:57:02]

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

[commit-bot: I haz the power, 2015-10-14 21:57:03]

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

[kmackay, 2015-10-14 22:27:29]

kmackay@chromium.org changed reviewers:
+ sievers@chromium.org
- gunsch@chromium.org

[halliwell, 2015-10-14 22:31:43]

On 2015/10/14 22:27:30, kmackay wrote:

@sievers: we're moving some Chromecast code to a different dir and it has gpu/
in its DEPS

[gunsch, 2015-10-14 22:41:04]

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

[commit-bot: I haz the power, 2015-10-14 22:41:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372393007/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372393007/200001

[gunsch, 2015-10-14 22:41:32]

reluctant lgtm

[commit-bot: I haz the power, 2015-10-14 22:57:02]

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

[commit-bot: I haz the power, 2015-10-14 22:57:03]

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

[alokp, 2015-10-14 23:16:53]

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

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream.cc:112: backend_->Resume();
Is it legal to call Start on a backend in error state?

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream.cc:120: first_start_ = false;
move this to Start?

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream.cc:127:
DCHECK(completion_cb_.is_null());
Shouldn't this be !is_null?

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream.cc:139:
decoder_->PushBuffer(nullptr, backend_buffer_.get());
either document what nullptr is or use a variable

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

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream_unittest.cc:83:
delegate_->OnPushBufferComplete(this,
this is not the right place to call the delegate. This function just sets the
pipeline status so that it can take action on the next pushbuffer call.

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream_unittest.cc:119: VideoDecoder*
CreateVideoDecoder() override { return nullptr; }
add NOTREACHED

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream_unittest.cc:242: if (!backend)
nit: return backend ? backend->decoder() : nullptr;

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream_unittest.cc:441:
EXPECT_EQ(backend->state(), FakeMediaPipelineBackend::kStateStopped);
This should be EXPECT_EQ(expectation, actual_value). Reverse the order here and
everywhere in this file.

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream_unittest.cc:522: // (have to
post because this directly calls buffer complete)
yeah you probably should not call buffer complete.

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream_unittest.cc:600:
EXPECT_TRUE(audio_device);
can we also check the state?

[kmackay, 2015-10-14 23:48:54]

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

[commit-bot: I haz the power, 2015-10-14 23:50:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372393007/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372393007/200001

[commit-bot: I haz the power, 2015-10-15 00:01:30]

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

[commit-bot: I haz the power, 2015-10-15 00:01:31]

Dry run: 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...)

[kmackay, 2015-10-15 00:35:54]

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

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream.cc:112: backend_->Resume();
On 2015/10/14 23:16:52, Alok Priyadarshi wrote:
> Is it legal to call Start on a backend in error state?

I don't see how the backend could be in an error state when Start() is called.
Anyway, we don't guarantee that no more methods will be called when the backend
goes into an error state.

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream.cc:120: first_start_ = false;
On 2015/10/14 23:16:52, Alok Priyadarshi wrote:
> move this to Start?

Done.

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream.cc:127:
DCHECK(completion_cb_.is_null());
On 2015/10/14 23:16:52, Alok Priyadarshi wrote:
> Shouldn't this be !is_null?

No, this is checking the member variable to make sure that there isn't already a
pending push in progress. I added a DCHECK to make sure that the incoming
completion_cb argument is non-null.

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream.cc:139:
decoder_->PushBuffer(nullptr, backend_buffer_.get());
On 2015/10/14 23:16:52, Alok Priyadarshi wrote:
> either document what nullptr is or use a variable

Done.

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

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream_unittest.cc:83:
delegate_->OnPushBufferComplete(this,
On 2015/10/14 23:16:52, Alok Priyadarshi wrote:
> this is not the right place to call the delegate. This function just sets the
> pipeline status so that it can take action on the next pushbuffer call.

It seems reasonable to me. Setting the pipeline state from busy->ok unblocks any
pending push. I made this more explicit by adding a pending_push_ member.

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream_unittest.cc:119: VideoDecoder*
CreateVideoDecoder() override { return nullptr; }
On 2015/10/14 23:16:52, Alok Priyadarshi wrote:
> add NOTREACHED

Done.

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream_unittest.cc:242: if (!backend)
On 2015/10/14 23:16:52, Alok Priyadarshi wrote:
> nit: return backend ? backend->decoder() : nullptr;

Done.

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream_unittest.cc:441:
EXPECT_EQ(backend->state(), FakeMediaPipelineBackend::kStateStopped);
On 2015/10/14 23:16:52, Alok Priyadarshi wrote:
> This should be EXPECT_EQ(expectation, actual_value). Reverse the order here
and
> everywhere in this file.

Done.

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream_unittest.cc:522: // (have to
post because this directly calls buffer complete)
On 2015/10/14 23:16:52, Alok Priyadarshi wrote:
> yeah you probably should not call buffer complete.

see comment above.

https://codereview.chromium.org/1372393007/diff/160001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream_unittest.cc:600:
EXPECT_TRUE(audio_device);
On 2015/10/14 23:16:52, Alok Priyadarshi wrote:
> can we also check the state?

Done.

[kmackay, 2015-10-15 00:36:08]

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

[commit-bot: I haz the power, 2015-10-15 00:37:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372393007/170094
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372393007/170094

[commit-bot: I haz the power, 2015-10-15 00:49:23]

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

[commit-bot: I haz the power, 2015-10-15 00:49:25]

Dry run: 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...)

[no sievers, 2015-10-15 17:27:40]

chromecast/renderer/media/DEPS lgtm

[kmackay, 2015-10-15 18:04:33]

The CQ bit was checked by kmackay@chromium.org

[kmackay, 2015-10-15 18:04:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
halliwell@chromium.org, gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1372393007/#ps170094
(title: "Address alokp@ comments")

[commit-bot: I haz the power, 2015-10-15 18:05:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372393007/170094
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372393007/170094

[commit-bot: I haz the power, 2015-10-15 18:11:59]

Committed patchset #12 (id:170094)

[commit-bot: I haz the power, 2015-10-15 18:12:55]

Patchset 12 (id:??) landed as
https://crrev.com/13e7a879298533b9fb111495c8d7dd1b954aae03
Cr-Commit-Position: refs/heads/master@{#354307}