Chromecast: Cma should has_audio/video after initialization finishes.

Chromecast: Cma should has_audio/video after initialization finishes.

R=lcwu@chromium.org,servolk@chromium.org,damienv@chromium.org
BUG=internal b/19868692

Committed: https://crrev.com/23b6c79d53147e704074c624bc6326bef4933ba4
Cr-Commit-Position: refs/heads/master@{#329949}

[gunsch, 2015-05-13 06:55:58]

Note to Sergey: I'm still unable to reproduce the original crash myself. I did
ten runs both with and without this change using Sergey's suggested URL (#16 in
b/19868692).

That said, I think this is a safe/good change for a few reasons:

1) Setting has_audio/has_video too early means Stop can be sent from the
renderer side even if initialization fails on the browser side. This fixes that
case.
2) While setting the flags is moved to be later in the initialization process,
it still occurs well before init_cb_ is called (which is what declares
initialization "finished").
3) Chromium appears to have no call sites to HasAudio(), and only one call site
to HasVideo(), which is in Pipeline::StopTask. So this change only affects
internal CmaRenderer logic, not external Chromium interaction with CmaRenderer.

[gunsch, 2015-05-13 06:59:33]

On 2015/05/13 06:55:58, gunsch wrote:
> Note to Sergey: I'm still unable to reproduce the original crash myself. I did
> ten runs both with and without this change using Sergey's suggested URL (#16
in
> b/19868692).
> 
> That said, I think this is a safe/good change for a few reasons:
> 
> 1) Setting has_audio/has_video too early means Stop can be sent from the
> renderer side even if initialization fails on the browser side. This fixes
that
> case.
> 2) While setting the flags is moved to be later in the initialization process,
> it still occurs well before init_cb_ is called (which is what declares
> initialization "finished").
> 3) Chromium appears to have no call sites to HasAudio(), and only one call
site
> to HasVideo(), which is in Pipeline::StopTask. So this change only affects
> internal CmaRenderer logic, not external Chromium interaction with
CmaRenderer.

One more note: I spent some time writing a unit test for CmaRenderer---we don't
have any currently and it's a bit tricky to find the right edges at which to
mock out. Ideally we could mock out at the IPC level using IPC::TestSink and
crafting custom messages to send back, so that only the "render-process" pieces
are participating. However, wiring up the shared-memory implementation is
proving tricky: there needs to be a MediaMessageFifo instance on both "sides" of
the IPC message, and since each one takes a scoped_ptr<base::SharedMemory>
that's assumed to be pointing to the same shared memory, teardown of the test
gets ugly. I'm open to suggestions but will keep poking around, because
CmaRenderer doesn't have any coverage right now.

[gunsch, 2015-05-13 16:06:35]

Mailed out some initial test coverage:
https://codereview.chromium.org/1137263002/

Whichever CL gets submitted first, I'll add a test case for this case in the
other CL :)

[servolk, 2015-05-13 22:15:13]

On 2015/05/13 06:55:58, gunsch wrote:
> Note to Sergey: I'm still unable to reproduce the original crash myself. I did
> ten runs both with and without this change using Sergey's suggested URL (#16
in
> b/19868692).
> 
> That said, I think this is a safe/good change for a few reasons:
> 
> 1) Setting has_audio/has_video too early means Stop can be sent from the
> renderer side even if initialization fails on the browser side. This fixes
that
> case.
> 2) While setting the flags is moved to be later in the initialization process,
> it still occurs well before init_cb_ is called (which is what declares
> initialization "finished").
> 3) Chromium appears to have no call sites to HasAudio(), and only one call
site
> to HasVideo(), which is in Pipeline::StopTask. So this change only affects
> internal CmaRenderer logic, not external Chromium interaction with
CmaRenderer.

This looks like the right thing to do, but can we also make a similar change in
MediaPipelineProxy::InitializeAudio/MediaPipelineProxy::InitializeVideo, which
set the has_audio_/has_video_ before the init is actually complete?

[gunsch, 2015-05-13 22:26:43]

On 2015/05/13 22:15:13, servolk wrote:
> On 2015/05/13 06:55:58, gunsch wrote:
> > Note to Sergey: I'm still unable to reproduce the original crash myself. I
did
> > ten runs both with and without this change using Sergey's suggested URL (#16
> in
> > b/19868692).
> > 
> > That said, I think this is a safe/good change for a few reasons:
> > 
> > 1) Setting has_audio/has_video too early means Stop can be sent from the
> > renderer side even if initialization fails on the browser side. This fixes
> that
> > case.
> > 2) While setting the flags is moved to be later in the initialization
process,
> > it still occurs well before init_cb_ is called (which is what declares
> > initialization "finished").
> > 3) Chromium appears to have no call sites to HasAudio(), and only one call
> site
> > to HasVideo(), which is in Pipeline::StopTask. So this change only affects
> > internal CmaRenderer logic, not external Chromium interaction with
> CmaRenderer.
> 
> This looks like the right thing to do, but can we also make a similar change
in
> MediaPipelineProxy::InitializeAudio/MediaPipelineProxy::InitializeVideo, which
> set the has_audio_/has_video_ before the init is actually complete?

I think that's actually okay there. It seems to me that all locations in
MediaPipelineProxy that care about has_audio_/has_video_ should still queue
events to be sent over IPC as needed. That would also potentially introduce
another thread-task-posting since there isn't currently a location to do that,
so MediaPipelineProxy would have to wrap status_cb to intercept the status
result on the way back.

[gunsch, 2015-05-13 22:27:01]

On 2015/05/13 22:26:43, gunsch wrote:
> On 2015/05/13 22:15:13, servolk wrote:
> > On 2015/05/13 06:55:58, gunsch wrote:
> > > Note to Sergey: I'm still unable to reproduce the original crash myself. I
> did
> > > ten runs both with and without this change using Sergey's suggested URL
(#16
> > in
> > > b/19868692).
> > > 
> > > That said, I think this is a safe/good change for a few reasons:
> > > 
> > > 1) Setting has_audio/has_video too early means Stop can be sent from the
> > > renderer side even if initialization fails on the browser side. This fixes
> > that
> > > case.
> > > 2) While setting the flags is moved to be later in the initialization
> process,
> > > it still occurs well before init_cb_ is called (which is what declares
> > > initialization "finished").
> > > 3) Chromium appears to have no call sites to HasAudio(), and only one call
> > site
> > > to HasVideo(), which is in Pipeline::StopTask. So this change only affects
> > > internal CmaRenderer logic, not external Chromium interaction with
> > CmaRenderer.
> > 
> > This looks like the right thing to do, but can we also make a similar change
> in
> > MediaPipelineProxy::InitializeAudio/MediaPipelineProxy::InitializeVideo,
which
> > set the has_audio_/has_video_ before the init is actually complete?
> 
> I think that's actually okay there. It seems to me that all locations in
> MediaPipelineProxy that care about has_audio_/has_video_ should still queue
> events to be sent over IPC as needed. That would also potentially introduce
> another thread-task-posting since there isn't currently a location to do that,
> so MediaPipelineProxy would have to wrap status_cb to intercept the status
> result on the way back.

Also, I was able to reproduce the crash somewhat reliably now, and can confirm
that it doesn't seem to occur anymore after this fix.

[damienv1, 2015-05-13 22:49:42]

lgtm

[servolk, 2015-05-14 21:50:32]

On 2015/05/13 22:49:42, damienv1 wrote:
> lgtm

lgtm too

[gunsch, 2015-05-14 22:00:24]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2015-05-14 22:02:19]

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

[commit-bot: I haz the power, 2015-05-14 22:19:22]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-05-14 22:20:12]

Patchset 1 (id:??) landed as
https://crrev.com/23b6c79d53147e704074c624bc6326bef4933ba4
Cr-Commit-Position: refs/heads/master@{#329949}

[gunsch, 2015-06-05 21:13:40]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/1162333005/ by gunsch@chromium.org.

The reason for reverting is: This wasn't the right fix. It was reverted
downstream but not upstream..