Add PlayerTracker and BrowserCdm interfaces.

Add PlayerTracker and BrowserCdm interfaces.

- PlayerTracker interface can be used by any CDM that needs to track media
  players. It can notify media players of new abailable key (to resume playback)
  or the destruction of the CDM.
- PlayerTrackerImpl is a simple implementation of PlayerTracker that can help
  any CDM to implement PlayerTracker interface.
- BrowserCdm is a common CDM interface for browser side CDMs.
- CdmFactory is renamed to BrowserCdmFactory.
- Now BrowserMediaPlayer only needs to call SetCdm() on the player and do not
  need to track CDM <-> player mapping.

TBR=damienv@chromium.org
BUG=373327
TEST=Existing test pages still work.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274470

[xhwang, 2014-05-30 01:57:28]

PTAL

[damienv1, 2014-05-30 15:10:15]

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

https://codereview.chromium.org/308073004/diff/40001/media/base/browser_cdm.h...
media/base/browser_cdm.h:15: public:
We should have a function to query whether a key ID is valid.
new_key_cb does not guarantee the key that is added is the one the media
pipeline is waiting for.

https://codereview.chromium.org/308073004/diff/40001/media/base/browser_cdm_f...
File media/base/browser_cdm_factory.h (right):

https://codereview.chromium.org/308073004/diff/40001/media/base/browser_cdm_f...
media/base/browser_cdm_factory.h:11: #include "media/base/browser_cdm.h"
Foward declaration for BrowserCdm ?

https://codereview.chromium.org/308073004/diff/40001/media/base/player_tracker.h
File media/base/player_tracker.h (right):

https://codereview.chromium.org/308073004/diff/40001/media/base/player_tracke...
media/base/player_tracker.h:26: const base::Closure& cdm_destroyed_cb) = 0;
There is an implicit assumption that the CDM and the player are running on the
same thread here (which is ok to me), but this might not apply to every use
case.
By the time the notification is received on the other thread, the CDM can
already be destroyed.

[xhwang, 2014-05-30 17:18:53]

comments only

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

https://codereview.chromium.org/308073004/diff/40001/media/base/browser_cdm.h...
media/base/browser_cdm.h:15: public:
On 2014/05/30 15:10:15, damienv1 wrote:
> We should have a function to query whether a key ID is valid.
> new_key_cb does not guarantee the key that is added is the one the media
> pipeline is waiting for.

Currently no CDM provides this functionality (usable key ID) yet. So I'd rather
add that later. It'll still fit in the PlayerTracker interface. WDYT?

https://codereview.chromium.org/308073004/diff/40001/media/base/player_tracker.h
File media/base/player_tracker.h (right):

https://codereview.chromium.org/308073004/diff/40001/media/base/player_tracke...
media/base/player_tracker.h:26: const base::Closure& cdm_destroyed_cb) = 0;
On 2014/05/30 15:10:15, damienv1 wrote:
> There is an implicit assumption that the CDM and the player are running on the
> same thread here (which is ok to me), but this might not apply to every use
> case.
> By the time the notification is received on the other thread, the CDM can
> already be destroyed.

The current plan is to use this only for BrowserCdm. So this simple model will
work fine. We need to work out a long term plan for CDM/player tracking for
desktop.

[ddorwin, 2014-05-30 20:50:04]

https://codereview.chromium.org/308073004/diff/40001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/308073004/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:383:
MediaKeys::KeyError error_code,
When BrowserCdm no longer inherits from MediaKeys, we'll need a duplicate of the
error (to become Exception) enum.

https://codereview.chromium.org/308073004/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:730: const int& id
= cdm_id;
nit: I think we usually just copy the value. The compiler should handle this.

This is also odd since cdm_id could change, changing the value of id.

https://codereview.chromium.org/308073004/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:766: // Currently
we do not support detaching CDM from a player.
Is this prevented by Blink? If so, DCHECK()? If not, we're not reporting a
failure.

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
media/base/android/media_drm_bridge.cc:427: player_tracker_.NotifyNewKey();
Just FYI: This will not be called here in the future.

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
media/base/android/media_drm_bridge.h:104: const base::Closure&
cdm_destroyed_cb) OVERRIDE;
UPDATE: Looking at the next file, this might need to handle the case when the
CDM disappears (i.e. the platform closes it). I still think "destroyed" is an
implementation detail of what the player should care about.

destroyed or disassociated?

It shouldn't be destroyed while associated should it? Or is this related to the
disconnection process where it may still be decrypting data until the threads
all stop. Even in that case, it seems like disconnection rather than destroyed,
which is really irrelevant (the player just needs to stop using it).

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
File media/base/android/media_player_android.cc (right):

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
media/base/android/media_player_android.cc:34: // Not all players support CDMs.
Do nothing by default.
Shouldn't this either be impossible (DCHECK) or cause an error for the app?

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
media/base/android/media_source_player.cc:70:
drm_bridge_->UnregisterPlayer(cdm_registration_id_);
Should this be called even if cdm_registration_id_ == 0?
I believe it must be non-0 iff drm_bridge_ is not null. In that case, maybe
these calls should be grouped.

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
media/base/android/media_source_player.cc:363: // Currently we don't support
resetting |drm_bridge_|.
Ditto about DCHECK and error reporting.

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
media/base/android/media_source_player.cc:370: cdm_registration_id_ =
cdm->RegisterPlayer(
A bit odd to cast then use the previous pointer (cdm). Note that Unregister is
called on the member.

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
media/base/android/media_source_player.cc:374: if
(drm_bridge_->GetMediaCrypto().is_null()) {
Should this logic be made generic as well? Some type of OnInitializedCB?

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
media/base/android/media_source_player.cc:1035: // TODO(xhwang): Support
detachment of CDM.
Don't you need to handle the platform-driven case (sleep) now?

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

https://codereview.chromium.org/308073004/diff/40001/media/base/browser_cdm.h...
media/base/browser_cdm.h:16: BrowserCdm();
protected or just remove.

https://codereview.chromium.org/308073004/diff/40001/media/base/browser_cdm_f...
File media/base/browser_cdm_factory.h (right):

https://codereview.chromium.org/308073004/diff/40001/media/base/browser_cdm_f...
media/base/browser_cdm_factory.h:13: #include "media/base/media_keys.h"
When inheritance is broken, it will be odd to be using th CBs from this file for
another type. Will we duplicate them, move them, or something else?

https://codereview.chromium.org/308073004/diff/40001/media/base/player_tracker.h
File media/base/player_tracker.h (right):

https://codereview.chromium.org/308073004/diff/40001/media/base/player_tracke...
media/base/player_tracker.h:14: // An interface that helps CDMs notify players
when a new decryption key becomes
nit: "An interface for players to register to be notified when..."

https://codereview.chromium.org/308073004/diff/40001/media/base/player_tracke...
media/base/player_tracker.h:18: PlayerTracker();
ditto

https://codereview.chromium.org/308073004/diff/40001/media/base/player_tracke...
media/base/player_tracker.h:23: // - |cdm_destroyed_cb| is fired when the CDM is
destroyed.
ditto on the name

https://codereview.chromium.org/308073004/diff/40001/media/base/player_tracke...
media/base/player_tracker.h:26: const base::Closure& cdm_destroyed_cb) = 0;
On 2014/05/30 17:18:53, xhwang wrote:
> On 2014/05/30 15:10:15, damienv1 wrote:
> > There is an implicit assumption that the CDM and the player are running on
the
> > same thread here (which is ok to me), but this might not apply to every use
> > case.
> > By the time the notification is received on the other thread, the CDM can
> > already be destroyed.
> 
> The current plan is to use this only for BrowserCdm. So this simple model will
> work fine. We need to work out a long term plan for CDM/player tracking for
> desktop.

Can we add DCHECKs on the thread?

[xhwang, 2014-06-02 20:10:29]

comments addressed

[xhwang, 2014-06-02 20:11:43]

I think I addressed all comments. PTAL again.

https://codereview.chromium.org/308073004/diff/40001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/308073004/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:383:
MediaKeys::KeyError error_code,
On 2014/05/30 20:50:05, ddorwin wrote:
> When BrowserCdm no longer inherits from MediaKeys, we'll need a duplicate of
the
> error (to become Exception) enum.

Shall we put those errors in a common place? e.g. meida/base/cdm_common.h?

https://codereview.chromium.org/308073004/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:730: const int& id
= cdm_id;
On 2014/05/30 20:50:05, ddorwin wrote:
> nit: I think we usually just copy the value. The compiler should handle this.
> 
> This is also odd since cdm_id could change, changing the value of id.

Done.

https://codereview.chromium.org/308073004/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:766: // Currently
we do not support detaching CDM from a player.
On 2014/05/30 20:50:05, ddorwin wrote:
> Is this prevented by Blink? If so, DCHECK()? If not, we're not reporting a
> failure.

This is prevented by the WMPA. Added NOT_REACHED.

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
media/base/android/media_drm_bridge.cc:427: player_tracker_.NotifyNewKey();
On 2014/05/30 20:50:05, ddorwin wrote:
> Just FYI: This will not be called here in the future.

Yep. Added TODO.

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
media/base/android/media_drm_bridge.h:104: const base::Closure&
cdm_destroyed_cb) OVERRIDE;
On 2014/05/30 20:50:05, ddorwin wrote:
> UPDATE: Looking at the next file, this might need to handle the case when the
> CDM disappears (i.e. the platform closes it). I still think "destroyed" is an
> implementation detail of what the player should care about.
> 
> destroyed or disassociated?

This is really the reverse of SetCdm(). How about "unset"?
 
> It shouldn't be destroyed while associated should it? Or is this related to
the
> disconnection process where it may still be decrypting data until the threads
> all stop. Even in that case, it seems like disconnection rather than
destroyed,
> which is really irrelevant (the player just needs to stop using it).

In normal cases, Blink should prevent the CDM from being destroyed while it's
still "set" on a player. But this is in the browser process and we should be
more error proof :)

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
File media/base/android/media_player_android.cc (right):

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
media/base/android/media_player_android.cc:34: // Not all players support CDMs.
Do nothing by default.
On 2014/05/30 20:50:05, ddorwin wrote:
> Shouldn't this either be impossible (DCHECK) or cause an error for the app?

Done.

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
media/base/android/media_source_player.cc:70:
drm_bridge_->UnregisterPlayer(cdm_registration_id_);
On 2014/05/30 20:50:05, ddorwin wrote:
> Should this be called even if cdm_registration_id_ == 0?
> I believe it must be non-0 iff drm_bridge_ is not null. In that case, maybe
> these calls should be grouped.

Done.

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
media/base/android/media_source_player.cc:363: // Currently we don't support
resetting |drm_bridge_|.
On 2014/05/30 20:50:05, ddorwin wrote:
> Ditto about DCHECK and error reporting.

Done.

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
media/base/android/media_source_player.cc:370: cdm_registration_id_ =
cdm->RegisterPlayer(
On 2014/05/30 20:50:05, ddorwin wrote:
> A bit odd to cast then use the previous pointer (cdm). Note that Unregister is
> called on the member.

Done.

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
media/base/android/media_source_player.cc:374: if
(drm_bridge_->GetMediaCrypto().is_null()) {
On 2014/05/30 20:50:05, ddorwin wrote:
> Should this logic be made generic as well? Some type of OnInitializedCB?

Not sure whether there's a general need for this. We need it here because the
creation of MediaCypto object could be delayed (e.g. due to provisioning).
However, other platforms may not need this at all. I'd like to keep this code as
is until we have another platform needs something similar.

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
media/base/android/media_source_player.cc:1035: // TODO(xhwang): Support
detachment of CDM.
On 2014/05/30 20:50:05, ddorwin wrote:
> Don't you need to handle the platform-driven case (sleep) now?

That'll be a non-trivial change. I'll probably do that later.

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

https://codereview.chromium.org/308073004/diff/40001/media/base/browser_cdm.h...
media/base/browser_cdm.h:16: BrowserCdm();
On 2014/05/30 20:50:05, ddorwin wrote:
> protected or just remove.

Done.

https://codereview.chromium.org/308073004/diff/40001/media/base/browser_cdm_f...
File media/base/browser_cdm_factory.h (right):

https://codereview.chromium.org/308073004/diff/40001/media/base/browser_cdm_f...
media/base/browser_cdm_factory.h:11: #include "media/base/browser_cdm.h"
On 2014/05/30 15:10:15, damienv1 wrote:
> Foward declaration for BrowserCdm ?

Done.

https://codereview.chromium.org/308073004/diff/40001/media/base/browser_cdm_f...
media/base/browser_cdm_factory.h:13: #include "media/base/media_keys.h"
On 2014/05/30 20:50:05, ddorwin wrote:
> When inheritance is broken, it will be odd to be using th CBs from this file
for
> another type. Will we duplicate them, move them, or something else?

+jrummell

Shall we move the callbacks to a common place to be shared by MediaKeys and
BrwoserCdm impls? e.g. in media/base/cdm_common.h?

https://codereview.chromium.org/308073004/diff/40001/media/base/player_tracker.h
File media/base/player_tracker.h (right):

https://codereview.chromium.org/308073004/diff/40001/media/base/player_tracke...
media/base/player_tracker.h:14: // An interface that helps CDMs notify players
when a new decryption key becomes
On 2014/05/30 20:50:05, ddorwin wrote:
> nit: "An interface for players to register to be notified when..."

Done.

https://codereview.chromium.org/308073004/diff/40001/media/base/player_tracke...
media/base/player_tracker.h:18: PlayerTracker();
On 2014/05/30 20:50:05, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/308073004/diff/40001/media/base/player_tracke...
media/base/player_tracker.h:23: // - |cdm_destroyed_cb| is fired when the CDM is
destroyed.
On 2014/05/30 20:50:05, ddorwin wrote:
> ditto on the name

Done.

https://codereview.chromium.org/308073004/diff/40001/media/base/player_tracke...
media/base/player_tracker.h:26: const base::Closure& cdm_destroyed_cb) = 0;
On 2014/05/30 20:50:05, ddorwin wrote:
> On 2014/05/30 17:18:53, xhwang wrote:
> > On 2014/05/30 15:10:15, damienv1 wrote:
> > > There is an implicit assumption that the CDM and the player are running on
> the
> > > same thread here (which is ok to me), but this might not apply to every
use
> > > case.
> > > By the time the notification is received on the other thread, the CDM can
> > > already be destroyed.
> > 
> > The current plan is to use this only for BrowserCdm. So this simple model
will
> > work fine. We need to work out a long term plan for CDM/player tracking for
> > desktop.
> 
> Can we add DCHECKs on the thread?

Done.

[ddorwin, 2014-06-02 20:20:28]

Just a few more things. 2 comments in PS2.

https://codereview.chromium.org/308073004/diff/40001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/308073004/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:383:
MediaKeys::KeyError error_code,
On 2014/06/02 20:11:43, xhwang wrote:
> On 2014/05/30 20:50:05, ddorwin wrote:
> > When BrowserCdm no longer inherits from MediaKeys, we'll need a duplicate of
> the
> > error (to become Exception) enum.
> 
> Shall we put those errors in a common place? e.g. meida/base/cdm_common.h?

Maybe. That's how jrummel has them in the Blink CL.

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
media/base/android/media_source_player.cc:1035: // TODO(xhwang): Support
detachment of CDM.
On 2014/06/02 20:11:43, xhwang wrote:
> On 2014/05/30 20:50:05, ddorwin wrote:
> > Don't you need to handle the platform-driven case (sleep) now?
> 
> That'll be a non-trivial change. I'll probably do that later.

But does this break existing prefixed API behavior on Chrome for Android?

https://codereview.chromium.org/308073004/diff/60001/media/base/android/media...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/308073004/diff/60001/media/base/android/media...
media/base/android/media_drm_bridge.h:104: const base::Closure& cdm_unset_cb)
OVERRIDE;
Not a fan of the name, but I don't have anything better. :)

https://codereview.chromium.org/308073004/diff/60001/media/base/android/media...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/308073004/diff/60001/media/base/android/media...
media/base/android/media_source_player.cc:70: DCHECK_NE(cdm_registration_id_,
0);
If you want a DCHECK, it's probably something like EQ(!drm_bridge_,
!cdm_registration_id_) above line 69.

https://codereview.chromium.org/308073004/diff/60001/media/base/android/media...
File media/base/android/media_source_player.h (right):

https://codereview.chromium.org/308073004/diff/60001/media/base/android/media...
media/base/android/media_source_player.h:176: // Called when the CDM is
destroyed.
update comment.

https://codereview.chromium.org/308073004/diff/60001/media/base/player_tracker.h
File media/base/player_tracker.h (right):

https://codereview.chromium.org/308073004/diff/60001/media/base/player_tracke...
media/base/player_tracker.h:22: // - |cdm_unset_cb| is fired when the CDM is
detached from the player.
would s/unset/detached/ be a better name?

[xhwang, 2014-06-02 21:40:39]

comments addressed

[xhwang, 2014-06-02 21:41:22]

PTAL again.

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/308073004/diff/40001/media/base/android/media...
media/base/android/media_source_player.cc:1035: // TODO(xhwang): Support
detachment of CDM.
On 2014/06/02 20:20:29, ddorwin wrote:
> On 2014/06/02 20:11:43, xhwang wrote:
> > On 2014/05/30 20:50:05, ddorwin wrote:
> > > Don't you need to handle the platform-driven case (sleep) now?
> > 
> > That'll be a non-trivial change. I'll probably do that later.
> 
> But does this break existing prefixed API behavior on Chrome for Android?

Currently we are not releasing MediaDrm when the devise goes to sleep. In the
future we need to fix this. Added TODO.

https://codereview.chromium.org/308073004/diff/60001/media/base/android/media...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/308073004/diff/60001/media/base/android/media...
media/base/android/media_drm_bridge.h:104: const base::Closure& cdm_unset_cb)
OVERRIDE;
On 2014/06/02 20:20:29, ddorwin wrote:
> Not a fan of the name, but I don't have anything better. :)

:) I'll keep it as is unless we have something better.

https://codereview.chromium.org/308073004/diff/60001/media/base/android/media...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/308073004/diff/60001/media/base/android/media...
media/base/android/media_source_player.cc:70: DCHECK_NE(cdm_registration_id_,
0);
On 2014/06/02 20:20:29, ddorwin wrote:
> If you want a DCHECK, it's probably something like EQ(!drm_bridge_,
> !cdm_registration_id_) above line 69.

Done.

https://codereview.chromium.org/308073004/diff/60001/media/base/android/media...
File media/base/android/media_source_player.h (right):

https://codereview.chromium.org/308073004/diff/60001/media/base/android/media...
media/base/android/media_source_player.h:176: // Called when the CDM is
destroyed.
On 2014/06/02 20:20:29, ddorwin wrote:
> update comment.

Done.

https://codereview.chromium.org/308073004/diff/60001/media/base/player_tracker.h
File media/base/player_tracker.h (right):

https://codereview.chromium.org/308073004/diff/60001/media/base/player_tracke...
media/base/player_tracker.h:22: // - |cdm_unset_cb| is fired when the CDM is
detached from the player.
On 2014/06/02 20:20:29, ddorwin wrote:
> would s/unset/detached/ be a better name?

I chose "unset" because we have SetCdm().

[ddorwin, 2014-06-02 22:24:36]

lgtm

[xhwang, 2014-06-02 23:49:32]

TBRing damienv@ since he's OOO. 

damienv: Feel free to uncheck the commit box if you have comments before it's
landed :)

[xhwang, 2014-06-02 23:49:40]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-06-02 23:53:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/308073004/80001

[commit-bot: I haz the power, 2014-06-03 11:55:12]

Change committed as 274470