media: Fix MojoAudioDecoder reinitialization

media/mojo/clients/mojo_audio_decoder.cc

 // Copyright 2016 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/mojo/clients/mojo_audio_decoder.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback_helpers.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "media/base/audio_buffer.h"
 #include "media/base/cdm_context.h"
 #include "media/base/demuxer_stream.h"
 #include "media/mojo/common/media_type_converters.h"
 #include "media/mojo/common/mojo_decoder_buffer_converter.h"
 
 namespace media {
 
 MojoAudioDecoder::MojoAudioDecoder(
     scoped_refptr<base::SingleThreadTaskRunner> task_runner,
     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__;
 }
 
 MojoAudioDecoder::~MojoAudioDecoder() {
   DVLOG(1) << __func__;
 }
 
 std::string MojoAudioDecoder::GetDisplayName() const {
   return "MojoAudioDecoder";
 }
 
 void MojoAudioDecoder::Initialize(const AudioDecoderConfig& config,
                                   CdmContext* cdm_context,
                                   const InitCB& init_cb,
                                   const OutputCB& output_cb) {
   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_bound_)

[dcheng, 2017/01/17 19:48:10]

One thing I'd like to understand is why we have these bools
(remote_decoder_bound_ and has_connection_error_) instead of just checking the
status of the InterfacePtr object. It seems like they could get out of sync, and
it's not 100% clear what they refer too either (in the case of
has_connection_error_). I would prefer we just check is_bound() on the
InterfacePtr directly.

[sandersd (OOO until July 31), 2017/01/17 20:03:05]

In this case (and also the code it is copied from), it's more about tracking
whether the method (BindRemoteDecoder()) was called, and less about the state of
the binding. Unbinding the interface should not make us call the method again,
for example.

That said, they currently have the same meaning, so it might be an improvement
to simplify it down to one concept until such time as the meanings diverge.

[sandersd (OOO until July 31), 2017/01/17 20:12:28]

After reading through the implementation, I'm not sure if an InterfacePtr can
become unbound on its own. If that's possible, are there any uses of the
InterfacePtr that would become invalid at the same time?

[dcheng, 2017/01/18 01:39:18]

I confirmed with the mojo team, but a connection error does not unbind.
Since both of these bits can be tracked with information already available on
InterfacePtr, I think it would be better if we just use what Mojo already has.
WDYT?

[xhwang, 2017/01/18 21:46:58]

Thanks for the suggestions. This makes sense to me. Done.

+    BindRemoteDecoder();
+
+  // This could happen during reinitialization.
+  if (has_connection_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)
                    ? cdm_context->GetCdmId()
                    : CdmContext::kInvalidCdmId;
 
   if (config.is_encrypted() && CdmContext::kInvalidCdmId == cdm_id) {
     task_runner_->PostTask(FROM_HERE, base::Bind(init_cb, false));
     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)));
 }
 
 void MojoAudioDecoder::Decode(const scoped_refptr<DecoderBuffer>& media_buffer,
                               const DecodeCB& decode_cb) {
   DVLOG(3) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   if (has_connection_error_) {
     task_runner_->PostTask(FROM_HERE,
@@ skipping 38 matching lines @@
       base::Bind(&MojoAudioDecoder::OnResetDone, base::Unretained(this)));
 }
 
 bool MojoAudioDecoder::NeedsBitstreamConversion() const {
   DVLOG(1) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   return needs_bitstream_conversion_;
 }
 
+void MojoAudioDecoder::BindRemoteDecoder() {
+  DVLOG(1) << __func__;
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  DCHECK(!remote_decoder_bound_);
+
+  remote_decoder_bound_ = true;
+  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());
 
   output_cb_.Run(buffer.To<scoped_refptr<AudioBuffer>>());
 }
 
 void MojoAudioDecoder::OnConnectionError() {
   DVLOG(1) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
@@ skipping 11 matching lines @@
     base::ResetAndReturn(&reset_cb_).Run();
 }
 
 void MojoAudioDecoder::OnInitialized(bool success,
                                      bool needs_bitstream_conversion) {
   DVLOG(1) << __func__ << ": success:" << success;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   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);
     // Pass consumer end to |remote_decoder_|.
     remote_decoder_->SetDataSource(std::move(remote_consumer_handle));
   }
 
   base::ResetAndReturn(&init_cb_).Run(success);
 }
 
@@ skipping 10 matching lines @@
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   // For pending decodes OnDecodeStatus() should arrive before OnResetDone().
   DCHECK(decode_cb_.is_null());
 
   DCHECK(!reset_cb_.is_null());
   base::ResetAndReturn(&reset_cb_).Run();
 }
 
 }  // namespace media