Prepare MediaDrmBridge to work with MediaCodecPlayer

Prepare MediaDrmBridge to work with MediaCodecPlayer

This CL is a prerequisite for MediaCodecPlayer stage 7 (DRM)
https://codereview.chromium.org/1344133002/

If the MediaCodecPlayer we post the callback that the MediaCrypto
is ready and all PlayerTracker related callbacks to the media
thread. We also delete the MediaDrmBridge on the media thread.

As a result we create the MediaCrypto object on UI thread
and pass it to the MediaCodec.configure() on media thread.

BUG=407577
Committed: https://crrev.com/bb1063aada4db17a35d6825a3be5ec464bc54593
Cr-Commit-Position: refs/heads/master@{#351588}

[Tima Vaisburd, 2015-09-14 20:42:34]

Min,

This is the first draft. Looks too big again. Do you think I need to split it?
It seems I can make MediaCodecPlayer-only changes in one patch (SetCdm does not
do anything) and the rest in another patch?

Thank you for taking a look.

https://codereview.chromium.org/1341883003/diff/1/media/base/android/media_dr...
File media/base/android/media_drm_proxy.h (right):

https://codereview.chromium.org/1341883003/diff/1/media/base/android/media_dr...
media/base/android/media_drm_proxy.h:79: // Internal callbacks
It is possible to relay the callbacks inside the MediaDrmProxy, but it seems not
needed since we do UI thread (Java callback) -> Media thread (DrmBridge) -> UI
thread (DrmProxy). I uploaded them in commented form in case we need them back,
but in the next patch I will clean it up.

https://codereview.chromium.org/1341883003/diff/20001/media/base/cdm_promise_...
File media/base/cdm_promise_adapter.h (right):

https://codereview.chromium.org/1341883003/diff/20001/media/base/cdm_promise_...
media/base/cdm_promise_adapter.h:55: base::Lock lock_;
I need to recall why I need this lock. It would be nice to remove it.

https://codereview.chromium.org/1341883003/diff/20001/media/cdm/player_tracke...
File media/cdm/player_tracker_impl.h (right):

https://codereview.chromium.org/1341883003/diff/20001/media/cdm/player_tracke...
media/cdm/player_tracker_impl.h:52: base::Lock lock_;
The lock is here because RegisterPlayer() has the sync interface: it returns the
new ID. I did not know how to work around this but this that protecting a map
with a lock in this class is acceptable.

[Tima Vaisburd, 2015-09-15 00:34:53]

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

[Tima Vaisburd, 2015-09-15 20:12:11]

Did the split

[qinmin, 2015-09-15 21:10:45]

https://codereview.chromium.org/1341883003/diff/80001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/1341883003/diff/80001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:30: #define
POST_ON_UI_THREAD(METHOD, ...)                                      \
s/METHOD/method/

https://codereview.chromium.org/1341883003/diff/80001/media/base/android/medi...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1341883003/diff/80001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:33: #define RUN_CB_ON_MEDIA_THREAD(CB,
...)                              \
s/CB/cb/

https://codereview.chromium.org/1341883003/diff/80001/media/base/android/medi...
File media/base/android/media_drm_proxy.cc (right):

https://codereview.chromium.org/1341883003/diff/80001/media/base/android/medi...
media/base/android/media_drm_proxy.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
s/2013/2015/

https://codereview.chromium.org/1341883003/diff/80001/media/base/android/medi...
media/base/android/media_drm_proxy.cc:9: 
no blank line here

https://codereview.chromium.org/1341883003/diff/80001/media/base/android/medi...
media/base/android/media_drm_proxy.cc:13: 
ditto

https://codereview.chromium.org/1341883003/diff/80001/media/base/android/medi...
media/base/android/media_drm_proxy.cc:16: #define RUN_ON_MEDIA_THREAD(METHOD,
...)                                  \
s/METHOD/method/

https://codereview.chromium.org/1341883003/diff/80001/media/base/android/medi...
media/base/android/media_drm_proxy.cc:16: #define RUN_ON_MEDIA_THREAD(METHOD,
...)                                  \
I would rename this to RUN_ON_MEDIA_THREAD_AND_RETURN so people reading the code
below know that things following RUN_ON_MEDIA_THREAD might not get executed.

https://codereview.chromium.org/1341883003/diff/80001/media/base/android/medi...
media/base/android/media_drm_proxy.cc:19:
/*DCHECK(ui_task_runner_->BelongsToCurrentThread());*/              \
remove this, it is implied by the above if statement.

https://codereview.chromium.org/1341883003/diff/80001/media/base/android/medi...
File media/base/android/media_drm_proxy.h (right):

https://codereview.chromium.org/1341883003/diff/80001/media/base/android/medi...
media/base/android/media_drm_proxy.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
s/2013/2015/

https://codereview.chromium.org/1341883003/diff/80001/media/cdm/player_tracke...
File media/cdm/player_tracker_impl.cc (right):

https://codereview.chromium.org/1341883003/diff/80001/media/cdm/player_tracke...
media/cdm/player_tracker_impl.cc:30: base::AutoLock lock(lock_);
what is this lock trying to guard against? all these 4 methods here are called
by MediaDrmBridge, they should be on the same thread? no?

[Tima Vaisburd, 2015-09-15 23:48:56]

https://codereview.chromium.org/1341883003/diff/80001/content/browser/media/c...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/1341883003/diff/80001/content/browser/media/c...
content/browser/media/cdm/browser_cdm_manager.cc:30: #define
POST_ON_UI_THREAD(METHOD, ...)                                      \
On 2015/09/15 21:10:44, qinmin wrote:
> s/METHOD/method/

Done.

https://codereview.chromium.org/1341883003/diff/80001/media/base/android/medi...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1341883003/diff/80001/media/base/android/medi...
media/base/android/media_drm_bridge.cc:33: #define RUN_CB_ON_MEDIA_THREAD(CB,
...)                              \
On 2015/09/15 21:10:44, qinmin wrote:
> s/CB/cb/

Done.

https://codereview.chromium.org/1341883003/diff/80001/media/base/android/medi...
File media/base/android/media_drm_proxy.cc (right):

https://codereview.chromium.org/1341883003/diff/80001/media/base/android/medi...
media/base/android/media_drm_proxy.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2015/09/15 21:10:44, qinmin wrote:
> s/2013/2015/

Done.

https://codereview.chromium.org/1341883003/diff/80001/media/base/android/medi...
media/base/android/media_drm_proxy.cc:9: 
On 2015/09/15 21:10:44, qinmin wrote:
> no blank line here

Done.

https://codereview.chromium.org/1341883003/diff/80001/media/base/android/medi...
media/base/android/media_drm_proxy.cc:13: 
On 2015/09/15 21:10:45, qinmin wrote:
> ditto

Done.

https://codereview.chromium.org/1341883003/diff/80001/media/base/android/medi...
media/base/android/media_drm_proxy.cc:16: #define RUN_ON_MEDIA_THREAD(METHOD,
...)                                  \
On 2015/09/15 21:10:44, qinmin wrote:
> s/METHOD/method/

Done.

https://codereview.chromium.org/1341883003/diff/80001/media/base/android/medi...
media/base/android/media_drm_proxy.cc:16: #define RUN_ON_MEDIA_THREAD(METHOD,
...)                                  \
On 2015/09/15 21:10:45, qinmin wrote:
> I would rename this to RUN_ON_MEDIA_THREAD_AND_RETURN so people reading the
code
> below know that things following RUN_ON_MEDIA_THREAD might not get executed.

I understood the meaning to be "make this method executed on media thread". At
least for me "run and return" would be a little confusing, I renamed to
POST_ON_MEDIA_THREAD_AND_RETURN.

https://codereview.chromium.org/1341883003/diff/80001/media/base/android/medi...
media/base/android/media_drm_proxy.cc:19:
/*DCHECK(ui_task_runner_->BelongsToCurrentThread());*/              \
On 2015/09/15 21:10:45, qinmin wrote:
> remove this, it is implied by the above if statement.

Done.

https://codereview.chromium.org/1341883003/diff/80001/media/cdm/player_tracke...
File media/cdm/player_tracker_impl.cc (right):

https://codereview.chromium.org/1341883003/diff/80001/media/cdm/player_tracke...
media/cdm/player_tracker_impl.cc:30: base::AutoLock lock(lock_);
On 2015/09/15 21:10:45, qinmin wrote:
> what is this lock trying to guard against? all these 4 methods here are called
> by MediaDrmBridge, they should be on the same thread? no?

Yes, the MediaDrmBridge calls them on the same media thread. I added the lock
because I thought I need to implement the MediaDrmProxy methods RegisterPlayer()
and UnregisterPlayer(), they are part of the interface that we return. But it
seems they are never called ?

https://codereview.chromium.org/1341883003/diff/120001/media/base/android/med...
File media/base/android/media_drm_proxy.h (right):

https://codereview.chromium.org/1341883003/diff/120001/media/base/android/med...
media/base/android/media_drm_proxy.h:78: // Internal callbacks.
The re-introduction of internal callbacks fixes the issue of no keys because it
restored the order of messages we send to the renderer process.
I could, however, added the lock into media/base/cdm_promise_adapter.h and keep
all callbacks on the UI thread. Please advise.

[Tima Vaisburd, 2015-09-16 01:26:29]

https://codereview.chromium.org/1341883003/diff/80001/media/cdm/player_tracke...
File media/cdm/player_tracker_impl.cc (right):

https://codereview.chromium.org/1341883003/diff/80001/media/cdm/player_tracke...
media/cdm/player_tracker_impl.cc:30: base::AutoLock lock(lock_);
On 2015/09/15 23:48:56, Tima Vaisburd wrote:
> On 2015/09/15 21:10:45, qinmin wrote:
> > what is this lock trying to guard against? all these 4 methods here are
called
> > by MediaDrmBridge, they should be on the same thread? no?
> 
> Yes, the MediaDrmBridge calls them on the same media thread. I added the lock
> because I thought I need to implement the MediaDrmProxy methods
RegisterPlayer()
> and UnregisterPlayer(), they are part of the interface that we return. But it
> seems they are never called ?

Removed the lock.

[qinmin, 2015-09-16 18:09:40]

lgtm % comments

https://codereview.chromium.org/1341883003/diff/140001/content/browser/media/...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/1341883003/diff/140001/content/browser/media/...
content/browser/media/cdm/browser_cdm_manager.cc:30: #define
POST_ON_UI_THREAD(method, ...)                                      \
nit: maybe RUN_ON_UI_THREAD is more appropriate since if the task is already
running on UI thread, no post will happen

https://codereview.chromium.org/1341883003/diff/140001/media/base/android/med...
File media/base/android/media_drm_proxy.h (right):

https://codereview.chromium.org/1341883003/diff/140001/media/base/android/med...
media/base/android/media_drm_proxy.h:110: std::string key_system_;
const

https://codereview.chromium.org/1341883003/diff/140001/media/base/android/med...
media/base/android/media_drm_proxy.h:113: MediaDrmBridge::SecurityLevel
widevine_security_level_;
const

[Tima Vaisburd, 2015-09-16 18:37:21]

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

[Tima Vaisburd, 2015-09-16 18:37:22]

https://codereview.chromium.org/1341883003/diff/140001/media/base/android/med...
File media/base/android/media_drm_proxy.h (right):

https://codereview.chromium.org/1341883003/diff/140001/media/base/android/med...
media/base/android/media_drm_proxy.h:110: std::string key_system_;
On 2015/09/16 18:09:40, qinmin wrote:
> const

Done.

https://codereview.chromium.org/1341883003/diff/140001/media/base/android/med...
media/base/android/media_drm_proxy.h:113: MediaDrmBridge::SecurityLevel
widevine_security_level_;
On 2015/09/16 18:09:40, qinmin wrote:
> const

Done.

[ddorwin, 2015-09-16 20:42:21]

Some initial comments.

https://codereview.chromium.org/1341883003/diff/160001/content/browser/media/...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/1341883003/diff/160001/content/browser/media/...
content/browser/media/cdm/browser_cdm_manager.cc:34:
manager_->method(__VA_ARGS__);                                      \
I think we should avoid using members in a macro, which is generally discouraged
anyway. It can be passed in.

https://codereview.chromium.org/1341883003/diff/160001/content/browser/media/...
content/browser/media/cdm/browser_cdm_manager.cc:92: #if defined(OS_ANDROID)
Why does this only apply to Android? This is a lot of #ifdefs.

Instead, can we always call RUN_ON_CORRECT_<FOO>_THREAD and #ifdef that
definition? to do the right thing? I think this also depends on the answer to my
question below.

https://codereview.chromium.org/1341883003/diff/160001/content/browser/media/...
content/browser/media/cdm/browser_cdm_manager.cc:93:
RUN_ON_UI_THREAD(RejectPromise, render_frame_id_, cdm_id_, promise_id_,
Why must this be put on the UI thread here?
The answer might indicate a better location.

https://codereview.chromium.org/1341883003/diff/160001/media/base/android/med...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1341883003/diff/160001/media/base/android/med...
media/base/android/media_drm_bridge.cc:376: if (key_system == kWidevineKeySystem
&&
We shouldn't need the WV check here again.

https://codereview.chromium.org/1341883003/diff/160001/media/base/android/med...
media/base/android/media_drm_bridge.cc:655:
RUN_CB_ON_MEDIA_THREAD(session_closed_cb_, session_id);
Should it be up to the caller to ensure this? At what layer is the contract
about threads? Unless it's being used to ensure synchronization (is it?), it
probably makes sense to move this up the stack. WDYT?

Would using something like
https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webmed...
when the CBs are created be better?

https://codereview.chromium.org/1341883003/diff/160001/media/base/android/med...
File media/base/android/media_drm_proxy.h (right):

https://codereview.chromium.org/1341883003/diff/160001/media/base/android/med...
media/base/android/media_drm_proxy.h:17: // This class delegates MediaDrmBridge
calls onto the Media thread
This is a lot of duplicate code and extra function calls. Is there a better way
to do this? Could this be handled within the BrowserCdm implementation?

https://codereview.chromium.org/1341883003/diff/160001/media/base/android/med...
media/base/android/media_drm_proxy.h:18: class MEDIA_EXPORT MediaDrmProxy :
public BrowserCdm {
"Proxy" isn't very descriptive. (We've regretted using that term in the past.)
Is there a better name?

[Tima Vaisburd, 2015-09-18 00:41:38]

https://codereview.chromium.org/1341883003/diff/160001/media/base/android/med...
File media/base/android/media_drm_proxy.h (right):

https://codereview.chromium.org/1341883003/diff/160001/media/base/android/med...
media/base/android/media_drm_proxy.h:17: // This class delegates MediaDrmBridge
calls onto the Media thread
On 2015/09/16 20:42:21, ddorwin wrote:
> This is a lot of duplicate code and extra function calls. Is there a better
way
> to do this? Could this be handled within the BrowserCdm implementation?

I tried a different approach, where only the callbacks necessary for the
MediaCodecPlayer are reposted to the Media thread. The MediaDrmBridge itself is
deleted on the Media thread, too. The code became simpler, there is no need for
a proxy object and there is no thread hopping for other callbacks. I updated the
CL description.

This change might address most of your concerns.

https://codereview.chromium.org/1341883003/diff/200001/media/base/android/med...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1341883003/diff/200001/media/base/android/med...
media/base/android/media_drm_bridge.cc:316: // TODO(timav): need a better way.
I will try to eliminate this lock in the next patch. The plan is to take the
media crypto with the callback and not call GetMediaCrypto at all.

[Tima Vaisburd, 2015-09-19 04:48:43]

In the Patch Set 12 I tried to ensure that GetMediaCrypto() is called on the UI
thread only, the same thread is is created. To do this I pass the MediaCrypto
object with the callback. In the MediaCodecPlayer (a separate CL which is
updated correspondingly) I use this passed MediaCrypto object. I assume it is
not changed during whole CDM session.

Still, I had to introduce a mutex that protects the callback variable, it is
accessed by two threads.

Please take a look.

[Tima Vaisburd, 2015-09-21 21:36:09]

On 2015/09/19 04:48:43, Tima Vaisburd wrote:
> In the Patch Set 12 I tried to ensure that GetMediaCrypto() is called on the
UI
> thread only, the same thread is is created. To do this I pass the MediaCrypto
> object with the callback. In the MediaCodecPlayer (a separate CL which is
> updated correspondingly) I use this passed MediaCrypto object. I assume it is
> not changed during whole CDM session.
> 
> Still, I had to introduce a mutex that protects the callback variable, it is
> accessed by two threads.
> 
> Please take a look.

Patch set 13 includes a simplification corresponding to
https://codereview.chromium.org/1344133002/
patch set 5.

[Tima Vaisburd, 2015-09-22 19:38:25]

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

[Tima Vaisburd, 2015-09-22 19:41:50]

timav@chromium.org changed reviewers:
+ xhwang@chromium.org
- xhwang@google.com

[qinmin, 2015-09-25 05:16:34]

https://codereview.chromium.org/1341883003/diff/260001/media/base/android/med...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1341883003/diff/260001/media/base/android/med...
media/base/android/media_drm_bridge.cc:292: use_media_thread_(false),
no need for this, since it will be overwritten in the ctor.

https://codereview.chromium.org/1341883003/diff/260001/media/base/android/med...
media/base/android/media_drm_bridge.cc:300: base::WeakPtr<MediaDrmBridge>
weak_this =
assignment of weak ptr with the weak ptr hasn't even been binded, is this
working?

[Tima Vaisburd, 2015-09-25 17:47:17]

https://codereview.chromium.org/1341883003/diff/260001/media/base/android/med...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1341883003/diff/260001/media/base/android/med...
media/base/android/media_drm_bridge.cc:292: use_media_thread_(false),
On 2015/09/25 05:16:34, qinmin wrote:
> no need for this, since it will be overwritten in the ctor.

I plan to submit the finch CL first, then I can do
use_media_thread_(UseMediaThread()) in the initialization list.

https://codereview.chromium.org/1341883003/diff/260001/media/base/android/med...
media/base/android/media_drm_bridge.cc:300: base::WeakPtr<MediaDrmBridge>
weak_this =
On 2015/09/25 05:16:34, qinmin wrote:
> assignment of weak ptr with the weak ptr hasn't even been binded, is this
> working?

I tested with both old and new player.

I think this usage is similar to
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...,
what do I miss?

[qinmin, 2015-09-25 17:59:41]

https://codereview.chromium.org/1341883003/diff/260001/media/base/android/med...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1341883003/diff/260001/media/base/android/med...
media/base/android/media_drm_bridge.cc:300: base::WeakPtr<MediaDrmBridge>
weak_this =
On 2015/09/25 17:47:17, Tima Vaisburd wrote:
> On 2015/09/25 05:16:34, qinmin wrote:
> > assignment of weak ptr with the weak ptr hasn't even been binded, is this
> > working?
> 
> I tested with both old and new player.
> 
> I think this usage is similar to
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...,
> what do I miss?

So the weak_this is never used anywhere except in the ctor, when the
media_weak_factory is never bounded to the media thread, and I don't even see
where media_weak_factory is ever used in this file other than the ctor

[Tima Vaisburd, 2015-09-25 18:27:56]

https://codereview.chromium.org/1341883003/diff/260001/media/base/android/med...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1341883003/diff/260001/media/base/android/med...
media/base/android/media_drm_bridge.cc:300: base::WeakPtr<MediaDrmBridge>
weak_this =
On 2015/09/25 17:59:41, qinmin wrote:
> On 2015/09/25 17:47:17, Tima Vaisburd wrote:
> > On 2015/09/25 05:16:34, qinmin wrote:
> > > assignment of weak ptr with the weak ptr hasn't even been binded, is this
> > > working?
> > 
> > I tested with both old and new player.
> > 
> > I think this usage is similar to
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...,
> > what do I miss?
> 
> So the weak_this is never used anywhere except in the ctor, when the
> media_weak_factory is never bounded to the media thread, and I don't even see
> where media_weak_factory is ever used in this file other than the ctor

As far as I understand, the weak_ptr becomes bound to the
internal_keys_added_cb_ on l.304. Then this callback is either posted from UI to
the Media thread on l.672 -- if use_media_thread_ == true ,  -- or invoked
directly via internal_keys_added_cb_.Run().

In the first case we need a weak pointer for the media thread and I use the
media_weak_factory_. This is where I wanted media_weak_factory to be used.

I seems that in the second case I do not need a weak pointer and can use an
unretained pointer to construct the callback.

[Tima Vaisburd, 2015-09-25 21:22:58]

https://codereview.chromium.org/1341883003/diff/260001/media/base/android/med...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1341883003/diff/260001/media/base/android/med...
media/base/android/media_drm_bridge.cc:292: use_media_thread_(false),
On 2015/09/25 17:47:17, Tima Vaisburd wrote:
> On 2015/09/25 05:16:34, qinmin wrote:
> > no need for this, since it will be overwritten in the ctor.
> 
> I plan to submit the finch CL first, then I can do
> use_media_thread_(UseMediaThread()) in the initialization list.

Done this.

https://codereview.chromium.org/1341883003/diff/260001/media/base/android/med...
media/base/android/media_drm_bridge.cc:300: base::WeakPtr<MediaDrmBridge>
weak_this =
On 2015/09/25 18:27:56, Tima Vaisburd wrote:
> On 2015/09/25 17:59:41, qinmin wrote:
> > On 2015/09/25 17:47:17, Tima Vaisburd wrote:
> > > On 2015/09/25 05:16:34, qinmin wrote:
> > > > assignment of weak ptr with the weak ptr hasn't even been binded, is
this
> > > > working?
> > > 
> > > I tested with both old and new player.
> > > 
> > > I think this usage is similar to
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...,
> > > what do I miss?
> > 
> > So the weak_this is never used anywhere except in the ctor, when the
> > media_weak_factory is never bounded to the media thread, and I don't even
see
> > where media_weak_factory is ever used in this file other than the ctor
> 
> As far as I understand, the weak_ptr becomes bound to the
> internal_keys_added_cb_ on l.304. Then this callback is either posted from UI
to
> the Media thread on l.672 -- if use_media_thread_ == true ,  -- or invoked
> directly via internal_keys_added_cb_.Run().
> 
> In the first case we need a weak pointer for the media thread and I use the
> media_weak_factory_. This is where I wanted media_weak_factory to be used.
> 
> I seems that in the second case I do not need a weak pointer and can use an
> unretained pointer to construct the callback.

Removed the macro and hopefully simplified, PTAL

[qinmin, 2015-09-26 00:40:32]

https://codereview.chromium.org/1341883003/diff/280001/media/base/android/med...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1341883003/diff/280001/media/base/android/med...
media/base/android/media_drm_bridge.cc:27: #include
"media/base/android/media_codec_player.h"  // for GetMediaTaskRunner()
MediaDrmBridge shouldn't depend on MediaCodecPlayer. GetMediaTaskRunner need to
be be put into a separate file to be included by both

https://codereview.chromium.org/1341883003/diff/280001/media/base/android/med...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/1341883003/diff/280001/media/base/android/med...
media/base/android/media_drm_bridge.h:202: base::WeakPtr<MediaDrmBridge>
WeakPtrForUIThread();
is this ever being used?

[qinmin, 2015-09-26 00:51:40]

lgtm % nits

[Tima Vaisburd, 2015-09-26 00:53:29]

https://codereview.chromium.org/1341883003/diff/280001/media/base/android/med...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1341883003/diff/280001/media/base/android/med...
media/base/android/media_drm_bridge.cc:27: #include
"media/base/android/media_codec_player.h"  // for GetMediaTaskRunner()
On 2015/09/26 00:40:32, qinmin wrote:
> MediaDrmBridge shouldn't depend on MediaCodecPlayer. GetMediaTaskRunner need
to
> be be put into a separate file to be included by both

Will do. It would probably make sense to put UseMediaThread() there as well?

https://codereview.chromium.org/1341883003/diff/280001/media/base/android/med...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/1341883003/diff/280001/media/base/android/med...
media/base/android/media_drm_bridge.h:202: base::WeakPtr<MediaDrmBridge>
WeakPtrForUIThread();
On 2015/09/26 00:40:32, qinmin wrote:
> is this ever being used?

Only in one place, in MediaCodecPlayer::SetCdm() in
https://codereview.chromium.org/1344133002/

For that matter, would it be better to move the changes in this file into that
CL?

[Tima Vaisburd, 2015-09-28 20:17:37]

https://codereview.chromium.org/1341883003/diff/280001/media/base/android/med...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1341883003/diff/280001/media/base/android/med...
media/base/android/media_drm_bridge.cc:27: #include
"media/base/android/media_codec_player.h"  // for GetMediaTaskRunner()
On 2015/09/26 00:53:28, Tima Vaisburd wrote:
> On 2015/09/26 00:40:32, qinmin wrote:
> > MediaDrmBridge shouldn't depend on MediaCodecPlayer. GetMediaTaskRunner need
> to
> > be be put into a separate file to be included by both
> 
> Will do. It would probably make sense to put UseMediaThread() there as well?

Added new files media_task_runner (.h and *.cc), put GetMediaTaskRunner() and
UseMediaThreadForMediaCallback() there. Please let me know if you have a better
idea of the naming.

[xhwang, 2015-09-29 17:28:24]

Sorry again for the delay. This is the first day I am back to work. Also, my
comments went across multiple PSs; sorry for the mess. If old comments are not
valid anymore feel free to ignore them.

The CL looks pretty good. Most comments are nits.

https://codereview.chromium.org/1341883003/diff/220001/media/base/android/med...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1341883003/diff/220001/media/base/android/med...
media/base/android/media_drm_bridge.cc:51: using
base::android::ScopedJavaGlobalRef;
nit: order

https://codereview.chromium.org/1341883003/diff/220001/media/base/android/med...
media/base/android/media_drm_bridge.cc:328: DVLOG(1) << "MediaDrmBridge::" <<
__FUNCTION__;
nit: in media code (and the rest of this file) we don't usually include the
class name in the log. You'll see the file name in the logs anyway.

https://codereview.chromium.org/1341883003/diff/280001/content/browser/media/...
File content/browser/media/android/media_drm_credential_manager.h (right):

https://codereview.chromium.org/1341883003/diff/280001/content/browser/media/...
content/browser/media/android/media_drm_credential_manager.h:55:
scoped_ptr<media::MediaDrmBridge, media::BrowserCdmDeleter> media_drm_bridge_;
Use BrowserCdmPtr?

https://codereview.chromium.org/1341883003/diff/280001/media/base/android/bro...
File media/base/android/browser_cdm_factory_android.cc (right):

https://codereview.chromium.org/1341883003/diff/280001/media/base/android/bro...
media/base/android/browser_cdm_factory_android.cc:29: scoped_ptr<MediaDrmBridge,
BrowserCdmDeleter> cdm(
BrowserCdmPtr?

https://codereview.chromium.org/1341883003/diff/280001/media/base/android/med...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1341883003/diff/280001/media/base/android/med...
media/base/android/media_drm_bridge.cc:319: scoped_ptr<MediaDrmBridge,
BrowserCdmDeleter> MediaDrmBridge::Create(
nit: here and throughout this file, use BrowserCdmPtr?

https://codereview.chromium.org/1341883003/diff/280001/media/base/android/med...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/1341883003/diff/280001/media/base/android/med...
media/base/android/media_drm_bridge.h:76: static scoped_ptr<MediaDrmBridge,
BrowserCdmDeleter> Create(
nit: here and throughout this file, use MediaDrmPtr?

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
media/base/android/media_drm_bridge.cc:38: using
base::android::ScopedJavaGlobalRef;
nit: order

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
media/base/android/media_drm_bridge.cc:299: Java_MediaDrmBridge_destroy(env,
j_media_drm_.obj());
nit: Could you please add a comment here that after this line Java will not call
into native MDB methods.

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
media/base/android/media_drm_bridge.cc:504: void
MediaDrmBridge::SetMediaCryptoReadyCB(const MediaCryptoReadyCB& closure) {
Can you let the caller (e.g. MediaSourcePlayer) decide which thread this
callback should be fired on? You can so by using something like
BindToCurrentLoop() [1].

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/bind_to...

With that, MediaDrmBridge shouldn't worry about how to post this |closure|.

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
media/base/android/media_drm_bridge.h:47: bool needs_protected_surface)>
MediaCryptoReadyCB;
nit: the new trend is to use "using" instead of "typedef" :)

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
media/base/android/media_drm_bridge.h:131: void SetMediaCryptoReadyCB(const
MediaCryptoReadyCB& closure);
nit: give |closure| a proper name, then also update the comment.

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
media/base/android/media_drm_bridge.h:241: PlayerTrackerImpl player_tracker_;
nit: add a comment that this must be accessed from the media thread if
|use_media_thread_| is true?

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
media/base/android/media_drm_bridge.h:249: bool use_media_thread_;
nit: const

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
media/base/android/media_drm_bridge.h:253: base::WeakPtrFactory<MediaDrmBridge>
ui_weak_factory_;
note to my self: how to ensure thread safety?

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
media/base/android/media_source_player.cc:267:
drm_bridge_->SetMediaCryptoReadyCB(MediaDrmBridge::MediaCryptoReadyCB());
As mentioned above, you can bind the callback to the correct thread here.

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
File media/base/android/media_task_runner.h (right):

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
media/base/android/media_task_runner.h:14: // Returns the task runner for the
media thread
tiny nit: add period at end

https://codereview.chromium.org/1341883003/diff/300001/media/base/browser_cdm.h
File media/base/browser_cdm.h (right):

https://codereview.chromium.org/1341883003/diff/300001/media/base/browser_cdm...
media/base/browser_cdm.h:36: typedef scoped_ptr<BrowserCdm, BrowserCdmDeleter>
BrowserCdmPtr;
nit: "using" is more popular now..

https://codereview.chromium.org/1341883003/diff/300001/media/cdm/player_track...
File media/cdm/player_tracker_impl.cc (right):

https://codereview.chromium.org/1341883003/diff/300001/media/cdm/player_track...
media/cdm/player_tracker_impl.cc:23: thread_checker_.DetachFromThread();
nit: add a comment why we need this?

[xhwang, 2015-09-29 17:28:56]

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
media/base/android/media_drm_bridge.h:253: base::WeakPtrFactory<MediaDrmBridge>
ui_weak_factory_;
On 2015/09/29 17:28:23, xhwang wrote:
> note to my self: how to ensure thread safety?

oops, please ignore this one :)

[Tima Vaisburd, 2015-09-29 19:55:44]

Xiaohan, may I ask you to read my comments while I figure out how to bind a
callback to a thread?

https://codereview.chromium.org/1341883003/diff/220001/media/base/android/med...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1341883003/diff/220001/media/base/android/med...
media/base/android/media_drm_bridge.cc:51: using
base::android::ScopedJavaGlobalRef;
On 2015/09/29 17:28:23, xhwang wrote:
> nit: order

Done.

https://codereview.chromium.org/1341883003/diff/220001/media/base/android/med...
media/base/android/media_drm_bridge.cc:328: DVLOG(1) << "MediaDrmBridge::" <<
__FUNCTION__;
On 2015/09/29 17:28:23, xhwang wrote:
> nit: in media code (and the rest of this file) we don't usually include the
> class name in the log. You'll see the file name in the logs anyway.

Class name removed.

https://codereview.chromium.org/1341883003/diff/280001/content/browser/media/...
File content/browser/media/android/media_drm_credential_manager.h (right):

https://codereview.chromium.org/1341883003/diff/280001/content/browser/media/...
content/browser/media/android/media_drm_credential_manager.h:55:
scoped_ptr<media::MediaDrmBridge, media::BrowserCdmDeleter> media_drm_bridge_;
On 2015/09/29 17:28:23, xhwang wrote:
> Use BrowserCdmPtr?

This seems like the only place that I can do this (see my other comments).
ResetCredentialsInternal() calls media_drm_bridge_->SetSecurityLevel() and
media_drm_bridge_->ResetDeviceCredentials() that are not part of BrowserCdm
interface. But it happens within one method so I can keep a base class pointer
here.

However I'm not sure that this particular change is worth it. Please take
another look.

https://codereview.chromium.org/1341883003/diff/280001/media/base/android/bro...
File media/base/android/browser_cdm_factory_android.cc (right):

https://codereview.chromium.org/1341883003/diff/280001/media/base/android/bro...
media/base/android/browser_cdm_factory_android.cc:29: scoped_ptr<MediaDrmBridge,
BrowserCdmDeleter> cdm(
On 2015/09/29 17:28:23, xhwang wrote:
> BrowserCdmPtr?

I though I cannot do it here because on l.42 it calls SetSecurityLevel() which
is not part of BrowserCdm interface ?

https://codereview.chromium.org/1341883003/diff/280001/media/base/android/med...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1341883003/diff/280001/media/base/android/med...
media/base/android/media_drm_bridge.cc:319: scoped_ptr<MediaDrmBridge,
BrowserCdmDeleter> MediaDrmBridge::Create(
On 2015/09/29 17:28:23, xhwang wrote:
> nit: here and throughout this file, use BrowserCdmPtr?

The methods that call Create() and the following CreateWithoutSessionSupport()
later use the methods of MediaDrmBridge that do not exist in BrowserCdm. See my
previous comment.

https://codereview.chromium.org/1341883003/diff/280001/media/base/android/med...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/1341883003/diff/280001/media/base/android/med...
media/base/android/media_drm_bridge.h:76: static scoped_ptr<MediaDrmBridge,
BrowserCdmDeleter> Create(
On 2015/09/29 17:28:23, xhwang wrote:
> nit: here and throughout this file, use MediaDrmPtr?

I guess s/MediaDrmPtr/BrowserCdmPtr/

I think I can't, the caller wants to call SetSecurityLevel() on this pointer.
See the prior comment.

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
media/base/android/media_drm_bridge.cc:38: using
base::android::ScopedJavaGlobalRef;
On 2015/09/29 17:28:23, xhwang wrote:
> nit: order

Done.

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
media/base/android/media_drm_bridge.cc:299: Java_MediaDrmBridge_destroy(env,
j_media_drm_.obj());
On 2015/09/29 17:28:23, xhwang wrote:
> nit: Could you please add a comment here that after this line Java will not
call
> into native MDB methods.

Done.

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
media/base/android/media_drm_bridge.h:47: bool needs_protected_surface)>
MediaCryptoReadyCB;
On 2015/09/29 17:28:24, xhwang wrote:
> nit: the new trend is to use "using" instead of "typedef" :)

Done.

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
media/base/android/media_drm_bridge.h:131: void SetMediaCryptoReadyCB(const
MediaCryptoReadyCB& closure);
On 2015/09/29 17:28:23, xhwang wrote:
> nit: give |closure| a proper name, then also update the comment.

Done.

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
media/base/android/media_drm_bridge.h:241: PlayerTrackerImpl player_tracker_;
On 2015/09/29 17:28:24, xhwang wrote:
> nit: add a comment that this must be accessed from the media thread if
> |use_media_thread_| is true?

Done.

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
media/base/android/media_drm_bridge.h:249: bool use_media_thread_;
On 2015/09/29 17:28:24, xhwang wrote:
> nit: const

Done.

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
File media/base/android/media_task_runner.h (right):

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
media/base/android/media_task_runner.h:14: // Returns the task runner for the
media thread
On 2015/09/29 17:28:24, xhwang wrote:
> tiny nit: add period at end

Done.

https://codereview.chromium.org/1341883003/diff/300001/media/base/browser_cdm.h
File media/base/browser_cdm.h (right):

https://codereview.chromium.org/1341883003/diff/300001/media/base/browser_cdm...
media/base/browser_cdm.h:36: typedef scoped_ptr<BrowserCdm, BrowserCdmDeleter>
BrowserCdmPtr;
On 2015/09/29 17:28:24, xhwang wrote:
> nit: "using" is more popular now..

Done.

https://codereview.chromium.org/1341883003/diff/300001/media/cdm/player_track...
File media/cdm/player_tracker_impl.cc (right):

https://codereview.chromium.org/1341883003/diff/300001/media/cdm/player_track...
media/cdm/player_tracker_impl.cc:23: thread_checker_.DetachFromThread();
On 2015/09/29 17:28:24, xhwang wrote:
> nit: add a comment why we need this?

Done.

[xhwang, 2015-09-29 21:04:24]

https://codereview.chromium.org/1341883003/diff/320001/content/browser/media/...
File content/browser/media/android/media_drm_credential_manager.h (right):

https://codereview.chromium.org/1341883003/diff/320001/content/browser/media/...
content/browser/media/android/media_drm_credential_manager.h:55:
media::BrowserCdmPtr media_drm_bridge_;
oops, I totally missed the point. I thought this is a scoped_ptr of BrowserCdm
and didn't realize it's a scoped_ptr of MediaDrmBridge. You can probably define

using ScopedMediaDrmBridgePtr = scoped_ptr<media::MediaDrmBridge,
media::BrowserCdmDeleter>;

in media_drm_bridge.h and then use ScopedMediaDrmBridgePtr everywhere you need
it.

With that, I think the old code looks better (to avoid an extra cast). Sorry for
the confusion.

https://codereview.chromium.org/1341883003/diff/320001/media/base/browser_cdm.h
File media/base/browser_cdm.h (right):

https://codereview.chromium.org/1341883003/diff/320001/media/base/browser_cdm...
media/base/browser_cdm.h:36: using BrowserCdmPtr = scoped_ptr<BrowserCdm,
BrowserCdmDeleter>;
I prefer ScopedBrowserCdmPtr, which is more clear. WDYT?

[Tima Vaisburd, 2015-09-29 21:51:41]

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1341883003/diff/300001/media/base/android/med...
media/base/android/media_drm_bridge.cc:504: void
MediaDrmBridge::SetMediaCryptoReadyCB(const MediaCryptoReadyCB& closure) {
On 2015/09/29 17:28:23, xhwang wrote:
> Can you let the caller (e.g. MediaSourcePlayer) decide which thread this
> callback should be fired on? You can so by using something like
> BindToCurrentLoop() [1].
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/bind_to...
> 
> With that, MediaDrmBridge shouldn't worry about how to post this |closure|.

Done.

https://codereview.chromium.org/1341883003/diff/320001/content/browser/media/...
File content/browser/media/android/media_drm_credential_manager.h (right):

https://codereview.chromium.org/1341883003/diff/320001/content/browser/media/...
content/browser/media/android/media_drm_credential_manager.h:55:
media::BrowserCdmPtr media_drm_bridge_;
On 2015/09/29 21:04:24, xhwang wrote:
> oops, I totally missed the point. I thought this is a scoped_ptr of BrowserCdm
> and didn't realize it's a scoped_ptr of MediaDrmBridge. You can probably
define
> 
> using ScopedMediaDrmBridgePtr = scoped_ptr<media::MediaDrmBridge,
> media::BrowserCdmDeleter>;
> 
> in media_drm_bridge.h and then use ScopedMediaDrmBridgePtr everywhere you need
> it.
> 
> With that, I think the old code looks better (to avoid an extra cast). Sorry
for
> the confusion.

Done.

https://codereview.chromium.org/1341883003/diff/320001/media/base/browser_cdm.h
File media/base/browser_cdm.h (right):

https://codereview.chromium.org/1341883003/diff/320001/media/base/browser_cdm...
media/base/browser_cdm.h:36: using BrowserCdmPtr = scoped_ptr<BrowserCdm,
BrowserCdmDeleter>;
On 2015/09/29 21:04:24, xhwang wrote:
> I prefer ScopedBrowserCdmPtr, which is more clear. WDYT?

Done.

https://codereview.chromium.org/1341883003/diff/340001/media/base/android/med...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1341883003/diff/340001/media/base/android/med...
media/base/android/media_drm_bridge.cc:538:
PostMediaCryptoReady(base::ResetAndReturn(&media_crypto_ready_cb_));
With this change the new code does one extra post from UI thread to itself for
MediaSourcePlayer, I presume this is ok.

[xhwang, 2015-09-29 23:00:30]

lgtm with two extra comments.

https://codereview.chromium.org/1341883003/diff/340001/media/base/android/med...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1341883003/diff/340001/media/base/android/med...
media/base/android/media_drm_bridge.cc:541: void
MediaDrmBridge::PostMediaCryptoReady(const MediaCryptoReadyCB& cb) {
We are not "Post"ing in this function any more. So maybe something like
NotifyMediaCryptoReady()?

https://codereview.chromium.org/1341883003/diff/340001/media/base/android/med...
media/base/android/media_drm_bridge.cc:552: // |cb| is bound to a thread so this
Run() will actually post a task.
This is really an implementation detail of the caller. If the caller knows that
this will be called on the correct thread, then this doesn't necessarily involve
a post. See comment below.

https://codereview.chromium.org/1341883003/diff/340001/media/base/android/med...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/1341883003/diff/340001/media/base/android/med...
media/base/android/media_source_player.cc:302:
drm_bridge_->SetMediaCryptoReadyCB(cb);
MSP is still running on the UI thread, right? In that case, can we skip the BTCL
here?

[Tima Vaisburd, 2015-09-29 23:17:39]

https://codereview.chromium.org/1341883003/diff/340001/media/base/android/med...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/1341883003/diff/340001/media/base/android/med...
media/base/android/media_source_player.cc:302:
drm_bridge_->SetMediaCryptoReadyCB(cb);
On 2015/09/29 23:00:30, xhwang wrote:
> MSP is still running on the UI thread, right? In that case, can we skip the
BTCL
> here?

If I won't BindToCurrentLoop() the MediaDrmBridge::SetMediaCryptoReadyCB() might
directly call OnMediaCryptoReady() which in turn calls SetMediaCryptoReadyCB()
again.

[xhwang, 2015-09-29 23:29:39]

https://codereview.chromium.org/1341883003/diff/340001/media/base/android/med...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/1341883003/diff/340001/media/base/android/med...
media/base/android/media_source_player.cc:302:
drm_bridge_->SetMediaCryptoReadyCB(cb);
On 2015/09/29 23:17:39, Tima Vaisburd wrote:
> On 2015/09/29 23:00:30, xhwang wrote:
> > MSP is still running on the UI thread, right? In that case, can we skip the
> BTCL
> > here?
> 
> If I won't BindToCurrentLoop() the MediaDrmBridge::SetMediaCryptoReadyCB()
might
> directly call OnMediaCryptoReady() which in turn calls SetMediaCryptoReadyCB()
> again.

Good catch. Let's keep this BTCL to avoid the reentrance craziness.

Meanwhile I wonder whether we can just remove the callback clearing logic at
l.268. It seems MediaDrmBridge is clearing the callback when firing it already.

[Tima Vaisburd, 2015-09-29 23:40:20]

https://codereview.chromium.org/1341883003/diff/340001/media/base/android/med...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/1341883003/diff/340001/media/base/android/med...
media/base/android/media_drm_bridge.cc:541: void
MediaDrmBridge::PostMediaCryptoReady(const MediaCryptoReadyCB& cb) {
On 2015/09/29 23:00:29, xhwang wrote:
> We are not "Post"ing in this function any more. So maybe something like
> NotifyMediaCryptoReady()?

Done.

https://codereview.chromium.org/1341883003/diff/340001/media/base/android/med...
media/base/android/media_drm_bridge.cc:552: // |cb| is bound to a thread so this
Run() will actually post a task.
On 2015/09/29 23:00:30, xhwang wrote:
> This is really an implementation detail of the caller. If the caller knows
that
> this will be called on the correct thread, then this doesn't necessarily
involve
> a post. See comment below.

Acknowledged.

[Tima Vaisburd, 2015-09-29 23:43:10]

https://codereview.chromium.org/1341883003/diff/340001/media/base/android/med...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/1341883003/diff/340001/media/base/android/med...
media/base/android/media_source_player.cc:302:
drm_bridge_->SetMediaCryptoReadyCB(cb);
On 2015/09/29 23:29:39, xhwang wrote:
> On 2015/09/29 23:17:39, Tima Vaisburd wrote:
> > On 2015/09/29 23:00:30, xhwang wrote:
> > > MSP is still running on the UI thread, right? In that case, can we skip
the
> > BTCL
> > > here?
> > 
> > If I won't BindToCurrentLoop() the MediaDrmBridge::SetMediaCryptoReadyCB()
> might
> > directly call OnMediaCryptoReady() which in turn calls
SetMediaCryptoReadyCB()
> > again.
> 
> Good catch. Let's keep this BTCL to avoid the reentrance craziness.
> 
> Meanwhile I wonder whether we can just remove the callback clearing logic at
> l.268. It seems MediaDrmBridge is clearing the callback when firing it
already.

Yes, I think it can be deleted, I did this for MediaSourcePlayer. I think it's
still better to keep BCTL to avoid reentrant calls. I'd lime to go ahead and
commit this CL, I will delete l.268 in the next CL.

[Tima Vaisburd, 2015-09-29 23:43:25]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2015-09-29 23:43:25]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1341883003/#ps380001
(title: "Renamed PostMediaCryptoReady() into NotifyMediaCryptoReady()")

[commit-bot: I haz the power, 2015-09-29 23:44:08]

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

[commit-bot: I haz the power, 2015-09-29 23:52:42]

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

[commit-bot: I haz the power, 2015-09-29 23:52:43]

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...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Tima Vaisburd, 2015-09-29 23:59:16]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2015-09-29 23:59:16]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1341883003/#ps400001
(title: "Added missing license")

[commit-bot: I haz the power, 2015-09-30 00:01:55]

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

[commit-bot: I haz the power, 2015-09-30 00:10:48]

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

[commit-bot: I haz the power, 2015-09-30 00:10:49]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Tima Vaisburd, 2015-09-30 00:18:42]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2015-09-30 00:18:42]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1341883003/#ps420001
(title: "Rebased again")

[commit-bot: I haz the power, 2015-09-30 00:21:06]

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

[commit-bot: I haz the power, 2015-09-30 00:30:18]

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

[commit-bot: I haz the power, 2015-09-30 00:30:20]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Tima Vaisburd, 2015-09-30 00:37:32]

The CQ bit was checked by timav@chromium.org

[commit-bot: I haz the power, 2015-09-30 00:38:16]

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

[commit-bot: I haz the power, 2015-09-30 01:04:07]

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

[commit-bot: I haz the power, 2015-09-30 01:04:09]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Tima Vaisburd, 2015-09-30 01:33:09]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2015-09-30 01:33:10]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1341883003/#ps440001
(title: "Potential fix of cast_shell_linux compilation")

[commit-bot: I haz the power, 2015-09-30 01:33:27]

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

[commit-bot: I haz the power, 2015-09-30 02:02:52]

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

[commit-bot: I haz the power, 2015-09-30 02:02:54]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Tima Vaisburd, 2015-09-30 05:57:10]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2015-09-30 05:57:10]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1341883003/#ps480001
(title: "Fixed MediaDrmBridgeTest unit tests")

[commit-bot: I haz the power, 2015-09-30 05:57:23]

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

[commit-bot: I haz the power, 2015-09-30 06:06:40]

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

[commit-bot: I haz the power, 2015-09-30 06:06:41]

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...)

[Tima Vaisburd, 2015-09-30 15:15:38]

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

[Tima Vaisburd, 2015-09-30 15:15:39]

Hi Le-Chun Wu,
Could you, please, review the changes that went into chromecast/ ?
Thank you.

[lcwu1, 2015-09-30 16:21:16]

chromecast/ lgtm

[Tima Vaisburd, 2015-09-30 17:22:21]

The CQ bit was checked by timav@chromium.org

[commit-bot: I haz the power, 2015-09-30 17:24:25]

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

[commit-bot: I haz the power, 2015-09-30 17:31:18]

Committed patchset #25 (id:480001)

[commit-bot: I haz the power, 2015-09-30 17:32:17]

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