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 80becdbe..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 f6b1268e..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
