Clean up media::DecryptingDemuxerStream.

media/filters/decrypting_demuxer_stream.cc

 // Copyright (c) 2012 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/filters/decrypting_demuxer_stream.h"
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/location.h"
 #include "base/logging.h"
@@ skipping 19 matching lines @@
           (stream->type() == DemuxerStream::VIDEO &&
            stream->video_decoder_config().IsValidConfig() &&
            stream->video_decoder_config().is_encrypted()));
 }
 
 DecryptingDemuxerStream::DecryptingDemuxerStream(
     const scoped_refptr<base::MessageLoopProxy>& message_loop,
     const SetDecryptorReadyCB& set_decryptor_ready_cb)
     : message_loop_(message_loop),
       state_(kUninitialized),
-      stream_type_(UNKNOWN),
       set_decryptor_ready_cb_(set_decryptor_ready_cb),
       decryptor_(NULL),
       key_added_while_decrypt_pending_(false) {
 }
 
 void DecryptingDemuxerStream::Initialize(
     const scoped_refptr<DemuxerStream>& stream,
     const PipelineStatusCB& status_cb) {
-  if (!message_loop_->BelongsToCurrentThread()) {
-    message_loop_->PostTask(FROM_HERE, base::Bind(
-        &DecryptingDemuxerStream::DoInitialize, this, stream, status_cb));
-    return;
-  }
-  DoInitialize(stream, status_cb);
+  DVLOG(2) << "Initialize()";
+  DCHECK(message_loop_->BelongsToCurrentThread());
+  DCHECK_EQ(state_, kUninitialized) << state_;
+
+  DCHECK(!demuxer_stream_);
+  demuxer_stream_ = stream;
+  init_cb_ = status_cb;
+
+  InitializeDecoderConfig();
+
+  state_ = kDecryptorRequested;
+  set_decryptor_ready_cb_.Run(
+      BIND_TO_LOOP(&DecryptingDemuxerStream::SetDecryptor));
 }
 
 void DecryptingDemuxerStream::Read(const ReadCB& read_cb) {
   DVLOG(3) << "Read()";
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK_EQ(state_, kIdle) << state_;
   DCHECK(!read_cb.is_null());
   CHECK(read_cb_.is_null()) << "Overlapping reads are not supported.";
 
   read_cb_ = read_cb;
   state_ = kPendingDemuxerRead;
   demuxer_stream_->Read(
       base::Bind(&DecryptingDemuxerStream::DecryptBuffer, this));
 }
 
 void DecryptingDemuxerStream::Reset(const base::Closure& closure) {
-  if (!message_loop_->BelongsToCurrentThread()) {
-    message_loop_->PostTask(FROM_HERE, base::Bind(
-        &DecryptingDemuxerStream::Reset, this, closure));
-    return;
-  }
-
   DVLOG(2) << "Reset() - state: " << state_;
+  DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK(state_ != kUninitialized && state_ != kDecryptorRequested) << state_;
   DCHECK(init_cb_.is_null());  // No Reset() during pending initialization.
   DCHECK(reset_cb_.is_null());
 
   reset_cb_ = BindToCurrentLoop(closure);
 
   decryptor_->CancelDecrypt(GetDecryptorStreamType());
 
   // Reset() cannot complete if the read callback is still pending.
   // Defer the resetting process in this case. The |reset_cb_| will be fired
   // after the read callback is fired - see DoDecryptBuffer() and
   // DoDeliverBuffer().
   if (state_ == kPendingDemuxerRead || state_ == kPendingDecrypt) {
     DCHECK(!read_cb_.is_null());
     return;
   }
 
   if (state_ == kWaitingForKey) {
     DCHECK(!read_cb_.is_null());
     pending_buffer_to_decrypt_ = NULL;
     base::ResetAndReturn(&read_cb_).Run(kAborted, NULL);
   }
 
   DCHECK(read_cb_.is_null());
   DoReset();
 }
 
 const AudioDecoderConfig& DecryptingDemuxerStream::audio_decoder_config() {
   DCHECK(state_ != kUninitialized && state_ != kDecryptorRequested) << state_;
-  CHECK_EQ(stream_type_, AUDIO);
+  CHECK_EQ(demuxer_stream_->type(), AUDIO);

[ddorwin, 2013/04/17 21:45:47]

Do we need to protect against demuxer_stream_ being NULL in all these cases or
is that programming error?

[xhwang, 2013/04/17 22:14:47]

I think audio_decoder_config should only be called after the DemuxerStream is
successfully initialized. So we should be fine here.

   return *audio_config_;
 }
 
 const VideoDecoderConfig& DecryptingDemuxerStream::video_decoder_config() {
   DCHECK(state_ != kUninitialized && state_ != kDecryptorRequested) << state_;
-  CHECK_EQ(stream_type_, VIDEO);
+  CHECK_EQ(demuxer_stream_->type(), VIDEO);
   return *video_config_;
 }
 
 DemuxerStream::Type DecryptingDemuxerStream::type() {
   DCHECK(state_ != kUninitialized && state_ != kDecryptorRequested) << state_;
-  return stream_type_;
+  return demuxer_stream_->type();
 }
 
 void DecryptingDemuxerStream::EnableBitstreamConverter() {
   demuxer_stream_->EnableBitstreamConverter();
 }
 
 DecryptingDemuxerStream::~DecryptingDemuxerStream() {}
 
-void DecryptingDemuxerStream::DoInitialize(
-    const scoped_refptr<DemuxerStream>& stream,
-    const PipelineStatusCB& status_cb) {
-  DVLOG(2) << "DoInitialize()";
-  DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK_EQ(state_, kUninitialized) << state_;
-
-  DCHECK(!demuxer_stream_);
-  demuxer_stream_ = stream;
-  stream_type_ = stream->type();
-  init_cb_ = status_cb;
-
-  SetDecoderConfig(stream);
-
-  state_ = kDecryptorRequested;
-  set_decryptor_ready_cb_.Run(
-      BIND_TO_LOOP(&DecryptingDemuxerStream::SetDecryptor));
-}
-
 void DecryptingDemuxerStream::SetDecryptor(Decryptor* decryptor) {
   DVLOG(2) << "SetDecryptor()";
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK_EQ(state_, kDecryptorRequested) << state_;
   DCHECK(!init_cb_.is_null());
   DCHECK(!set_decryptor_ready_cb_.is_null());
 
   set_decryptor_ready_cb_.Reset();
   decryptor_ = decryptor;
 
   decryptor_->RegisterNewKeyCB(
       GetDecryptorStreamType(),
       BIND_TO_LOOP(&DecryptingDemuxerStream::OnKeyAdded));
 
   state_ = kIdle;
   base::ResetAndReturn(&init_cb_).Run(PIPELINE_OK);
 }
 
 void DecryptingDemuxerStream::DecryptBuffer(
     DemuxerStream::Status status,
     const scoped_refptr<DecoderBuffer>& buffer) {
-  // In theory, we don't need to force post the task here, because we do a
-  // force task post in DeliverBuffer(). Therefore, even if
-  // demuxer_stream_->Read() execute the read callback on the same execution
-  // stack we are still fine. But it looks like a force post task makes the
-  // logic more understandable and manageable, so why not?
-  message_loop_->PostTask(FROM_HERE, base::Bind(
-      &DecryptingDemuxerStream::DoDecryptBuffer, this, status, buffer));
-}
-
-void DecryptingDemuxerStream::DoDecryptBuffer(
-    DemuxerStream::Status status,
-    const scoped_refptr<DecoderBuffer>& buffer) {
-  DVLOG(3) << "DoDecryptBuffer()";
+  DVLOG(3) << "DecryptBuffer()";
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK_EQ(state_, kPendingDemuxerRead) << state_;
   DCHECK(!read_cb_.is_null());
   DCHECK_EQ(buffer != NULL, status == kOk) << status;
 
   if (!reset_cb_.is_null()) {
     base::ResetAndReturn(&read_cb_).Run(kAborted, NULL);
     DoReset();
     return;
   }
 
   if (status == kAborted) {
     DVLOG(2) << "DoDecryptBuffer() - kAborted.";
     state_ = kIdle;
     base::ResetAndReturn(&read_cb_).Run(kAborted, NULL);
     return;
   }
 
   if (status == kConfigChanged) {
     DVLOG(2) << "DoDecryptBuffer() - kConfigChanged.";
-    DCHECK_EQ(demuxer_stream_->type(), stream_type_);
+    DCHECK_EQ(demuxer_stream_->type() == AUDIO, audio_config_ != NULL);
+    DCHECK_EQ(demuxer_stream_->type() == VIDEO, video_config_ != NULL);
 
     // Update the decoder config, which the decoder will use when it is notified
     // of kConfigChanged.
-    SetDecoderConfig(demuxer_stream_);
+    InitializeDecoderConfig();
     state_ = kIdle;
     base::ResetAndReturn(&read_cb_).Run(kConfigChanged, NULL);
     return;
   }
 
   if (buffer->IsEndOfStream()) {
     DVLOG(2) << "DoDecryptBuffer() - EOS buffer.";
     state_ = kIdle;
     base::ResetAndReturn(&read_cb_).Run(status, buffer);
     return;
   }
 
   pending_buffer_to_decrypt_ = buffer;
   state_ = kPendingDecrypt;
   DecryptPendingBuffer();
 }
 
 void DecryptingDemuxerStream::DecryptPendingBuffer() {
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK_EQ(state_, kPendingDecrypt) << state_;
   decryptor_->Decrypt(
       GetDecryptorStreamType(),
       pending_buffer_to_decrypt_,
-      base::Bind(&DecryptingDemuxerStream::DeliverBuffer, this));
+      BIND_TO_LOOP(&DecryptingDemuxerStream::DeliverBuffer));

[xhwang, 2013/04/17 22:14:47]

Now I feel bad about AesDecryptor firing callbacks synchronouly...

 }
 
 void DecryptingDemuxerStream::DeliverBuffer(
     Decryptor::Status status,
     const scoped_refptr<DecoderBuffer>& decrypted_buffer) {
-  // We need to force task post here because the DecryptCB can be executed
-  // synchronously in Reset(). Instead of using more complicated logic in
-  // those function to fix it, why not force task post here to make everything
-  // simple and clear?
-  message_loop_->PostTask(FROM_HERE, base::Bind(
-      &DecryptingDemuxerStream::DoDeliverBuffer, this,
-      status, decrypted_buffer));
-}
-
-void DecryptingDemuxerStream::DoDeliverBuffer(
-    Decryptor::Status status,
-    const scoped_refptr<DecoderBuffer>& decrypted_buffer) {
-  DVLOG(3) << "DoDeliverBuffer() - status: " << status;
+  DVLOG(3) << "DeliverBuffer() - status: " << status;

[ddorwin, 2013/04/17 21:45:47]

Synchronous calls from Reset() are no longer a concern? Just confirming.

[xhwang, 2013/04/17 22:14:47]

The BIND_TO_LOOP force posts the DecryptCB so we should be good here.

   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK_EQ(state_, kPendingDecrypt) << state_;
   DCHECK_NE(status, Decryptor::kNeedMoreData);
   DCHECK(!read_cb_.is_null());
   DCHECK(pending_buffer_to_decrypt_);
 
   bool need_to_try_again_if_nokey = key_added_while_decrypt_pending_;
   key_added_while_decrypt_pending_ = false;
 
   if (!reset_cb_.is_null()) {
@@ skipping 46 matching lines @@
 }
 
 void DecryptingDemuxerStream::DoReset() {
   DCHECK(init_cb_.is_null());
   DCHECK(read_cb_.is_null());
   state_ = kIdle;
   base::ResetAndReturn(&reset_cb_).Run();
 }
 
 Decryptor::StreamType DecryptingDemuxerStream::GetDecryptorStreamType() const {
-  DCHECK(stream_type_ == AUDIO || stream_type_ == VIDEO);
-  return stream_type_ == AUDIO ? Decryptor::kAudio : Decryptor::kVideo;
+  if (demuxer_stream_->type() == AUDIO)
+    return Decryptor::kAudio;
+
+  DCHECK_EQ(demuxer_stream_->type(), VIDEO);
+  return Decryptor::kVideo;
 }
 
-void DecryptingDemuxerStream::SetDecoderConfig(
-    const scoped_refptr<DemuxerStream>& stream) {
+void DecryptingDemuxerStream::InitializeDecoderConfig() {
   // The decoder selector or upstream demuxer make sure the stream is valid and
   // potentially encrypted.
-  DCHECK(IsStreamValidAndEncrypted(stream));
+  DCHECK(IsStreamValidAndEncrypted(demuxer_stream_));
 
-  switch (stream_type_) {
+  switch (demuxer_stream_->type()) {
     case AUDIO: {
       const AudioDecoderConfig& input_audio_config =
-          stream->audio_decoder_config();
+          demuxer_stream_->audio_decoder_config();
       audio_config_.reset(new AudioDecoderConfig());
       audio_config_->Initialize(input_audio_config.codec(),
                                 input_audio_config.sample_format(),
                                 input_audio_config.channel_layout(),
                                 input_audio_config.samples_per_second(),
                                 input_audio_config.extra_data(),
                                 input_audio_config.extra_data_size(),
                                 false,  // Output audio is not encrypted.
                                 false);
       break;
     }
 
     case VIDEO: {
       const VideoDecoderConfig& input_video_config =
-          stream->video_decoder_config();
+          demuxer_stream_->video_decoder_config();
       video_config_.reset(new VideoDecoderConfig());
       video_config_->Initialize(input_video_config.codec(),
                                 input_video_config.profile(),
                                 input_video_config.format(),
                                 input_video_config.coded_size(),
                                 input_video_config.visible_rect(),
                                 input_video_config.natural_size(),
                                 input_video_config.extra_data(),
                                 input_video_config.extra_data_size(),
                                 false,  // Output video is not encrypted.
                                 false);
       break;
     }
 
     default:
       NOTREACHED();
       return;
   }
 }
 
 }  // namespace media