MediaRenderer: Returns initialization result in the init callback.

Currently the initialization result is returned in the OnError() call. However,
the init_cb is returned on the MediaRenderer pipe, while OnError() is called on
the client (MediaRendererClient) pipe. The order of these two calls are not
deterministic.

This CL makes sure the initialization result is always returned in the init
callback. This is also more consistent with the media::Renderer interface.

BUG=509801
TEST=All existing tests pass.

Review URL: https://codereview.chromium.org/1232703005

Cr-Commit-Position: refs/heads/master@{#338581}

6 files changed, 36 insertions, 37 deletions

diff --git a/media/mojo/interfaces/media_renderer.mojom b/media/mojo/interfaces/media_renderer.mojom
index cbf558f..798e809 100644
--- a/media/mojo/interfaces/media_renderer.mojom
+++ b/media/mojo/interfaces/media_renderer.mojom
@@ -9,12 +9,10 @@ import "media/mojo/interfaces/media_types.mojom";
 
 interface MediaRenderer {
   // Initializes the Renderer with one or both of an audio and video stream,
-  // calling back upon completion.
-  // NOTE: If an error occurs, MediaRendererClient::OnError() will be called
-  // before the callback is executed.
+  // executing the callback with whether the initialization succeeded.
   Initialize(MediaRendererClient client,
              DemuxerStream? audio,
-             DemuxerStream? video) => ();
+             DemuxerStream? video) => (bool success);
 
   // Discards any buffered data, executing callback when completed.
   // NOTE: If an error occurs, MediaRendererClient::OnError() can be called
diff --git a/media/mojo/services/media_apptest.cc b/media/mojo/services/media_apptest.cc
index 3741f37..051fee3 100644
--- a/media/mojo/services/media_apptest.cc
+++ b/media/mojo/services/media_apptest.cc
@@ -83,9 +83,10 @@ class MediaAppTest : public mojo::test::ApplicationTestBase {
         base::Bind(&MediaAppTest::OnCdmInitialized, base::Unretained(this)));
   }
 
-  MOCK_METHOD0(OnRendererInitialized, void());
+  MOCK_METHOD1(OnRendererInitialized, void(bool));
 
-  void InitializeRenderer(const VideoDecoderConfig& video_config) {
+  void InitializeRenderer(const VideoDecoderConfig& video_config,
+                          bool expected_result) {
     video_demuxer_stream_.set_video_decoder_config(video_config);
 
     mojo::DemuxerStreamPtr video_stream;
@@ -94,9 +95,9 @@ class MediaAppTest : public mojo::test::ApplicationTestBase {
     mojo::MediaRendererClientPtr client_ptr;
     media_renderer_client_binding_.Bind(GetProxy(&client_ptr));
 
-    EXPECT_CALL(*this, OnRendererInitialized())
+    EXPECT_CALL(*this, OnRendererInitialized(expected_result))
         .Times(Exactly(1))
-        .WillOnce(Invoke(run_loop_.get(), &base::RunLoop::Quit));
+        .WillOnce(InvokeWithoutArgs(run_loop_.get(), &base::RunLoop::Quit));
     media_renderer_->Initialize(client_ptr.Pass(), nullptr, video_stream.Pass(),
                                 base::Bind(&MediaAppTest::OnRendererInitialized,
                                            base::Unretained(this)));
@@ -133,13 +134,12 @@ TEST_F(MediaAppTest, InitializeCdm_InvalidKeySystem) {
 }
 
 TEST_F(MediaAppTest, InitializeRenderer_Success) {
-  InitializeRenderer(TestVideoConfig::Normal());
+  InitializeRenderer(TestVideoConfig::Normal(), true);
   run_loop_->Run();
 }
 
 TEST_F(MediaAppTest, InitializeRenderer_InvalidConfig) {
-  EXPECT_CALL(media_renderer_client_, OnError());
-  InitializeRenderer(TestVideoConfig::Invalid());
+  InitializeRenderer(TestVideoConfig::Invalid(), false);
   run_loop_->Run();
 }
 
diff --git a/media/mojo/services/mojo_renderer_impl.cc b/media/mojo/services/mojo_renderer_impl.cc
index 2791c02..31681a5 100644
--- a/media/mojo/services/mojo_renderer_impl.cc
+++ b/media/mojo/services/mojo_renderer_impl.cc
@@ -188,6 +188,7 @@ void MojoRendererImpl::OnEnded() {
 
 void MojoRendererImpl::OnError() {
   DVLOG(1) << __FUNCTION__;
+  DCHECK(init_cb_.is_null());
 
   if (!task_runner_->BelongsToCurrentThread()) {
     task_runner_->PostTask(
@@ -198,17 +199,16 @@ void MojoRendererImpl::OnError() {
 
   // TODO(tim): Should we plumb error code from remote renderer?
   // http://crbug.com/410451.
-  if (init_cb_.is_null())  // We have initialized already.
-    error_cb_.Run(PIPELINE_ERROR_DECODE);
-  else
-    init_cb_.Run(PIPELINE_ERROR_COULD_NOT_RENDER);
+  error_cb_.Run(PIPELINE_ERROR_DECODE);
 }
 
-void MojoRendererImpl::OnInitialized() {
+void MojoRendererImpl::OnInitialized(bool success) {
   DVLOG(1) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
-  if (!init_cb_.is_null())
-    base::ResetAndReturn(&init_cb_).Run(PIPELINE_OK);
+  DCHECK(!init_cb_.is_null());
+
+  base::ResetAndReturn(&init_cb_)
+      .Run(success ? PIPELINE_OK : PIPELINE_ERROR_INITIALIZATION_FAILED);
 }
 
 }  // namespace media
diff --git a/media/mojo/services/mojo_renderer_impl.h b/media/mojo/services/mojo_renderer_impl.h
index 7b955a2..bc36d18 100644
--- a/media/mojo/services/mojo_renderer_impl.h
+++ b/media/mojo/services/mojo_renderer_impl.h
@@ -58,7 +58,7 @@ class MojoRendererImpl : public Renderer, public mojo::MediaRendererClient {
 
  private:
   // Called when |remote_media_renderer_| has finished initializing.
-  void OnInitialized();
+  void OnInitialized(bool success);
 
   // Task runner used to execute pipeline tasks.
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
diff --git a/media/mojo/services/mojo_renderer_service.cc b/media/mojo/services/mojo_renderer_service.cc
index d1bf751e..8152307 100644
--- a/media/mojo/services/mojo_renderer_service.cc
+++ b/media/mojo/services/mojo_renderer_service.cc
@@ -69,10 +69,11 @@ MojoRendererService::MojoRendererService(
 MojoRendererService::~MojoRendererService() {
 }
 
-void MojoRendererService::Initialize(mojo::MediaRendererClientPtr client,
-                                     mojo::DemuxerStreamPtr audio,
-                                     mojo::DemuxerStreamPtr video,
-                                     const mojo::Closure& callback) {
+void MojoRendererService::Initialize(
+    mojo::MediaRendererClientPtr client,
+    mojo::DemuxerStreamPtr audio,
+    mojo::DemuxerStreamPtr video,
+    const mojo::Callback<void(bool)>& callback) {
   DVLOG(1) << __FUNCTION__;
   DCHECK_EQ(state_, STATE_UNINITIALIZED);
   client_ = client.Pass();
@@ -129,7 +130,8 @@ void MojoRendererService::SetCdm(int32_t cdm_id,
                                             weak_this_, callback));
 }
 
-void MojoRendererService::OnStreamReady(const mojo::Closure& callback) {
+void MojoRendererService::OnStreamReady(
+    const mojo::Callback<void(bool)>& callback) {
   DCHECK_EQ(state_, STATE_INITIALIZING);
 
   renderer_->Initialize(
@@ -144,20 +146,19 @@ void MojoRendererService::OnStreamReady(const mojo::Closure& callback) {
 }
 
 void MojoRendererService::OnRendererInitializeDone(
-    const mojo::Closure& callback, PipelineStatus status) {
+    const mojo::Callback<void(bool)>& callback,
+    PipelineStatus status) {
   DVLOG(1) << __FUNCTION__;
+  DCHECK_EQ(state_, STATE_INITIALIZING);
 
-  if (status != PIPELINE_OK && state_ != STATE_ERROR)
-    OnError(status);
-
-  if (state_ == STATE_ERROR) {
-    renderer_.reset();
-  } else {
-    DCHECK_EQ(state_, STATE_INITIALIZING);
-    state_ = STATE_PLAYING;
+  if (status != PIPELINE_OK) {
+    state_ = STATE_ERROR;
+    callback.Run(false);
+    return;
   }
 
-  callback.Run();
+  state_ = STATE_PLAYING;
+  callback.Run(true);
 }
 
 void MojoRendererService::OnUpdateStatistics(const PipelineStatistics& stats) {
diff --git a/media/mojo/services/mojo_renderer_service.h b/media/mojo/services/mojo_renderer_service.h
index 085cea2..d087806 100644
--- a/media/mojo/services/mojo_renderer_service.h
+++ b/media/mojo/services/mojo_renderer_service.h
@@ -49,7 +49,7 @@ class MEDIA_EXPORT MojoRendererService
   void Initialize(mojo::MediaRendererClientPtr client,
                   mojo::DemuxerStreamPtr audio,
                   mojo::DemuxerStreamPtr video,
-                  const mojo::Closure& callback) final;
+                  const mojo::Callback<void(bool)>& callback) final;
   void Flush(const mojo::Closure& callback) final;
   void StartPlayingFrom(int64_t time_delta_usec) final;
   void SetPlaybackRate(double playback_rate) final;
@@ -67,10 +67,10 @@ class MEDIA_EXPORT MojoRendererService
 
   // Called when the DemuxerStreamProviderShim is ready to go (has a config,
   // pipe handle, etc) and can be handed off to a renderer for use.
-  void OnStreamReady(const mojo::Closure& callback);
+  void OnStreamReady(const mojo::Callback<void(bool)>& callback);
 
   // Called when |audio_renderer_| initialization has completed.
-  void OnRendererInitializeDone(const mojo::Closure& callback,
+  void OnRendererInitializeDone(const mojo::Callback<void(bool)>& callback,
                                 PipelineStatus status);
 
   // Callback executed by filters to update statistics.