media: Simplify CdmService

- Restrict CdmService to only host library CDM. Previously CdmService is generic, but it's never used anywhere else, and that assumption has caused some unnecessary code complexity. - As such, also remove ENABLE_STANDALONE_CDM_SERVICE and replace it with ENABLE_LIBRARY_CDMS where applicable. - Remove the use of media::mojom::InterfaceFactory for the CdmService, and replace it with a dedicated media::mojom::CdmFactory. - Remove the use of MojoMediaClient in CdmService, and replace it with CdmService::Client. BUG=771791 Change-Id: I49e0fc4958b18e1b619e20ab4772245b256ab454 Reviewed-on: https://chromium-review.googlesource.com/852556 Commit-Queue: Xiaohan Wang <xhwang@chromium.org> Reviewed-by: John Rummell <jrummell@chromium.org> Reviewed-by: Nasko Oskov <nasko@chromium.org> Cr-Commit-Position: refs/heads/master@{#531329}
Denger-Network · Jan 23, 2018 · a3cb5ee · a3cb5ee
1 parent e8ce3a4
commit a3cb5ee
Show file tree

Hide file tree

Showing 16 changed files with 231 additions and 214 deletions.
diff --git a/content/browser/media/media_interface_proxy.cc b/content/browser/media/media_interface_proxy.cc
@@ -15,6 +15,7 @@
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/common/content_client.h"
 #include "content/public/common/service_manager_connection.h"
+#include "media/mojo/features.h"
 #include "media/mojo/interfaces/constants.mojom.h"
 #include "media/mojo/interfaces/media_service.mojom.h"
 #include "media/mojo/services/media_interface_provider.h"
@@ -36,16 +37,13 @@
 #include "media/base/key_system_names.h"
 #include "mojo/public/cpp/bindings/interface_request.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
-#if defined(OS_MACOSX)
-#include "sandbox/mac/seatbelt_extension.h"
-#endif  // defined(OS_MACOSX)
-#endif  // BUILDFLAG(ENABLE_LIBRARY_CDMS)
-
 #if defined(OS_MACOSX)
 #include "media/mojo/interfaces/cdm_service_mac.mojom.h"
+#include "sandbox/mac/seatbelt_extension.h"
 #else
 #include "media/mojo/interfaces/cdm_service.mojom.h"
 #endif  // defined(OS_MACOSX)
+#endif  // BUILDFLAG(ENABLE_LIBRARY_CDMS)
 
 namespace content {
 
@@ -123,8 +121,8 @@ MediaInterfaceProxy::MediaInterfaceProxy(
 
   binding_.set_connection_error_handler(error_handler);
 
-  // |interface_factory_ptr_| and |cdm_interface_factory_map_| will be lazily
-  // connected in GetMediaInterfaceFactory() and GetCdmInterfaceFactory().
+  // |interface_factory_ptr_| and |cdm_factory_map_| will be lazily
+  // connected in GetMediaInterfaceFactory() and GetCdmFactory().
 }
 
 MediaInterfaceProxy::~MediaInterfaceProxy() {
@@ -161,16 +159,15 @@ void MediaInterfaceProxy::CreateCdm(
     const std::string& key_system,
     media::mojom::ContentDecryptionModuleRequest request) {
   DCHECK(thread_checker_.CalledOnValidThread());
-
-  InterfaceFactory* factory =
-#if !BUILDFLAG(ENABLE_STANDALONE_CDM_SERVICE)
-      GetMediaInterfaceFactory();
+#if !BUILDFLAG(ENABLE_LIBRARY_CDMS)
+  auto* factory = GetMediaInterfaceFactory();
+  if (factory)
+    factory->CreateCdm(key_system, std::move(request));
 #else
-      GetCdmInterfaceFactory(key_system);
-#endif
-
+  auto* factory = GetCdmFactory(key_system);
   if (factory)
     factory->CreateCdm(key_system, std::move(request));
+#endif
 }
 
 void MediaInterfaceProxy::CreateCdmProxy(
@@ -258,17 +255,16 @@ void MediaInterfaceProxy::OnMediaServiceConnectionError() {
   interface_factory_ptr_.reset();
 }
 
-#if BUILDFLAG(ENABLE_STANDALONE_CDM_SERVICE)
+#if BUILDFLAG(ENABLE_LIBRARY_CDMS)
 
-media::mojom::InterfaceFactory* MediaInterfaceProxy::GetCdmInterfaceFactory(
+media::mojom::CdmFactory* MediaInterfaceProxy::GetCdmFactory(
     const std::string& key_system) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   std::string cdm_guid;
   base::FilePath cdm_path;
   std::string cdm_file_system_id;
 
-#if BUILDFLAG(ENABLE_LIBRARY_CDMS)
   std::unique_ptr<CdmInfo> cdm_info =
       KeySystemSupportImpl::GetCdmInfoForKeySystem(key_system);
   if (!cdm_info) {
@@ -290,22 +286,21 @@ media::mojom::InterfaceFactory* MediaInterfaceProxy::GetCdmInterfaceFactory(
   cdm_guid = cdm_info->guid;
   cdm_path = cdm_info->path;
   cdm_file_system_id = cdm_info->file_system_id;
-#endif  // BUILDFLAG(ENABLE_LIBRARY_CDMS)
 
-  auto found = cdm_interface_factory_map_.find(cdm_guid);
-  if (found != cdm_interface_factory_map_.end())
+  auto found = cdm_factory_map_.find(cdm_guid);
+  if (found != cdm_factory_map_.end())
     return found->second.get();
 
   return ConnectToCdmService(cdm_guid, cdm_path, cdm_file_system_id);
 }
 
-media::mojom::InterfaceFactory* MediaInterfaceProxy::ConnectToCdmService(
+media::mojom::CdmFactory* MediaInterfaceProxy::ConnectToCdmService(
     const std::string& cdm_guid,
     const base::FilePath& cdm_path,
     const std::string& cdm_file_system_id) {
   DVLOG(1) << __func__ << ": cdm_guid = " << cdm_guid;
 
-  DCHECK(!cdm_interface_factory_map_.count(cdm_guid));
+  DCHECK(!cdm_factory_map_.count(cdm_guid));
   service_manager::Identity identity(media::mojom::kCdmServiceName,
                                      service_manager::mojom::kInheritUserID,
                                      cdm_guid);
@@ -317,7 +312,6 @@ media::mojom::InterfaceFactory* MediaInterfaceProxy::ConnectToCdmService(
   media::mojom::CdmServicePtr cdm_service;
   connector->BindInterface(identity, &cdm_service);
 
-#if BUILDFLAG(ENABLE_LIBRARY_CDMS)
 #if defined(OS_MACOSX)
   // LoadCdm() should always be called before CreateInterfaceFactory().
   media::mojom::SeatbeltExtensionTokenProviderPtr token_provider_ptr;
@@ -329,33 +323,28 @@ media::mojom::InterfaceFactory* MediaInterfaceProxy::ConnectToCdmService(
 #else
   cdm_service->LoadCdm(cdm_path);
 #endif  // defined(OS_MACOSX)
-#endif  // BUILDFLAG(ENABLE_LIBRARY_CDMS)
 
-  InterfaceFactoryPtr interface_factory_ptr;
-  cdm_service->CreateInterfaceFactory(
-      MakeRequest(&interface_factory_ptr),
-      GetFrameServices(cdm_guid, cdm_file_system_id));
-  interface_factory_ptr.set_connection_error_handler(
+  media::mojom::CdmFactoryPtr cdm_factory_ptr;
+  cdm_service->CreateCdmFactory(MakeRequest(&cdm_factory_ptr),
+                                GetFrameServices(cdm_guid, cdm_file_system_id));
+  cdm_factory_ptr.set_connection_error_handler(
       base::BindOnce(&MediaInterfaceProxy::OnCdmServiceConnectionError,
                      base::Unretained(this), cdm_guid));
 
-  InterfaceFactory* cdm_interface_factory = interface_factory_ptr.get();
-  cdm_interface_factory_map_.emplace(cdm_guid,
-                                     std::move(interface_factory_ptr));
-  return cdm_interface_factory;
+  auto* cdm_factory = cdm_factory_ptr.get();
+  cdm_factory_map_.emplace(cdm_guid, std::move(cdm_factory_ptr));
+  return cdm_factory;
 }
 
 void MediaInterfaceProxy::OnCdmServiceConnectionError(
     const std::string& cdm_guid) {
   DVLOG(1) << __func__;
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  DCHECK(cdm_interface_factory_map_.count(cdm_guid));
-  cdm_interface_factory_map_.erase(cdm_guid);
+  DCHECK(cdm_factory_map_.count(cdm_guid));
+  cdm_factory_map_.erase(cdm_guid);
 }
-#endif  // BUILDFLAG(ENABLE_STANDALONE_CDM_SERVICE)
 
-#if BUILDFLAG(ENABLE_LIBRARY_CDMS)
 void MediaInterfaceProxy::CreateCdmProxyInternal(
     const std::string& cdm_guid,
     media::mojom::CdmProxyRequest request) {

diff --git a/content/browser/media/media_interface_proxy.h b/content/browser/media/media_interface_proxy.h
@@ -11,7 +11,8 @@
 
 #include "base/macros.h"
 #include "base/threading/thread_checker.h"
-#include "media/mojo/features.h"
+#include "media/media_features.h"
+#include "media/mojo/interfaces/content_decryption_module.mojom.h"
 #include "media/mojo/interfaces/interface_factory.mojom.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "services/service_manager/public/interfaces/interface_provider.mojom.h"
@@ -27,9 +28,9 @@ class RenderFrameHost;
 // This implements the media::mojom::InterfaceFactory interface for a
 // RenderFrameHostImpl. Upon InterfaceFactory calls, it will
 // figure out where to forward to the interface requests. For example,
-// - When |enable_standalone_cdm_service| is true, forward CDM request to a
-//   standalone CDM service rather than the general media service.
-// - Forward CDM requests to different CDM service instances based on library
+// - When |enable_library_cdms| is true, forward CDM request to the CdmService
+// rather than the general media service.
+// - Forward CDM requests to different CdmService instances based on library
 //   CDM types.
 class MediaInterfaceProxy : public media::mojom::InterfaceFactory {
  public:
@@ -52,8 +53,6 @@ class MediaInterfaceProxy : public media::mojom::InterfaceFactory {
                       media::mojom::CdmProxyRequest request) final;
 
  private:
-  using InterfaceFactoryPtr = media::mojom::InterfaceFactoryPtr;
-
   // Gets services provided by the browser (at RenderFrameHost level) to the
   // mojo media (or CDM) service running remotely. |cdm_file_system_id| is
   // used to register the appropriate CdmStorage interface needed by the CDM.
@@ -70,27 +69,25 @@ class MediaInterfaceProxy : public media::mojom::InterfaceFactory {
   // Callback for connection error from |interface_factory_ptr_|.
   void OnMediaServiceConnectionError();
 
-#if BUILDFLAG(ENABLE_STANDALONE_CDM_SERVICE)
-  // Gets a CDM InterfaceFactory pointer for |key_system|. Returns null if
-  // unexpected error happened.
-  InterfaceFactory* GetCdmInterfaceFactory(const std::string& key_system);
+#if BUILDFLAG(ENABLE_LIBRARY_CDMS)
+  // Gets a CdmFactory pointer for |key_system|. Returns null if unexpected
+  // error happened.
+  media::mojom::CdmFactory* GetCdmFactory(const std::string& key_system);
 
   // Connects to the CDM service associated with |cdm_guid|, adds the new
-  // InterfaceFactoryPtr to the |cdm_interface_factory_map_|, and returns the
-  // newly created InterfaceFactory pointer. Returns nullptr if unexpected error
-  // happened. |cdm_path| will be used to preload the CDM, if necessary.
-  // |cdm_file_system_id| is used when creating the matching storage
-  // interface.
-  InterfaceFactory* ConnectToCdmService(const std::string& cdm_guid,
-                                        const base::FilePath& cdm_path,
-                                        const std::string& cdm_file_system_id);
-
-  // Callback for connection error from the InterfaceFactoryPtr in the
-  // |cdm_interface_factory_map_| associated with |cdm_guid|.
+  // CdmFactoryPtr to the |cdm_factory_map_|, and returns the newly created
+  // CdmFactory pointer. Returns nullptr if unexpected error happened.
+  // |cdm_path| will be used to preload the CDM, if necessary.
+  // |cdm_file_system_id| is used when creating the matching storage interface.
+  media::mojom::CdmFactory* ConnectToCdmService(
+      const std::string& cdm_guid,
+      const base::FilePath& cdm_path,
+      const std::string& cdm_file_system_id);
+
+  // Callback for connection error from the CdmFactoryPtr in the
+  // |cdm_factory_map_| associated with |cdm_guid|.
   void OnCdmServiceConnectionError(const std::string& cdm_guid);
-#endif  // BUILDFLAG(ENABLE_STANDALONE_CDM_SERVICE)
 
-#if BUILDFLAG(ENABLE_LIBRARY_CDMS)
   // Creates a CdmProxy for the CDM in CdmService. Not implemented in
   // CreateCdmProxy() because we don't want any client to be able to create
   // a CdmProxy.
@@ -112,13 +109,13 @@ class MediaInterfaceProxy : public media::mojom::InterfaceFactory {
   // in the service named kMediaServiceName hosted in the process specified by
   // the "mojo_media_host" gn argument. Available options are browser, GPU and
   // utility processes.
-  InterfaceFactoryPtr interface_factory_ptr_;
+  media::mojom::InterfaceFactoryPtr interface_factory_ptr_;
 
-#if BUILDFLAG(ENABLE_STANDALONE_CDM_SERVICE)
+#if BUILDFLAG(ENABLE_LIBRARY_CDMS)
   // CDM GUID to CDM InterfaceFactoryPtr mapping, where the InterfaceFactory
   // instances live in the standalone kCdmServiceName service instances.
-  std::map<std::string, InterfaceFactoryPtr> cdm_interface_factory_map_;
-#endif  // BUILDFLAG(ENABLE_STANDALONE_CDM_SERVICE)
+  std::map<std::string, media::mojom::CdmFactoryPtr> cdm_factory_map_;
+#endif  // BUILDFLAG(ENABLE_LIBRARY_CDMS)
 
   base::ThreadChecker thread_checker_;
 

diff --git a/content/browser/service_manager/service_manager_context.cc b/content/browser/service_manager/service_manager_context.cc
@@ -43,6 +43,7 @@
 #include "content/public/common/service_manager_connection.h"
 #include "content/public/common/service_names.mojom.h"
 #include "device/geolocation/geolocation_provider.h"
+#include "media/media_features.h"
 #include "media/mojo/features.h"
 #include "media/mojo/interfaces/constants.mojom.h"
 #include "mojo/edk/embedder/embedder.h"
@@ -579,7 +580,7 @@ ServiceManagerContext::ServiceManagerContext() {
       base::ASCIIToUTF16("Media Service");
 #endif
 
-#if BUILDFLAG(ENABLE_STANDALONE_CDM_SERVICE)
+#if BUILDFLAG(ENABLE_LIBRARY_CDMS)
   out_of_process_services[media::mojom::kCdmServiceName] =
       base::ASCIIToUTF16("Content Decryption Module Service");
 #endif

diff --git a/content/utility/utility_service_factory.cc b/content/utility/utility_service_factory.cc
@@ -32,7 +32,6 @@
 
 #if BUILDFLAG(ENABLE_LIBRARY_CDMS)
 #include "media/cdm/cdm_adapter_factory.h"           // nogncheck
-#include "media/mojo/features.h"                     // nogncheck
 #include "media/mojo/interfaces/constants.mojom.h"   // nogncheck
 #include "media/mojo/services/cdm_service.h"         // nogncheck
 #include "media/mojo/services/mojo_cdm_helper.h"     // nogncheck
@@ -62,18 +61,15 @@ namespace {
 
 #if BUILDFLAG(ENABLE_LIBRARY_CDMS)
 
-static_assert(BUILDFLAG(ENABLE_STANDALONE_CDM_SERVICE), "");
-static_assert(BUILDFLAG(ENABLE_MOJO_CDM), "");
-
 std::unique_ptr<media::CdmAuxiliaryHelper> CreateCdmHelper(
     service_manager::mojom::InterfaceProvider* interface_provider) {
   return std::make_unique<media::MojoCdmHelper>(interface_provider);
 }
 
-class CdmMojoMediaClient final : public media::MojoMediaClient {
+class ContentCdmServiceClient final : public media::CdmService::Client {
  public:
-  CdmMojoMediaClient() {}
-  ~CdmMojoMediaClient() override {}
+  ContentCdmServiceClient() {}
+  ~ContentCdmServiceClient() override {}
 
   void EnsureSandboxed() override {
 #if defined(OS_WIN)
@@ -100,7 +96,7 @@ class CdmMojoMediaClient final : public media::MojoMediaClient {
 
 std::unique_ptr<service_manager::Service> CreateCdmService() {
   return std::unique_ptr<service_manager::Service>(
-      new ::media::CdmService(std::make_unique<CdmMojoMediaClient>()));
+      new ::media::CdmService(std::make_unique<ContentCdmServiceClient>()));
 }
 #endif  // BUILDFLAG(ENABLE_LIBRARY_CDMS)
 

diff --git a/media/media_options.gni b/media/media_options.gni
@@ -129,7 +129,8 @@ declare_args() {
 }
 
 # Enables the use of library CDMs that implements the interface defined at
-# media/cdm/api/content_decryption_module.h
+# media/cdm/api/content_decryption_module.h. If true, the actually library CDM
+# will be hosted in the mojo CDM service running in the CDM (utility) process.
 # TODO(xhwang): Remove |enable_plugins| once pepper CDM support is removed.
 enable_library_cdms = enable_plugins && (is_linux || is_mac || is_win)
 
@@ -194,20 +195,19 @@ declare_args() {
   #                    Renderer. Cannot be used with the mojo Renderer above.
   mojo_media_services = []
 
-  # The process to host the mojo media service.
+  # The process that the mojo MediaService runs in. By default, all services
+  # registered in |mojo_media_services| are hosted in the MediaService, with the
+  # exception that when |enable_library_cdms| is true, the "cdm" service will
+  # run in a separate CdmService in the CDM (utility) process, while other
+  # |mojo_media_services| still run in the MediaService in the process specified
+  # by "mojo_media_host".
   # Valid options are:
   # - "none": Do not use mojo media service.
   # - "browser": Use mojo media service hosted in the browser process.
   # - "gpu": Use mojo media service hosted in the gpu process.
   # - "utility": Use mojo media service hosted in the utility process.
   mojo_media_host = "none"
 
-  # Force to host the CDM service in standalone "cdm" service instead of the
-  # "media" service hosted in the process specified by "mojo_media_host". Other
-  # mojo media services will not be affected. If true, the "cdm" service must
-  # also be included in "mojo_media_services".
-  enable_standalone_cdm_service = false
-
   # Default mojo_media_services and mojo_media_host on various platforms.
   # Can be overridden by gn build arguments from the --args command line flag
   # for local testing.
@@ -229,7 +229,6 @@ declare_args() {
     } else if (enable_library_cdms) {
       mojo_media_services = [ "cdm" ]
       mojo_media_host = "gpu"
-      enable_standalone_cdm_service = true
     }
 
     if (is_win) {

diff --git a/media/mojo/BUILD.gn b/media/mojo/BUILD.gn
@@ -46,16 +46,11 @@ buildflag_header("features") {
       enable_mojo_media_in_gpu_process = true
     } else if (mojo_media_host == "utility") {
       enable_mojo_media_in_utility_process = true
-    } else if (!enable_standalone_cdm_service) {
+    } else {
       assert(false, "Invalid mojo media host: $mojo_media_host")
     }
   }
 
-  if (enable_standalone_cdm_service) {
-    assert(enable_mojo_cdm,
-           "Mojo CDM must be enabled to run in standalone CDM service.")
-  }
-
   flags = [
     "ENABLE_MOJO_MEDIA=$enable_mojo_media",
     "ENABLE_TEST_MOJO_MEDIA_CLIENT=$enable_test_mojo_media_client",
@@ -66,7 +61,6 @@ buildflag_header("features") {
     "ENABLE_MOJO_MEDIA_IN_BROWSER_PROCESS=$enable_mojo_media_in_browser_process",
     "ENABLE_MOJO_MEDIA_IN_GPU_PROCESS=$enable_mojo_media_in_gpu_process",
     "ENABLE_MOJO_MEDIA_IN_UTILITY_PROCESS=$enable_mojo_media_in_utility_process",
-    "ENABLE_STANDALONE_CDM_SERVICE=$enable_standalone_cdm_service",
   ]
 }