New issue
Advanced search Search tips

Issue 786736 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

merge duplicate code in mojo_(audio, video)_decoder_service

Project Member Reported by liber...@chromium.org, Nov 18 2017

Issue description

from xhwang:

1. Previously we pass MojoCdmServiceContext as a weakptr, because MojoVideoDecoderService et al use StrongBinding, so that they can outlive InterfaceFactoryImpl (which owns MojoCdmServiceContext). Now that we use StrongBindingSet in InterfaceFactoryImpl, and this is not true anymore. We should just pass in a raw pointer of MojoCdmServiceContext and DCHECK that it is not null. This can help us get rid of the first check.

2. All users of MojoCdmServiceContext actually only need the |cdm_context|. The reason that they need the |cdm| is such the CDM will not be destroyed (it's ref-counted). Maybe we should fix this, e.g. instead of pass in a raw pointer of CdmContext, we pass in a unique_ptr<CdmContext>, such that each |cdm_context| will hold a ref to the actual CDM. Then we don't need to expose the CDM at all.

With that, the code will be much simpler:

  std::unique_ptr<CdmContext> cdm_context = mojo_cdm_service_context_->GetCdm(cdm_id);

  if (config.is_encrypted() && !cdm_context) {
    std::move(callback).Run(false, false);
    return;
  }

  // .... Initialize(..., std::move(cdm_context)...

We can do (1) easily but (2) would take more work.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 22 2017

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

commit d44c932dd455b5f12346dfdb2ab60a56a34c442d
Author: Xiaohan Wang <xhwang@chromium.org>
Date: Wed Nov 22 08:30:28 2017

media: Remove WeakPtr usage on MojoCdmServiceContext

Currently we use StrongBindingSet for mojo interface implementations
that needs access to MojoCdmServiceContext. Hence the lifetime of those
objects are determined and we should be able to use raw pointers instead
of weak pointers.

In MojoCdmService, MojoAudioDecoderService and MojoVideoDecoderService
we assume the MojoCdmServiceContext always exists.

In MojoRendererService, where it is currently also used outside of
MediaService (see RenderFrameHostImpl), it's possible that
MojoCdmServiceContext can be null, and we have explicit check on that.

BUG= 786736 
TEST=Existing tests pass.

Change-Id: Idd7e0a1bb24d109c7a17607e3fd856506432e9ee
Reviewed-on: https://chromium-review.googlesource.com/780046
Reviewed-by: Frank Liberato <liberato@chromium.org>
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518568}
[modify] https://crrev.com/d44c932dd455b5f12346dfdb2ab60a56a34c442d/media/mojo/clients/mojo_audio_decoder_unittest.cc
[modify] https://crrev.com/d44c932dd455b5f12346dfdb2ab60a56a34c442d/media/mojo/clients/mojo_cdm_unittest.cc
[modify] https://crrev.com/d44c932dd455b5f12346dfdb2ab60a56a34c442d/media/mojo/clients/mojo_renderer_unittest.cc
[modify] https://crrev.com/d44c932dd455b5f12346dfdb2ab60a56a34c442d/media/mojo/services/interface_factory_impl.cc
[modify] https://crrev.com/d44c932dd455b5f12346dfdb2ab60a56a34c442d/media/mojo/services/interface_factory_impl.h
[modify] https://crrev.com/d44c932dd455b5f12346dfdb2ab60a56a34c442d/media/mojo/services/mojo_audio_decoder_service.cc
[modify] https://crrev.com/d44c932dd455b5f12346dfdb2ab60a56a34c442d/media/mojo/services/mojo_audio_decoder_service.h
[modify] https://crrev.com/d44c932dd455b5f12346dfdb2ab60a56a34c442d/media/mojo/services/mojo_cdm_service.cc
[modify] https://crrev.com/d44c932dd455b5f12346dfdb2ab60a56a34c442d/media/mojo/services/mojo_cdm_service.h
[modify] https://crrev.com/d44c932dd455b5f12346dfdb2ab60a56a34c442d/media/mojo/services/mojo_cdm_service_context.cc
[modify] https://crrev.com/d44c932dd455b5f12346dfdb2ab60a56a34c442d/media/mojo/services/mojo_cdm_service_context.h
[modify] https://crrev.com/d44c932dd455b5f12346dfdb2ab60a56a34c442d/media/mojo/services/mojo_renderer_service.cc
[modify] https://crrev.com/d44c932dd455b5f12346dfdb2ab60a56a34c442d/media/mojo/services/mojo_renderer_service.h
[modify] https://crrev.com/d44c932dd455b5f12346dfdb2ab60a56a34c442d/media/mojo/services/mojo_video_decoder_service.cc
[modify] https://crrev.com/d44c932dd455b5f12346dfdb2ab60a56a34c442d/media/mojo/services/mojo_video_decoder_service.h

Project Member

Comment 2 by bugdroid1@chromium.org, Jan 22 2018

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

commit 24cfe2ca626220cf9b9c46a6a57ea753795d06e0
Author: Xiaohan Wang <xhwang@chromium.org>
Date: Mon Jan 22 23:16:00 2018

media: Add CdmContextRef interface

This new interface serves two purposes:

1. By holding this class, we can make sure the underlying CDM or
CdmProxy are kept alive. This provides a unified way to support both CDM
and CdmProxy.

2. This helps avoid the need to hold a reference to the CDM directly,
further separating the media stack and the EME stack. After this change,
we should be able to move content_decryption_module.h from media/base to
media/cdm (in a later CL).

This CL also adds CdmContextRefImpl, a default CdmContextRef impl that
holds a reference to a media::ContentDecryptionModule.

BUG=799310, 786736 ,803710
TEST=No functionality change. All existing tests pass.

Change-Id: I64bae76d5a5a0912728ebd8af7ea1ef57a638305
Reviewed-on: https://chromium-review.googlesource.com/876767
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Reviewed-by: John Rummell <jrummell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531049}
[modify] https://crrev.com/24cfe2ca626220cf9b9c46a6a57ea753795d06e0/media/base/cdm_context.h
[modify] https://crrev.com/24cfe2ca626220cf9b9c46a6a57ea753795d06e0/media/base/content_decryption_module.cc
[modify] https://crrev.com/24cfe2ca626220cf9b9c46a6a57ea753795d06e0/media/base/content_decryption_module.h
[modify] https://crrev.com/24cfe2ca626220cf9b9c46a6a57ea753795d06e0/media/blink/cdm_session_adapter.cc
[modify] https://crrev.com/24cfe2ca626220cf9b9c46a6a57ea753795d06e0/media/blink/cdm_session_adapter.h
[modify] https://crrev.com/24cfe2ca626220cf9b9c46a6a57ea753795d06e0/media/blink/webcontentdecryptionmodule_impl.cc
[modify] https://crrev.com/24cfe2ca626220cf9b9c46a6a57ea753795d06e0/media/blink/webcontentdecryptionmodule_impl.h
[modify] https://crrev.com/24cfe2ca626220cf9b9c46a6a57ea753795d06e0/media/blink/webmediaplayer_impl.cc
[modify] https://crrev.com/24cfe2ca626220cf9b9c46a6a57ea753795d06e0/media/blink/webmediaplayer_impl.h
[modify] https://crrev.com/24cfe2ca626220cf9b9c46a6a57ea753795d06e0/media/cdm/BUILD.gn
[add] https://crrev.com/24cfe2ca626220cf9b9c46a6a57ea753795d06e0/media/cdm/cdm_context_ref_impl.cc
[add] https://crrev.com/24cfe2ca626220cf9b9c46a6a57ea753795d06e0/media/cdm/cdm_context_ref_impl.h
[modify] https://crrev.com/24cfe2ca626220cf9b9c46a6a57ea753795d06e0/media/mojo/services/mojo_audio_decoder_service.cc
[modify] https://crrev.com/24cfe2ca626220cf9b9c46a6a57ea753795d06e0/media/mojo/services/mojo_audio_decoder_service.h
[modify] https://crrev.com/24cfe2ca626220cf9b9c46a6a57ea753795d06e0/media/mojo/services/mojo_cdm_service_context.cc
[modify] https://crrev.com/24cfe2ca626220cf9b9c46a6a57ea753795d06e0/media/mojo/services/mojo_cdm_service_context.h
[modify] https://crrev.com/24cfe2ca626220cf9b9c46a6a57ea753795d06e0/media/mojo/services/mojo_renderer_service.cc
[modify] https://crrev.com/24cfe2ca626220cf9b9c46a6a57ea753795d06e0/media/mojo/services/mojo_renderer_service.h
[modify] https://crrev.com/24cfe2ca626220cf9b9c46a6a57ea753795d06e0/media/mojo/services/mojo_video_decoder_service.cc
[modify] https://crrev.com/24cfe2ca626220cf9b9c46a6a57ea753795d06e0/media/mojo/services/mojo_video_decoder_service.h

Comment 3 by xhw...@chromium.org, Mar 24 2018

Status: Fixed (was: Assigned)

Sign in to add a comment