[Chromecast] Add CmaMediaRendererFactory and IPC proxy components

Add CmaMediaRendererFactory and IPC components on render process

BUG=408189
Committed: https://crrev.com/7218ee391b4251c878f0f74df0bfaf1f5cfcd9fb
Cr-Commit-Position: refs/heads/master@{#309455}

[erickung1, 2014-12-20 08:07:54]

erickung@chromium.org changed reviewers:
+ gunsch@chromium.org, lcwu@chromium.org

[gunsch, 2014-12-20 22:18:07]

Commit message comments:
* Since Chromium commits go to everyone (build status pages, build bot
breakages, git logs, etc.), adding "Chromecast:" or "[Chromecast]" to the
beginning of the first line of commit can help provide some context
* Use BUG=408189 for this CL

Will review CL shortly

[gunsch, 2014-12-20 22:41:35]

https://codereview.chromium.org/814403002/diff/1/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/chromecast.gyp#ne...
chromecast/chromecast.gyp:179: 'renderer/media/audio_pipeline_proxy.cc',
Move these to 'cast_shell_media' below. Including CMA code will break the
Android cast_shell build.

https://codereview.chromium.org/814403002/diff/1/chromecast/common/media/cma_...
File chromecast/common/media/cma_ipc_common.h (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/common/media/cma_...
chromecast/common/media/cma_ipc_common.h:11: extern const int kNoTrackId;
Are these values more appropriate as an enum?

https://codereview.chromium.org/814403002/diff/1/chromecast/common/media/shar...
File chromecast/common/media/shared_memory_chunk.h (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/common/media/shar...
chromecast/common/media/shared_memory_chunk.h:25: virtual void* data() const
override;
remove "virtual" upstream for overridden methods; only "override"

https://codereview.chromium.org/814403002/diff/1/chromecast/common/media/shar...
chromecast/common/media/shared_memory_chunk.h:33:
DISALLOW_COPY_AND_ASSIGN(SharedMemoryChunk);
include base/macros.h for this

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/au...
File chromecast/renderer/media/audio_pipeline_proxy.cc (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/au...
chromecast/renderer/media/audio_pipeline_proxy.cc:31: void Noop() {
probably rename to something like "IgnoreResult"

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/au...
chromecast/renderer/media/audio_pipeline_proxy.cc:50: // TODO(damienv): either
send an IPC message or write a byte on the
we probably shouldn't be adding any TODO(damienv) into our code :) please check
the CL for these and either resolve or remove if they're no longer relevant

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/au...
chromecast/renderer/media/audio_pipeline_proxy.cc:82:
DISALLOW_COPY_AND_ASSIGN(AudioPipelineProxyInternal);
include base/macros.h

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/au...
chromecast/renderer/media/audio_pipeline_proxy.cc:171: #define
DEFINE_STATE_TRANSITION_1(param_fn, param_msg, param_type1) \
This #define is only used once; this would be clearer if you just unrolled it.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/au...
chromecast/renderer/media/audio_pipeline_proxy.cc:208: #define
FORWARD_ON_IO_0(param_fn) \
This #define is also only used once, just unroll it

alternately, can you combine all the #defines with ##__VA_ARGS__ ?

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/au...
File chromecast/renderer/media/audio_pipeline_proxy.h (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/au...
chromecast/renderer/media/audio_pipeline_proxy.h:33: namespace cma {
note: this should just all be chromecast::media . We should drop the old "cma"
namespace while upstreaming this code. This includes most of the new classes
added.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/au...
chromecast/renderer/media/audio_pipeline_proxy.h:76:
DISALLOW_COPY_AND_ASSIGN(AudioPipelineProxy);
include base/macros.h

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/cm...
File chromecast/renderer/media/cma_media_renderer_factory.cc (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/cm...
chromecast/renderer/media/cma_media_renderer_factory.cc:25: // TODO(erickung):
Need to provide right chromecast::media::LoadType.
Link to the crbug discussing this issue

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/cm...
chromecast/renderer/media/cma_media_renderer_factory.cc:33: // TODO(gunsch):
investigate calling SetCdm here if available.
this TODO is obsolete due to xhwang@ having wired up SetCdm correctly to the
Renderer instance

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/cm...
chromecast/renderer/media/cma_media_renderer_factory.cc:35: return
scoped_ptr<::media::Renderer>(
check this CL for "<::" ; it should always be "< ::" and there are a few
instances

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/cm...
File chromecast/renderer/media/cma_media_renderer_factory.h (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/cm...
chromecast/renderer/media/cma_media_renderer_factory.h:17: 
// ::media::RendererFactory impleentation:

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/cm...
chromecast/renderer/media/cma_media_renderer_factory.h:24:
DISALLOW_COPY_AND_ASSIGN(CmaMediaRendererFactory);
base/macros.h

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/cm...
File chromecast/renderer/media/cma_message_filter_proxy.h (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/cm...
chromecast/renderer/media/cma_message_filter_proxy.h:76: //
IPC::ChannelProxy::MessageFilter override. Called on IO thread.
// IPC::ChannelProxy::MessageFilter implementation.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/cm...
chromecast/renderer/media/cma_message_filter_proxy.h:125: };
DISALLOW_COPY_AND_ASSIGN and include base/macros.h

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
File chromecast/renderer/media/media_channel_proxy.h (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
chromecast/renderer/media/media_channel_proxy.h:18: namespace cma {
chromecast::media

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
chromecast/renderer/media/media_channel_proxy.h:22: //
remove line

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
chromecast/renderer/media/media_channel_proxy.h:35: // Return a null or negative
ID if no channel has been opened.
can't return null, it's an int

also, from the code, looks like "0" is the value for if the channel is not
opened

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
chromecast/renderer/media/media_channel_proxy.h:51: virtual
~MediaChannelProxy();
~MediaChannelProxy() override;

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
chromecast/renderer/media/media_channel_proxy.h:62:
DISALLOW_COPY_AND_ASSIGN(MediaChannelProxy);
include base/macros.h

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
File chromecast/renderer/media/media_pipeline_proxy.cc (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
chromecast/renderer/media/media_pipeline_proxy.cc:54:
DISALLOW_COPY_AND_ASSIGN(MediaPipelineProxyInternal);
base/macros.h

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
chromecast/renderer/media/media_pipeline_proxy.cc:153: #define
FORWARD_ON_IO_0(param_fn) \
same comment: can these be merged using ##__VA_ARGS__ ?

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
File chromecast/renderer/media/media_pipeline_proxy.h (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
chromecast/renderer/media/media_pipeline_proxy.h:22: namespace cma {
chromecast::media

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
chromecast/renderer/media/media_pipeline_proxy.h:35: virtual
~MediaPipelineProxy();
~MediaPipelineProxy override();

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
chromecast/renderer/media/media_pipeline_proxy.h:76:
base::WeakPtrFactory<MediaPipelineProxy> weak_factory_;
see https://code.google.com/p/chromium/issues/detail?id=303818 , WeakPtrFactory
should be last. need to fix here and then adjust ctor.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
chromecast/renderer/media/media_pipeline_proxy.h:79:
DISALLOW_COPY_AND_ASSIGN(MediaPipelineProxy);
include base/macros.h

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
File chromecast/renderer/media/video_pipeline_proxy.cc (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
chromecast/renderer/media/video_pipeline_proxy.cc:30: void Noop() {
prefer name ilke IgnoreResult

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
chromecast/renderer/media/video_pipeline_proxy.cc:49: // TODO(damienv): either
send an IPC message or write a byte on the
same comment

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
chromecast/renderer/media/video_pipeline_proxy.cc:80:
DISALLOW_COPY_AND_ASSIGN(VideoPipelineProxyInternal);
base/macros.h

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
chromecast/renderer/media/video_pipeline_proxy.cc:169: #define
DEFINE_STATE_TRANSITION_1(param_fn, param_msg, param_type1) \
only used one place, please unroll

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
chromecast/renderer/media/video_pipeline_proxy.cc:196: #define
FORWARD_ON_IO_0(param_fn) \
same comment, can these be merged

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
chromecast/renderer/media/video_pipeline_proxy.cc:215: :
io_message_loop_proxy_(io_message_loop_proxy),
indentation

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
File chromecast/renderer/media/video_pipeline_proxy.h (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
chromecast/renderer/media/video_pipeline_proxy.h:33: namespace cma {
chromecast::media

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
chromecast/renderer/media/video_pipeline_proxy.h:73:
base::WeakPtrFactory<VideoPipelineProxy> weak_factory_;
WeakPtrFactory should be last

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
chromecast/renderer/media/video_pipeline_proxy.h:76:
DISALLOW_COPY_AND_ASSIGN(VideoPipelineProxy);
base/macros.h

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
File chromecast/renderer/media/video_plane_proxy.h (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
chromecast/renderer/media/video_plane_proxy.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
actually, probably leave out VideoPlaneProxy for now in this CL.. I think Luke
is looking into how to handle the UpdateVideoSurface path, but this shouldn't be
a dependency for anything in the rest of the CL.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
chromecast/renderer/media/video_plane_proxy.h:13: namespace cma {
chromecast::media

[erickung1, 2014-12-21 11:10:48]

https://codereview.chromium.org/814403002/diff/1/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/chromecast.gyp#ne...
chromecast/chromecast.gyp:179: 'renderer/media/audio_pipeline_proxy.cc',
On 2014/12/20 22:41:33, gunsch wrote:
> Move these to 'cast_shell_media' below. Including CMA code will break the
> Android cast_shell build.

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/common/media/cma_...
File chromecast/common/media/cma_ipc_common.h (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/common/media/cma_...
chromecast/common/media/cma_ipc_common.h:11: extern const int kNoTrackId;
On 2014/12/20 22:41:33, gunsch wrote:
> Are these values more appropriate as an enum?

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/common/media/shar...
File chromecast/common/media/shared_memory_chunk.h (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/common/media/shar...
chromecast/common/media/shared_memory_chunk.h:25: virtual void* data() const
override;
On 2014/12/20 22:41:33, gunsch wrote:
> remove "virtual" upstream for overridden methods; only "override"

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/common/media/shar...
chromecast/common/media/shared_memory_chunk.h:33:
DISALLOW_COPY_AND_ASSIGN(SharedMemoryChunk);
On 2014/12/20 22:41:33, gunsch wrote:
> include base/macros.h for this

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/au...
File chromecast/renderer/media/audio_pipeline_proxy.cc (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/au...
chromecast/renderer/media/audio_pipeline_proxy.cc:31: void Noop() {
On 2014/12/20 22:41:33, gunsch wrote:
> probably rename to something like "IgnoreResult"

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/au...
chromecast/renderer/media/audio_pipeline_proxy.cc:50: // TODO(damienv): either
send an IPC message or write a byte on the
On 2014/12/20 22:41:33, gunsch wrote:
> we probably shouldn't be adding any TODO(damienv) into our code :) please
check
> the CL for these and either resolve or remove if they're no longer relevant

Change to my name first. I checked the code and it's a ACK message sending back
to browser side once the work is done. So far it's working fine without sending
other byte. I will probably leave as is for now.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/au...
chromecast/renderer/media/audio_pipeline_proxy.cc:82:
DISALLOW_COPY_AND_ASSIGN(AudioPipelineProxyInternal);
On 2014/12/20 22:41:33, gunsch wrote:
> include base/macros.h

probably don't need it since audio_pipeline_proxy.h includes base/macros.h
already?

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/au...
chromecast/renderer/media/audio_pipeline_proxy.cc:171: #define
DEFINE_STATE_TRANSITION_1(param_fn, param_msg, param_type1) \
On 2014/12/20 22:41:33, gunsch wrote:
> This #define is only used once; this would be clearer if you just unrolled it.

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/au...
chromecast/renderer/media/audio_pipeline_proxy.cc:208: #define
FORWARD_ON_IO_0(param_fn) \
On 2014/12/20 22:41:33, gunsch wrote:
> This #define is also only used once, just unroll it
> 
> alternately, can you combine all the #defines with ##__VA_ARGS__ ?

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/au...
File chromecast/renderer/media/audio_pipeline_proxy.h (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/au...
chromecast/renderer/media/audio_pipeline_proxy.h:33: namespace cma {
On 2014/12/20 22:41:33, gunsch wrote:
> note: this should just all be chromecast::media . We should drop the old "cma"
> namespace while upstreaming this code. This includes most of the new classes
> added.

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/au...
chromecast/renderer/media/audio_pipeline_proxy.h:76:
DISALLOW_COPY_AND_ASSIGN(AudioPipelineProxy);
On 2014/12/20 22:41:33, gunsch wrote:
> include base/macros.h

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/cm...
File chromecast/renderer/media/cma_media_renderer_factory.cc (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/cm...
chromecast/renderer/media/cma_media_renderer_factory.cc:25: // TODO(erickung):
Need to provide right chromecast::media::LoadType.
On 2014/12/20 22:41:33, gunsch wrote:
> Link to the crbug discussing this issue

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/cm...
chromecast/renderer/media/cma_media_renderer_factory.cc:33: // TODO(gunsch):
investigate calling SetCdm here if available.
On 2014/12/20 22:41:33, gunsch wrote:
> this TODO is obsolete due to xhwang@ having wired up SetCdm correctly to the
> Renderer instance

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/cm...
chromecast/renderer/media/cma_media_renderer_factory.cc:35: return
scoped_ptr<::media::Renderer>(
On 2014/12/20 22:41:33, gunsch wrote:
> check this CL for "<::" ; it should always be "< ::" and there are a few
> instances

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/cm...
File chromecast/renderer/media/cma_media_renderer_factory.h (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/cm...
chromecast/renderer/media/cma_media_renderer_factory.h:17: 
On 2014/12/20 22:41:33, gunsch wrote:
> // ::media::RendererFactory impleentation:

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/cm...
chromecast/renderer/media/cma_media_renderer_factory.h:24:
DISALLOW_COPY_AND_ASSIGN(CmaMediaRendererFactory);
On 2014/12/20 22:41:33, gunsch wrote:
> base/macros.h

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/cm...
File chromecast/renderer/media/cma_message_filter_proxy.h (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/cm...
chromecast/renderer/media/cma_message_filter_proxy.h:76: //
IPC::ChannelProxy::MessageFilter override. Called on IO thread.
On 2014/12/20 22:41:33, gunsch wrote:
> // IPC::ChannelProxy::MessageFilter implementation.

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/cm...
chromecast/renderer/media/cma_message_filter_proxy.h:125: };
On 2014/12/20 22:41:33, gunsch wrote:
> DISALLOW_COPY_AND_ASSIGN and include base/macros.h

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
File chromecast/renderer/media/media_channel_proxy.h (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
chromecast/renderer/media/media_channel_proxy.h:18: namespace cma {
On 2014/12/20 22:41:34, gunsch wrote:
> chromecast::media

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
chromecast/renderer/media/media_channel_proxy.h:22: //
On 2014/12/20 22:41:34, gunsch wrote:
> remove line

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
chromecast/renderer/media/media_channel_proxy.h:35: // Return a null or negative
ID if no channel has been opened.
On 2014/12/20 22:41:34, gunsch wrote:
> can't return null, it's an int
> 
> also, from the code, looks like "0" is the value for if the channel is not
> opened

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
chromecast/renderer/media/media_channel_proxy.h:51: virtual
~MediaChannelProxy();
On 2014/12/20 22:41:34, gunsch wrote:
> ~MediaChannelProxy() override;

This has to be virtual or not virtual(too risky) since the base class is
RefCountedThreadSafe<MediaChannelProxy>  which doesn't use virtual for
destructor

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
chromecast/renderer/media/media_channel_proxy.h:62:
DISALLOW_COPY_AND_ASSIGN(MediaChannelProxy);
On 2014/12/20 22:41:34, gunsch wrote:
> include base/macros.h

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
File chromecast/renderer/media/media_pipeline_proxy.cc (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
chromecast/renderer/media/media_pipeline_proxy.cc:54:
DISALLOW_COPY_AND_ASSIGN(MediaPipelineProxyInternal);
On 2014/12/20 22:41:34, gunsch wrote:
> base/macros.h

may not need it since media_pipeline_proxy.h already includes base/macros.h?

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
chromecast/renderer/media/media_pipeline_proxy.cc:153: #define
FORWARD_ON_IO_0(param_fn) \
On 2014/12/20 22:41:34, gunsch wrote:
> same comment: can these be merged using ##__VA_ARGS__ ?

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
File chromecast/renderer/media/media_pipeline_proxy.h (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
chromecast/renderer/media/media_pipeline_proxy.h:22: namespace cma {
On 2014/12/20 22:41:34, gunsch wrote:
> chromecast::media

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
chromecast/renderer/media/media_pipeline_proxy.h:35: virtual
~MediaPipelineProxy();
On 2014/12/20 22:41:34, gunsch wrote:
> ~MediaPipelineProxy override();

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
chromecast/renderer/media/media_pipeline_proxy.h:76:
base::WeakPtrFactory<MediaPipelineProxy> weak_factory_;
On 2014/12/20 22:41:34, gunsch wrote:
> see https://code.google.com/p/chromium/issues/detail?id=303818 ,
WeakPtrFactory
> should be last. need to fix here and then adjust ctor.

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/me...
chromecast/renderer/media/media_pipeline_proxy.h:79:
DISALLOW_COPY_AND_ASSIGN(MediaPipelineProxy);
On 2014/12/20 22:41:34, gunsch wrote:
> include base/macros.h

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
File chromecast/renderer/media/video_pipeline_proxy.cc (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
chromecast/renderer/media/video_pipeline_proxy.cc:30: void Noop() {
On 2014/12/20 22:41:34, gunsch wrote:
> prefer name ilke IgnoreResult

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
chromecast/renderer/media/video_pipeline_proxy.cc:49: // TODO(damienv): either
send an IPC message or write a byte on the
On 2014/12/20 22:41:34, gunsch wrote:
> same comment

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
chromecast/renderer/media/video_pipeline_proxy.cc:80:
DISALLOW_COPY_AND_ASSIGN(VideoPipelineProxyInternal);
On 2014/12/20 22:41:34, gunsch wrote:
> base/macros.h
video_pipeline_proxy.h has included base/macros.h already.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
chromecast/renderer/media/video_pipeline_proxy.cc:169: #define
DEFINE_STATE_TRANSITION_1(param_fn, param_msg, param_type1) \
On 2014/12/20 22:41:34, gunsch wrote:
> only used one place, please unroll

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
chromecast/renderer/media/video_pipeline_proxy.cc:196: #define
FORWARD_ON_IO_0(param_fn) \
On 2014/12/20 22:41:34, gunsch wrote:
> same comment, can these be merged

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
chromecast/renderer/media/video_pipeline_proxy.cc:215: :
io_message_loop_proxy_(io_message_loop_proxy),
On 2014/12/20 22:41:34, gunsch wrote:
> indentation

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
File chromecast/renderer/media/video_pipeline_proxy.h (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
chromecast/renderer/media/video_pipeline_proxy.h:33: namespace cma {
On 2014/12/20 22:41:34, gunsch wrote:
> chromecast::media

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
chromecast/renderer/media/video_pipeline_proxy.h:73:
base::WeakPtrFactory<VideoPipelineProxy> weak_factory_;
On 2014/12/20 22:41:34, gunsch wrote:
> WeakPtrFactory should be last

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
chromecast/renderer/media/video_pipeline_proxy.h:76:
DISALLOW_COPY_AND_ASSIGN(VideoPipelineProxy);
On 2014/12/20 22:41:34, gunsch wrote:
> base/macros.h

Done.

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
File chromecast/renderer/media/video_plane_proxy.h (right):

https://codereview.chromium.org/814403002/diff/1/chromecast/renderer/media/vi...
chromecast/renderer/media/video_plane_proxy.h:13: namespace cma {
On 2014/12/20 22:41:35, gunsch wrote:
> chromecast::media

Done.

[gunsch, 2014-12-21 22:17:22]

lgtm % nits

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

https://codereview.chromium.org/814403002/diff/20001/chromecast/renderer/medi...
chromecast/renderer/media/cma_media_renderer_factory.cc:22:
scoped_ptr<::media::Renderer> CmaMediaRendererFactory::CreateRenderer(
"<::"  --> "< ::"

https://codereview.chromium.org/814403002/diff/20001/chromecast/renderer/medi...
File chromecast/renderer/media/cma_media_renderer_factory.h (right):

https://codereview.chromium.org/814403002/diff/20001/chromecast/renderer/medi...
chromecast/renderer/media/cma_media_renderer_factory.h:20:
scoped_ptr<::media::Renderer> CreateRenderer(
"<::" --> "< ::"

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

https://codereview.chromium.org/814403002/diff/20001/chromecast/renderer/medi...
chromecast/renderer/media/media_pipeline_proxy.cc:21: // A macro runs current
member function on |io_message_loop_proxy_| thread.
note: this should probably be moved back to after MediaPipelineProxyInternal,
where it was before (this file and others).

Since it can only be used after it's #defined, that prevents
MediaPipelineProxyInternal from inadvertently trying to use it.

[erickung1, 2014-12-22 01:41:37]

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

https://codereview.chromium.org/814403002/diff/20001/chromecast/renderer/medi...
chromecast/renderer/media/cma_media_renderer_factory.cc:22:
scoped_ptr<::media::Renderer> CmaMediaRendererFactory::CreateRenderer(
On 2014/12/21 22:17:22, gunsch wrote:
> "<::"  --> "< ::"

Done.

https://codereview.chromium.org/814403002/diff/20001/chromecast/renderer/medi...
File chromecast/renderer/media/cma_media_renderer_factory.h (right):

https://codereview.chromium.org/814403002/diff/20001/chromecast/renderer/medi...
chromecast/renderer/media/cma_media_renderer_factory.h:20:
scoped_ptr<::media::Renderer> CreateRenderer(
On 2014/12/21 22:17:22, gunsch wrote:
> "<::" --> "< ::"

Done.

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

https://codereview.chromium.org/814403002/diff/20001/chromecast/renderer/medi...
chromecast/renderer/media/media_pipeline_proxy.cc:21: // A macro runs current
member function on |io_message_loop_proxy_| thread.
On 2014/12/21 22:17:22, gunsch wrote:
> note: this should probably be moved back to after MediaPipelineProxyInternal,
> where it was before (this file and others).
> 
> Since it can only be used after it's #defined, that prevents
> MediaPipelineProxyInternal from inadvertently trying to use it.

Done.

[erickung1, 2014-12-22 01:41:51]

The CQ bit was checked by erickung@chromium.org

[commit-bot: I haz the power, 2014-12-22 01:42:05]

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

[commit-bot: I haz the power, 2014-12-22 01:48:29]

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

[commit-bot: I haz the power, 2014-12-22 01:48:30]

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

[gunsch, 2014-12-22 01:51:30]

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

[gunsch, 2014-12-22 01:52:07]

On 2014/12/22 01:48:30, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

@tsepez: can you review IPC message change at
https://codereview.chromium.org/814403002/diff/40001/chromecast/common/media/...
? It's just changing an int to a newly defined enum at
https://codereview.chromium.org/814403002/diff/40001/chromecast/common/media/...
.

[Tom Sepez, 2014-12-22 20:16:37]

Messages LGTM.

[erickung1, 2014-12-22 20:21:37]

The CQ bit was checked by erickung@chromium.org

[commit-bot: I haz the power, 2014-12-22 20:22:48]

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

[commit-bot: I haz the power, 2014-12-22 20:30:53]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-22 20:32:11]

Patchset 3 (id:??) landed as
https://crrev.com/7218ee391b4251c878f0f74df0bfaf1f5cfcd9fb
Cr-Commit-Position: refs/heads/master@{#309455}