Get CdmContext in MojoAudioDecoderService

Get CdmContext in MojoAudioDecoderService

This CL assumes that CDM has already been registered
in MojoCDMService by the time of audio decoder service
initialization.

BUG=542910
Committed: https://crrev.com/5ee3f242cefebba7eb0f05424e7308f9158d8388
Cr-Commit-Position: refs/heads/master@{#382459}

[Tima Vaisburd, 2016-03-21 20:40:56]

timav@chromium.org changed reviewers:
+ xhwang@chromium.org

[Tima Vaisburd, 2016-03-21 20:40:57]

There is a preliminary CL, could you, please, answer a question?

https://codereview.chromium.org/1824763002/diff/1/media/mojo/services/mojo_au...
File media/mojo/services/mojo_audio_decoder_service.cc (right):

https://codereview.chromium.org/1824763002/diff/1/media/mojo/services/mojo_au...
media/mojo/services/mojo_audio_decoder_service.cc:47: scoped_refptr<MediaKeys>
cdm = mojo_cdm_service_context_->GetCdm(cdm_id);
[Q] Will this call succeed? Do we have cdm_id already registered?

[xhwang, 2016-03-21 20:50:49]

Reply to question only.

https://codereview.chromium.org/1824763002/diff/1/media/mojo/services/mojo_au...
File media/mojo/services/mojo_audio_decoder_service.cc (right):

https://codereview.chromium.org/1824763002/diff/1/media/mojo/services/mojo_au...
media/mojo/services/mojo_audio_decoder_service.cc:47: scoped_refptr<MediaKeys>
cdm = mojo_cdm_service_context_->GetCdm(cdm_id);
On 2016/03/21 20:40:57, Tima Vaisburd wrote:
> [Q] Will this call succeed? Do we have cdm_id already registered?

This will work. But the check in line 55 will fail. I am fixing that right now.
See https://chromiumcodereview.appspot.com/1808983002/

[xhwang, 2016-03-21 20:55:58]

Looking good. I have one comment.

https://codereview.chromium.org/1824763002/diff/1/media/mojo/services/mojo_au...
File media/mojo/services/mojo_audio_decoder_service.cc (right):

https://codereview.chromium.org/1824763002/diff/1/media/mojo/services/mojo_au...
media/mojo/services/mojo_audio_decoder_service.cc:47: scoped_refptr<MediaKeys>
cdm = mojo_cdm_service_context_->GetCdm(cdm_id);
When initialization is successful, you should hold a reference to the |cdm| so
avoid life time issues. See:
https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/service...

[Tima Vaisburd, 2016-03-21 21:53:01]

https://codereview.chromium.org/1824763002/diff/1/media/mojo/services/mojo_au...
File media/mojo/services/mojo_audio_decoder_service.cc (right):

https://codereview.chromium.org/1824763002/diff/1/media/mojo/services/mojo_au...
media/mojo/services/mojo_audio_decoder_service.cc:47: scoped_refptr<MediaKeys>
cdm = mojo_cdm_service_context_->GetCdm(cdm_id);
On 2016/03/21 20:55:58, xhwang wrote:
> When initialization is successful, you should hold a reference to the |cdm| so
> avoid life time issues. See:
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/service...

Done, see my question though.

https://codereview.chromium.org/1824763002/diff/20001/media/mojo/services/moj...
File media/mojo/services/mojo_audio_decoder_service.cc (right):

https://codereview.chromium.org/1824763002/diff/20001/media/mojo/services/moj...
media/mojo/services/mojo_audio_decoder_service.cc:61: cdm_ = cdm;
I can bind |cdm| to OnInitialized() instead, but do we need to hold it for
decoder_->Initialize() as well?

[xhwang, 2016-03-21 22:31:23]

https://codereview.chromium.org/1824763002/diff/20001/media/mojo/services/moj...
File media/mojo/services/mojo_audio_decoder_service.cc (right):

https://codereview.chromium.org/1824763002/diff/20001/media/mojo/services/moj...
media/mojo/services/mojo_audio_decoder_service.cc:61: cdm_ = cdm;
On 2016/03/21 21:53:01, Tima Vaisburd wrote:
> I can bind |cdm| to OnInitialized() instead, but do we need to hold it for
> decoder_->Initialize() as well?

Either way is fine. When you bind |cdm| into the callback, the callback takes a
reference. The reference will only be dropped when the callback is fired and we
drop the |cdm| in OnInitialized() (instead saving it), or when the callback is
destroyed, e.g. when message loop is destructing.

[Tima Vaisburd, 2016-03-21 23:07:05]

https://codereview.chromium.org/1824763002/diff/20001/media/mojo/services/moj...
File media/mojo/services/mojo_audio_decoder_service.cc (right):

https://codereview.chromium.org/1824763002/diff/20001/media/mojo/services/moj...
media/mojo/services/mojo_audio_decoder_service.cc:61: cdm_ = cdm;
On 2016/03/21 22:31:23, xhwang wrote:
> On 2016/03/21 21:53:01, Tima Vaisburd wrote:
> > I can bind |cdm| to OnInitialized() instead, but do we need to hold it for
> > decoder_->Initialize() as well?
> 
> Either way is fine. When you bind |cdm| into the callback, the callback takes
a
> reference. The reference will only be dropped when the callback is fired and
we
> drop the |cdm| in OnInitialized() (instead saving it), or when the callback is
> destroyed, e.g. when message loop is destructing.

That you, I missed the fact that Bind() will hold the reference. Using Bind
seems more elegant, I did that.

[xhwang, 2016-03-21 23:13:03]

https://codereview.chromium.org/1824763002/diff/20001/media/mojo/services/moj...
File media/mojo/services/mojo_audio_decoder_service.cc (right):

https://codereview.chromium.org/1824763002/diff/20001/media/mojo/services/moj...
media/mojo/services/mojo_audio_decoder_service.cc:61: cdm_ = cdm;
On 2016/03/21 23:07:05, Tima Vaisburd wrote:
> On 2016/03/21 22:31:23, xhwang wrote:
> > On 2016/03/21 21:53:01, Tima Vaisburd wrote:
> > > I can bind |cdm| to OnInitialized() instead, but do we need to hold it for
> > > decoder_->Initialize() as well?
> > 
> > Either way is fine. When you bind |cdm| into the callback, the callback
takes
> a
> > reference. The reference will only be dropped when the callback is fired and
> we
> > drop the |cdm| in OnInitialized() (instead saving it), or when the callback
is
> > destroyed, e.g. when message loop is destructing.
> 
> That you, I missed the fact that Bind() will hold the reference. Using Bind
> seems more elegant, I did that.

Yes, this often results in more elegant code. But it has it's own drawbacks. For
example, if the task (bound callback) is posted to a different thread, and
somehow message loop destruction happens, the callback and all it's bound
parameters will be destroyed on that thread. If you have BindToCurrentLoop() and
you expect the callback to always be posted back because the bound parameter is
not thread safe, this will cause trouble. Just FYI :)

In this case we are fine because we are all on the same thread.

[xhwang, 2016-03-21 23:14:24]

lgtm

[Tima Vaisburd, 2016-03-21 23:58:44]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2016-03-21 23:58:44]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1824763002/#ps60001
(title: "Rebased, fixed typo in a comment")

[commit-bot: I haz the power, 2016-03-21 23:59:10]

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

[Tima Vaisburd, 2016-03-22 00:15:49]

timav@chromium.org changed reviewers:
+ sandersd@chromium.org

[Tima Vaisburd, 2016-03-22 00:15:50]

+sandersd@: forgot to add you on this.

[commit-bot: I haz the power, 2016-03-22 00:37:26]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-22 00:38:44]

Description was changed from

==========
Get CdmContext in MojoAudioDecoderService

This CL assumes that CDM has already been registered
in MojoCDMService by the time of audio decoder service
initialization.

BUG=542910
==========

to

==========
Get CdmContext in MojoAudioDecoderService

This CL assumes that CDM has already been registered
in MojoCDMService by the time of audio decoder service
initialization.

BUG=542910

Committed: https://crrev.com/5ee3f242cefebba7eb0f05424e7308f9158d8388
Cr-Commit-Position: refs/heads/master@{#382459}
==========

[commit-bot: I haz the power, 2016-03-22 00:38:45]

Patchset 4 (id:??) landed as
https://crrev.com/5ee3f242cefebba7eb0f05424e7308f9158d8388
Cr-Commit-Position: refs/heads/master@{#382459}