media: Fix MojoAudioDecoder reinitialization

BUG=679095
TEST=New test added.

Review-Url: https://codereview.chromium.org/2624213006
Cr-Commit-Position: refs/heads/master@{#444888}

media/mojo/clients/mojo_audio_decoder.cc

@@ -24,9 +24,7 @@ MojoAudioDecoder::MojoAudioDecoder(
     mojom::AudioDecoderPtr remote_decoder)
     : task_runner_(task_runner),
       remote_decoder_info_(remote_decoder.PassInterface()),
-      client_binding_(this),
-      has_connection_error_(false),
-      needs_bitstream_conversion_(false) {
+      client_binding_(this) {
   DVLOG(1) << __func__;
 }
 
@@ -45,8 +43,14 @@ void MojoAudioDecoder::Initialize(const AudioDecoderConfig& config,
   DVLOG(1) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
-  // Bind |remote_decoder_| to the |task_runner_|.
-  remote_decoder_.Bind(std::move(remote_decoder_info_));
+  if (!remote_decoder_.is_bound())
+    BindRemoteDecoder();
+
+  // This could happen during reinitialization.
+  if (remote_decoder_.encountered_error()) {
+    task_runner_->PostTask(FROM_HERE, base::Bind(init_cb, false));
+    return;
+  }
 
   // Fail immediately if the stream is encrypted but |cdm_context| is invalid.
   int cdm_id = (config.is_encrypted() && cdm_context)
@@ -58,23 +62,13 @@ void MojoAudioDecoder::Initialize(const AudioDecoderConfig& config,
     return;
   }
 
-  // Otherwise, set an error handler to catch the connection error.
-  // Using base::Unretained(this) is safe because |this| owns |remote_decoder_|,
-  // and the error handler can't be invoked once |remote_decoder_| is destroyed.
-  remote_decoder_.set_connection_error_handler(
-      base::Bind(&MojoAudioDecoder::OnConnectionError, base::Unretained(this)));
-
   init_cb_ = init_cb;
   output_cb_ = output_cb;
 
-  mojom::AudioDecoderClientAssociatedPtrInfo client_ptr_info;
-  client_binding_.Bind(&client_ptr_info, remote_decoder_.associated_group());
-
   // Using base::Unretained(this) is safe because |this| owns |remote_decoder_|,
   // and the callback won't be dispatched if |remote_decoder_| is destroyed.
   remote_decoder_->Initialize(
-      std::move(client_ptr_info), mojom::AudioDecoderConfig::From(config),
-      cdm_id,
+      mojom::AudioDecoderConfig::From(config), cdm_id,
       base::Bind(&MojoAudioDecoder::OnInitialized, base::Unretained(this)));
 }
 
@@ -83,7 +77,7 @@ void MojoAudioDecoder::Decode(const scoped_refptr<DecoderBuffer>& media_buffer,
   DVLOG(3) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
-  if (has_connection_error_) {
+  if (remote_decoder_.encountered_error()) {
     task_runner_->PostTask(FROM_HERE,
                            base::Bind(decode_cb, DecodeStatus::DECODE_ERROR));
     return;
@@ -109,7 +103,7 @@ void MojoAudioDecoder::Reset(const base::Closure& closure) {
   DVLOG(2) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
-  if (has_connection_error_) {
+  if (remote_decoder_.encountered_error()) {
     if (!decode_cb_.is_null()) {
       task_runner_->PostTask(FROM_HERE,
                              base::Bind(base::ResetAndReturn(&decode_cb_),
@@ -133,6 +127,23 @@ bool MojoAudioDecoder::NeedsBitstreamConversion() const {
   return needs_bitstream_conversion_;
 }
 
+void MojoAudioDecoder::BindRemoteDecoder() {
+  DVLOG(1) << __func__;
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  remote_decoder_.Bind(std::move(remote_decoder_info_));
+
+  // Using base::Unretained(this) is safe because |this| owns |remote_decoder_|,
+  // and the error handler can't be invoked once |remote_decoder_| is destroyed.
+  remote_decoder_.set_connection_error_handler(
+      base::Bind(&MojoAudioDecoder::OnConnectionError, base::Unretained(this)));
+
+  mojom::AudioDecoderClientAssociatedPtrInfo client_ptr_info;
+  client_binding_.Bind(&client_ptr_info, remote_decoder_.associated_group());
+
+  remote_decoder_->Construct(std::move(client_ptr_info));
+}
+
 void MojoAudioDecoder::OnBufferDecoded(mojom::AudioBufferPtr buffer) {
   DVLOG(1) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
@@ -143,8 +154,7 @@ void MojoAudioDecoder::OnBufferDecoded(mojom::AudioBufferPtr buffer) {
 void MojoAudioDecoder::OnConnectionError() {
   DVLOG(1) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
-
-  has_connection_error_ = true;
+  DCHECK(remote_decoder_.encountered_error());
 
   if (!init_cb_.is_null()) {
     base::ResetAndReturn(&init_cb_).Run(false);
@@ -164,7 +174,7 @@ void MojoAudioDecoder::OnInitialized(bool success,
 
   needs_bitstream_conversion_ = needs_bitstream_conversion;
 
-  if (success) {
+  if (success && !mojo_decoder_buffer_writer_) {
     mojo::ScopedDataPipeConsumerHandle remote_consumer_handle;
     mojo_decoder_buffer_writer_ = MojoDecoderBufferWriter::Create(
         DemuxerStream::AUDIO, &remote_consumer_handle);

media/mojo/clients/mojo_audio_decoder.h

@@ -45,6 +45,8 @@ class MojoAudioDecoder : public AudioDecoder, public mojom::AudioDecoderClient {
   void OnBufferDecoded(mojom::AudioBufferPtr buffer) final;
 
  private:
+  void BindRemoteDecoder();
+
   // Callback for connection error on |remote_decoder_|.
   void OnConnectionError();
 
@@ -80,12 +82,9 @@ class MojoAudioDecoder : public AudioDecoder, public mojom::AudioDecoderClient {
   DecodeCB decode_cb_;
   base::Closure reset_cb_;
 
-  // Flag that is set if we got connection error. Never cleared.
-  bool has_connection_error_;
-
   // Flag telling whether this decoder requires bitstream conversion.
   // Passed from |remote_decoder_| as a result of its initialization.
-  bool needs_bitstream_conversion_;
+  bool needs_bitstream_conversion_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(MojoAudioDecoder);
 };

media/mojo/clients/mojo_audio_decoder_unittest.cc

@@ -108,11 +108,13 @@ class MojoAudioDecoderTest : public ::testing::Test {
     mock_audio_decoder_ = mock_audio_decoder.get();
 
     EXPECT_CALL(*mock_audio_decoder_, Initialize(_, _, _, _))
-        .WillOnce(DoAll(SaveArg<3>(&output_cb_), RunCallback<2>(true)));
+        .WillRepeatedly(DoAll(SaveArg<3>(&output_cb_), RunCallback<2>(true)));
     EXPECT_CALL(*mock_audio_decoder_, Decode(_, _))
         .WillRepeatedly(
             DoAll(InvokeWithoutArgs(this, &MojoAudioDecoderTest::ReturnOutput),
                   RunCallback<1>(DecodeStatus::OK)));
+    EXPECT_CALL(*mock_audio_decoder_, Reset(_))
+        .WillRepeatedly(RunCallback<0>());
 
     mojo::MakeStrongBinding(base::MakeUnique<MojoAudioDecoderService>(
                                 mojo_cdm_service_context_.GetWeakPtr(),
@@ -139,6 +141,16 @@ class MojoAudioDecoderTest : public ::testing::Test {
 
   void Initialize() { InitializeAndExpect(true); }
 
+  void Reset() {
+    DVLOG(1) << __func__;
+    EXPECT_CALL(*this, OnReset())
+        .WillOnce(InvokeWithoutArgs(this, &MojoAudioDecoderTest::QuitLoop));
+
+    mojo_audio_decoder_->Reset(
+        base::Bind(&MojoAudioDecoderTest::OnReset, base::Unretained(this)));
+    RunLoop();
+  }
+
   void ReturnOutput() {
     for (int i = 0; i < kOutputPerDecode; ++i) {
       scoped_refptr<AudioBuffer> audio_buffer = MakeAudioBuffer<float>(
@@ -211,6 +223,15 @@ TEST_F(MojoAudioDecoderTest, Initialize_Success) {
   Initialize();
 }
 
+TEST_F(MojoAudioDecoderTest, Reinitialize_Success) {
+  Initialize();
+  DecodeMultipleTimes(10);
+  Reset();
+
+  // Reinitialize MojoAudioDecoder.
+  Initialize();
+}
+
 // Makes sure all callbacks and client calls are called in order. See
 // http://crbug.com/646054
 TEST_F(MojoAudioDecoderTest, Decode_MultipleTimes) {

media/mojo/interfaces/audio_decoder.mojom

@@ -7,12 +7,18 @@ module media.mojom;
 import "media/mojo/interfaces/media_types.mojom";
 
 interface AudioDecoder {
+  // Initialize the decoder. This must be called before any other method.
+  //
+  // TODO(sandersd): Rename to Initialize() if/when
+  // media::AudioDecoder::Initialize() is renamed to Configure().
+  Construct(associated AudioDecoderClient client);
+
   // Initializes the AudioDecoder with the audio codec configuration and CDM id.
   // For the unencrypted streams the |cdm_id| is ignored. Executed the callback
   // with whether the initialization succeeded, and whether the pipeline needs
   // bitstream conversion.
-  Initialize(associated AudioDecoderClient client, AudioDecoderConfig config,
-             int32 cdm_id) => (bool success, bool needs_bitstream_conversion);
+  Initialize(AudioDecoderConfig config, int32 cdm_id)
+    => (bool success, bool needs_bitstream_conversion);
 
   // Establishes data connection. Should be called before Decode().
   SetDataSource(handle<data_pipe_consumer> receive_pipe);

media/mojo/services/mojo_audio_decoder_service.cc

@@ -26,8 +26,13 @@ MojoAudioDecoderService::MojoAudioDecoderService(
 
 MojoAudioDecoderService::~MojoAudioDecoderService() {}
 
+void MojoAudioDecoderService::Construct(
+    mojom::AudioDecoderClientAssociatedPtrInfo client) {
+  DVLOG(1) << __func__;
+  client_.Bind(std::move(client));
+}
+
 void MojoAudioDecoderService::Initialize(
-    mojom::AudioDecoderClientAssociatedPtrInfo client,
     mojom::AudioDecoderConfigPtr config,
     int32_t cdm_id,
     const InitializeCallback& callback) {
@@ -59,8 +64,6 @@ void MojoAudioDecoderService::Initialize(
     }
   }
 
-  client_.Bind(std::move(client));
-
   decoder_->Initialize(
       config.To<media::AudioDecoderConfig>(), cdm_context,
       base::Bind(&MojoAudioDecoderService::OnInitialized, weak_this_, callback,

media/mojo/services/mojo_audio_decoder_service.h

@@ -31,8 +31,8 @@ class MEDIA_MOJO_EXPORT MojoAudioDecoderService
   ~MojoAudioDecoderService() final;
 
   // mojom::AudioDecoder implementation
-  void Initialize(mojom::AudioDecoderClientAssociatedPtrInfo client,
-                  mojom::AudioDecoderConfigPtr config,
+  void Construct(mojom::AudioDecoderClientAssociatedPtrInfo client) final;
+  void Initialize(mojom::AudioDecoderConfigPtr config,
                   int32_t cdm_id,
                   const InitializeCallback& callback) final;