Add DecryptingRendererFactory

Returning a DecryptingRenderer from the MojoRendererFactory introduced
some regressions:
- DecryptingRenderer crashes when used with URL media resources.
- MediaPlayerRendererClient sets up a delayed crash due to a static_cast
  into MojoRenderer*.

This CL moves the creation of DecryptingRenderer into its own factory.
The DecryptingRendererFactory is a small wrapper around
MojoRendererFactory.
This gets rid of both regressions, and makes it explicit where/when
DecryptingRenderers are used.

Bug: 919494, 919819
Change-Id: I52a091a13dd858ccb787e17fe1abacbb32ec3916
Reviewed-on: https://chromium-review.googlesource.com/c/1405814
Commit-Queue: Thomas Guilbert <tguilbert@chromium.org>
Reviewed-by: Xiaohan Wang <xhwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622166}
diff --git a/content/renderer/media/media_factory.cc b/content/renderer/media/media_factory.cc
index 80becdb..5e618b5 100644
--- a/content/renderer/media/media_factory.cc
+++ b/content/renderer/media/media_factory.cc
@@ -36,6 +36,7 @@
 #include "media/blink/webmediaplayer_impl.h"
 #include "media/filters/context_3d.h"
 #include "media/media_buildflags.h"
+#include "media/renderers/decrypting_renderer_factory.h"
 #include "media/renderers/default_decoder_factory.h"
 #include "media/renderers/default_renderer_factory.h"
 #include "media/video/gpu_video_accelerator_factories.h"
@@ -434,7 +435,7 @@
   // MediaPlayerRendererClientFactory setup.
   auto mojo_media_player_renderer_factory =
       std::make_unique<media::MojoRendererFactory>(
-          media_log, media::mojom::HostedRendererType::kMediaPlayer,
+          media::mojom::HostedRendererType::kMediaPlayer,
           media::MojoRendererFactory::GetGpuFactoriesCB(),
           GetMediaInterfaceFactory());
 
@@ -454,7 +455,7 @@
 
   // FlingingRendererClientFactory (FRCF) setup.
   auto mojo_flinging_factory = std::make_unique<media::MojoRendererFactory>(
-      media_log, media::mojom::HostedRendererType::kFlinging,
+      media::mojom::HostedRendererType::kFlinging,
       media::MojoRendererFactory::GetGpuFactoriesCB(),
       GetMediaInterfaceFactory());
 
@@ -495,13 +496,20 @@
   use_mojo_renderer_factory = true;
 #endif  // BUILDFLAG(ENABLE_RUNTIME_MEDIA_RENDERER_SELECTION)
   if (use_mojo_renderer_factory) {
+    auto mojo_renderer_factory = std::make_unique<media::MojoRendererFactory>(
+        media::mojom::HostedRendererType::kDefault,
+        base::Bind(&RenderThreadImpl::GetGpuFactories,
+                   base::Unretained(render_thread)),
+        GetMediaInterfaceFactory());
+
+    // The "default" MojoRendererFactory can be wrapped by a
+    // DecryptingRendererFactory without changing any behavior.
+    // TODO(tguilbert/xhwang): Add "FactoryType::DECRYPTING" if ever we need to
+    // distinguish between a "pure" and "decrypting" MojoRenderer.
     factory_selector->AddFactory(
         media::RendererFactorySelector::FactoryType::MOJO,
-        std::make_unique<media::MojoRendererFactory>(
-            media_log, media::mojom::HostedRendererType::kDefault,
-            base::Bind(&RenderThreadImpl::GetGpuFactories,
-                       base::Unretained(render_thread)),
-            GetMediaInterfaceFactory()));
+        std::make_unique<media::DecryptingRendererFactory>(
+            media_log, std::move(mojo_renderer_factory)));
 
     factory_selector->SetBaseFactoryType(
         media::RendererFactorySelector::FactoryType::MOJO);
diff --git a/media/mojo/clients/mojo_renderer_factory.cc b/media/mojo/clients/mojo_renderer_factory.cc
index 7dab77e..8107f20 100644
--- a/media/mojo/clients/mojo_renderer_factory.cc
+++ b/media/mojo/clients/mojo_renderer_factory.cc
@@ -17,12 +17,10 @@
 namespace media {
 
 MojoRendererFactory::MojoRendererFactory(
-    media::MediaLog* media_log,
     mojom::HostedRendererType type,
     const GetGpuFactoriesCB& get_gpu_factories_cb,
     media::mojom::InterfaceFactory* interface_factory)
-    : media_log_(media_log),
-      get_gpu_factories_cb_(get_gpu_factories_cb),
+    : get_gpu_factories_cb_(get_gpu_factories_cb),
       interface_factory_(interface_factory),
       hosted_renderer_type_(type) {
   DCHECK(interface_factory_);
@@ -51,12 +49,12 @@
         std::make_unique<VideoOverlayFactory>(get_gpu_factories_cb_.Run());
   }
 
-  // TODO(xhwang): use DecryptingRenderer in other renderer factories.
-  return std::make_unique<DecryptingRenderer>(
-      std::make_unique<MojoRenderer>(media_task_runner,
-                                     std::move(overlay_factory),
-                                     video_renderer_sink, GetRendererPtr()),
-      media_log_, media_task_runner);
+  // MediaPlayerRendererClientFactory depends on |this| always returning a MR,
+  // since it uses a static_cast to use some MojoRenderer specific interfaces.
+  // Therefore, |this| should never return anything else than a MojoRenderer.
+  return std::make_unique<MojoRenderer>(media_task_runner,
+                                        std::move(overlay_factory),
+                                        video_renderer_sink, GetRendererPtr());
 }
 
 mojom::RendererPtr MojoRendererFactory::GetRendererPtr() {
diff --git a/media/mojo/clients/mojo_renderer_factory.h b/media/mojo/clients/mojo_renderer_factory.h
index 3e9a6d3..91d3976 100644
--- a/media/mojo/clients/mojo_renderer_factory.h
+++ b/media/mojo/clients/mojo_renderer_factory.h
@@ -8,7 +8,6 @@
 #include <memory>
 
 #include "base/macros.h"
-#include "media/base/media_log.h"
 #include "media/base/renderer_factory.h"
 #include "media/mojo/interfaces/interface_factory.mojom.h"
 #include "media/mojo/interfaces/renderer.mojom.h"
@@ -22,13 +21,24 @@
 class GpuVideoAcceleratorFactories;
 
 // The default factory class for creating MojoRenderer.
+//
+// The MojoRenderer should be thought of as a pure communication layer between
+// media::Pipeline and a media::Renderer in a different process.
+//
+// Implementors of new media::Renderer types are encouraged to create small
+// wrapper factories that use MRF, rather than creating derived MojoRenderer
+// types, or extending MRF. See DecryptingRendererFactory and
+// MediaPlayerRendererClientFactory for examples of small wrappers around MRF.
+//
+// NOTE: MediaPlayerRendererClientFactory uses MojoRenderer specific methods,
+//       and uses a static_cast<MojoRenderer*> internally. |this| should
+//       never return anything but a MojoRenderer. See crbug.com/919494.
 class MojoRendererFactory : public RendererFactory {
  public:
   using GetGpuFactoriesCB = base::Callback<GpuVideoAcceleratorFactories*()>;
   using GetTypeSpecificIdCB = base::Callback<std::string()>;
 
-  MojoRendererFactory(media::MediaLog* media_log,
-                      mojom::HostedRendererType type,
+  MojoRendererFactory(mojom::HostedRendererType type,
                       const GetGpuFactoriesCB& get_gpu_factories_cb,
                       media::mojom::InterfaceFactory* interface_factory);
 
@@ -53,8 +63,6 @@
  private:
   mojom::RendererPtr GetRendererPtr();
 
-  media::MediaLog* const media_log_;
-
   GetGpuFactoriesCB get_gpu_factories_cb_;
   GetTypeSpecificIdCB get_type_specific_id_;
 
diff --git a/media/renderers/BUILD.gn b/media/renderers/BUILD.gn
index f6b1268..8c8bd60 100644
--- a/media/renderers/BUILD.gn
+++ b/media/renderers/BUILD.gn
@@ -15,6 +15,8 @@
     "audio_renderer_impl.h",
     "decrypting_renderer.cc",
     "decrypting_renderer.h",
+    "decrypting_renderer_factory.cc",
+    "decrypting_renderer_factory.h",
     "default_decoder_factory.cc",
     "default_decoder_factory.h",
     "default_renderer_factory.cc",
diff --git a/media/renderers/decrypting_renderer.cc b/media/renderers/decrypting_renderer.cc
index db7da79..509cd93 100644
--- a/media/renderers/decrypting_renderer.cc
+++ b/media/renderers/decrypting_renderer.cc
@@ -48,17 +48,13 @@
   DCHECK(media_resource);
   DCHECK(client);
 
+  // Using |this| with a MediaResource::Type::URL will result in a crash.
+  DCHECK_EQ(media_resource->GetType(), MediaResource::Type::STREAM);
+
   media_resource_ = media_resource;
   client_ = client;
   init_cb_ = std::move(init_cb);
 
-  // Using a DecryptingMediaResource when our MediaResource has a URL type will
-  // result in a crash.
-  if (media_resource_->GetType() == MediaResource::URL) {
-    InitializeRenderer(true);
-    return;
-  }
-
   bool has_encrypted_stream = HasEncryptedStream();
 
   // If we do not have a valid |cdm_context_| and there are encrypted streams we
@@ -165,8 +161,8 @@
     return;
   }
 
-  // |decrypting_media_resource_| is null when |media_resource_| has a URL type
-  // or when |cdm_context_| is null and there are no encrypted streams.
+  // |decrypting_media_resource_| when |cdm_context_| is null and there are no
+  // encrypted streams.
   MediaResource* const maybe_decrypting_media_resource =
       decrypting_media_resource_ ? decrypting_media_resource_.get()
                                  : media_resource_;
diff --git a/media/renderers/decrypting_renderer.h b/media/renderers/decrypting_renderer.h
index 6bd51fa2..582eaf9 100644
--- a/media/renderers/decrypting_renderer.h
+++ b/media/renderers/decrypting_renderer.h
@@ -29,6 +29,9 @@
 // implementation.
 //
 // All methods are pass-through except Initialize() and SetCdm().
+//
+// The caller must guarantee that DecryptingRenderer will never be initialized
+// with a |media_resource| of type MediaResource::Type::URL.
 class MEDIA_EXPORT DecryptingRenderer : public Renderer {
  public:
   DecryptingRenderer(
diff --git a/media/renderers/decrypting_renderer_factory.cc b/media/renderers/decrypting_renderer_factory.cc
new file mode 100644
index 0000000..42171d4
--- /dev/null
+++ b/media/renderers/decrypting_renderer_factory.cc
@@ -0,0 +1,34 @@
+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "media/renderers/decrypting_renderer_factory.h"
+
+#include "media/base/media_log.h"
+#include "media/renderers/decrypting_renderer.h"
+
+namespace media {
+
+DecryptingRendererFactory::DecryptingRendererFactory(
+    media::MediaLog* media_log,
+    std::unique_ptr<media::RendererFactory> renderer_factory)
+    : media_log_(media_log), renderer_factory_(std::move(renderer_factory)) {}
+
+DecryptingRendererFactory::~DecryptingRendererFactory() = default;
+
+std::unique_ptr<Renderer> DecryptingRendererFactory::CreateRenderer(
+    const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner,
+    const scoped_refptr<base::TaskRunner>& worker_task_runner,
+    AudioRendererSink* audio_renderer_sink,
+    VideoRendererSink* video_renderer_sink,
+    const RequestOverlayInfoCB& request_overlay_info_cb,
+    const gfx::ColorSpace& target_color_space) {
+  std::unique_ptr<media::Renderer> renderer = renderer_factory_->CreateRenderer(
+      media_task_runner, worker_task_runner, audio_renderer_sink,
+      video_renderer_sink, request_overlay_info_cb, target_color_space);
+
+  return std::make_unique<DecryptingRenderer>(std::move(renderer), media_log_,
+                                              media_task_runner);
+}
+
+}  // namespace media
diff --git a/media/renderers/decrypting_renderer_factory.h b/media/renderers/decrypting_renderer_factory.h
new file mode 100644
index 0000000..3f115c8
--- /dev/null
+++ b/media/renderers/decrypting_renderer_factory.h
@@ -0,0 +1,50 @@
+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef MEDIA_RENDERERS_DECRYPTING_RENDERER_FACTORY_H_
+#define MEDIA_RENDERERS_DECRYPTING_RENDERER_FACTORY_H_
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "media/base/media_export.h"
+#include "media/base/renderer_factory.h"
+
+namespace media {
+
+class MediaLog;
+
+// Simple RendererFactory wrapper class. It wraps any Renderer created by the
+// underlying factory, and returns it as a DecryptingRenderer.
+//
+// See DecryptingRenderer for more information.
+//
+// The caller must guarantee that the returned DecryptingRenderer will never
+// be initialized with a |media_resource| of type MediaResource::Type::URL.
+class MEDIA_EXPORT DecryptingRendererFactory : public RendererFactory {
+ public:
+  DecryptingRendererFactory(
+      MediaLog* media_log,
+      std::unique_ptr<media::RendererFactory> renderer_factory);
+  ~DecryptingRendererFactory() final;
+
+  // RendererFactory implementation.
+  std::unique_ptr<Renderer> CreateRenderer(
+      const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner,
+      const scoped_refptr<base::TaskRunner>& worker_task_runner,
+      AudioRendererSink* audio_renderer_sink,
+      VideoRendererSink* video_renderer_sink,
+      const RequestOverlayInfoCB& request_overlay_info_cb,
+      const gfx::ColorSpace& target_color_space) final;
+
+ private:
+  MediaLog* media_log_;
+
+  std::unique_ptr<media::RendererFactory> renderer_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(DecryptingRendererFactory);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_RENDERERS_DECRYPTING_RENDERER_FACTORY_H_
diff --git a/media/renderers/decrypting_renderer_unittest.cc b/media/renderers/decrypting_renderer_unittest.cc
index a452173..69634d8 100644
--- a/media/renderers/decrypting_renderer_unittest.cc
+++ b/media/renderers/decrypting_renderer_unittest.cc
@@ -265,16 +265,4 @@
   InitializeDecryptingRendererWithFalse();
 }
 
-TEST_F(DecryptingRendererTest, MediaResourceHasURLType) {
-  EXPECT_CALL(*renderer_, Initialize(_, _, _))
-      .WillOnce(RunCallback<2>(PIPELINE_OK));
-  EXPECT_CALL(renderer_init_cb_, Run(PIPELINE_OK));
-  EXPECT_CALL(media_resource_, GetType())
-      .WillRepeatedly(Return(MediaResource::URL));
-
-  decrypting_renderer_->Initialize(&media_resource_, &renderer_client_,
-                                   renderer_init_cb_.Get());
-  scoped_task_environment_.RunUntilIdle();
-}
-
 }  // namespace media