New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 821288 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Support NewKeyCB registration in CdmContext and remove media::PlayerTracker

Project Member Reported by xhw...@chromium.org, Mar 13 2018

Issue description

This class was introduced back when we have BrowserMediaDrmManager to make sure a media player (decoder) doesn't reference a deleted CDM (MediaDrmBridge).

Now with Mojo Media, we always guarantee that the CDM will be destructed after the player (Renderer/Decoder) that has the CDM set on it. Hence, PlayerTracker is not needed anymore.

We do still need RegisterNewKeyCB() so that the player will get notified when a new key is available on the CDM. We should add this to CdmContext interface since this is needed in all cases. Note that currently on desktop Chrome we have Decryptor::RegisterNewKeyCB(). We should unify all these cases.

One tricky issue is that RegisterNewKeyCB() is often called on a different thread. We should make sure this is documented and correctly handled.
 

Comment 1 by xhw...@chromium.org, Mar 23 2018

Labels: -M-67 M-68

Comment 2 by xhw...@chromium.org, Apr 13 2018

Cc: rkuroiwa@chromium.org
Labels: -Pri-2 OS-Chrome OS-Linux OS-Mac OS-Windows Pri-1
Status: Started (was: Assigned)
Summary: Support NewKeyCB registration in CdmContext and remove media::PlayerTracker (was: Remove media::PlayerTracker)
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e3f1f4f85bd6f63480eade838955072b7d690f86

commit e3f1f4f85bd6f63480eade838955072b7d690f86
Author: Xiaohan Wang <xhwang@chromium.org>
Date: Mon Apr 16 21:34:06 2018

media: Misc cleanup of CdmContext

- Clarify lifetime model.
- Clarify thread model.
- Remove GetClassIdentifier(). Moving foreard, we should add virtual
  methods to CdmContext to support platform-specific interfaces, instead
  of using static_cast.

TBR=jrummell@chromium.org,rkuroiwa@chromium.org

Bug: 821288
Test: No functionality change.
Change-Id: I13e30ed2c508855727219edd3975abf5d67e6721
Reviewed-on: https://chromium-review.googlesource.com/1014284
Reviewed-by: Xiaohan Wang <xhwang@chromium.org>
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551128}
[modify] https://crrev.com/e3f1f4f85bd6f63480eade838955072b7d690f86/media/base/cdm_context.cc
[modify] https://crrev.com/e3f1f4f85bd6f63480eade838955072b7d690f86/media/base/cdm_context.h

Project Member

Comment 4 by bugdroid1@chromium.org, Apr 17 2018

Labels: merge-merged-testbranch
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e3f1f4f85bd6f63480eade838955072b7d690f86

commit e3f1f4f85bd6f63480eade838955072b7d690f86
Author: Xiaohan Wang <xhwang@chromium.org>
Date: Mon Apr 16 21:34:06 2018

media: Misc cleanup of CdmContext

- Clarify lifetime model.
- Clarify thread model.
- Remove GetClassIdentifier(). Moving foreard, we should add virtual
  methods to CdmContext to support platform-specific interfaces, instead
  of using static_cast.

TBR=jrummell@chromium.org,rkuroiwa@chromium.org

Bug: 821288
Test: No functionality change.
Change-Id: I13e30ed2c508855727219edd3975abf5d67e6721
Reviewed-on: https://chromium-review.googlesource.com/1014284
Reviewed-by: Xiaohan Wang <xhwang@chromium.org>
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551128}
[modify] https://crrev.com/e3f1f4f85bd6f63480eade838955072b7d690f86/media/base/cdm_context.cc
[modify] https://crrev.com/e3f1f4f85bd6f63480eade838955072b7d690f86/media/base/cdm_context.h

Project Member

Comment 5 by bugdroid1@chromium.org, Apr 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a80616b718518a18b5117b925f7682304f9e121a

commit a80616b718518a18b5117b925f7682304f9e121a
Author: Xiaohan Wang <xhwang@chromium.org>
Date: Tue Apr 17 20:49:21 2018

media: Support NewKeyCB registration in CdmContext

All players/decoders handling encrypted media should implement the logic
to suspend playback when the decryption key is not available, and then
resume playback when a new usable decryption key is added. Hence,
there's a common need for the players/decoders to be notified when such
an event (new usable key) happens.

Since CdmContext is the common interface bridging players/decoders and
the CDM, this CL adds RegisterNewKeyCB() to CdmContext.

To facilitate the implementation of RegisterNewKeyCB() in various CDMs
and CdmProxies, CallbackRegistry is also introduced, which can manage
callback registration, notification and unregistration. Note that
there's base::CallbackList which is similar, but that class is not as
simple and is not thread-safe.

This CL also updates D3D11CdmProxy to use CallbackRegistry to manage
new key callbacks. D3D11VideoDecoderImpl is also updated to be able to
register new key callbacks.

Bug: 821288
Test: Covered by existing tests.
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I0380f934efb63d3b3c893d909e0d78f5f8e082f4
Reviewed-on: https://chromium-review.googlesource.com/1014245
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551468}
[modify] https://crrev.com/a80616b718518a18b5117b925f7682304f9e121a/media/base/BUILD.gn
[add] https://crrev.com/a80616b718518a18b5117b925f7682304f9e121a/media/base/callback_registry.h
[add] https://crrev.com/a80616b718518a18b5117b925f7682304f9e121a/media/base/callback_registry_unittest.cc
[modify] https://crrev.com/a80616b718518a18b5117b925f7682304f9e121a/media/base/cdm_context.cc
[modify] https://crrev.com/a80616b718518a18b5117b925f7682304f9e121a/media/base/cdm_context.h
[modify] https://crrev.com/a80616b718518a18b5117b925f7682304f9e121a/media/cdm/aes_decryptor.cc
[modify] https://crrev.com/a80616b718518a18b5117b925f7682304f9e121a/media/cdm/aes_decryptor.h
[modify] https://crrev.com/a80616b718518a18b5117b925f7682304f9e121a/media/cdm/cdm_adapter.cc
[modify] https://crrev.com/a80616b718518a18b5117b925f7682304f9e121a/media/cdm/cdm_adapter.h
[modify] https://crrev.com/a80616b718518a18b5117b925f7682304f9e121a/media/gpu/windows/d3d11_cdm_proxy.cc
[modify] https://crrev.com/a80616b718518a18b5117b925f7682304f9e121a/media/gpu/windows/d3d11_video_decoder_impl.cc
[modify] https://crrev.com/a80616b718518a18b5117b925f7682304f9e121a/media/gpu/windows/d3d11_video_decoder_impl.h

Comment 6 by xhw...@chromium.org, May 18 2018

Labels: -Pri-1 Pri-2
This is not blocking D3D11CdmProxy any more. Mark as P2.

The rest of the work is to refactor and unify how we register NewKeyCB on all platforms, and eventually remove media::PlayerTracker.

Comment 7 by xhw...@chromium.org, Jun 22 2018

Labels: -M-68 Target-70
Labels: -Target-70 Target-71
Labels: -Target-71 Target-72
Labels: M-72
Labels: -M-72 -Target-72 M-74 Target-74

Sign in to add a comment