media: Fix MojoAudioDecoder reinitialization

media/mojo/clients/mojo_audio_decoder.cc

Index: media/mojo/clients/mojo_audio_decoder.cc
diff --git a/media/mojo/clients/mojo_audio_decoder.cc b/media/mojo/clients/mojo_audio_decoder.cc
index 9347714173fa9f1ba0c09545a8f118fbe518fc03..2785570ea89a942efa9f732e46de749594a9969b 100644
--- a/media/mojo/clients/mojo_audio_decoder.cc
+++ b/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_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)
@@ -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)));
 }
 
@@ -133,6 +127,25 @@ bool MojoAudioDecoder::NeedsBitstreamConversion() const {
   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());
@@ -164,7 +177,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);