Clean up media::DecryptingDemuxerStream.

media/filters/decrypting_demuxer_stream.cc

Index: media/filters/decrypting_demuxer_stream.cc
diff --git a/media/filters/decrypting_demuxer_stream.cc b/media/filters/decrypting_demuxer_stream.cc
index 694116811aeaae9f3a79b756bb8c4034601746db..9ad105c215683362725305b5449a5ea1473608be 100644
--- a/media/filters/decrypting_demuxer_stream.cc
+++ b/media/filters/decrypting_demuxer_stream.cc
@@ -37,7 +37,6 @@ DecryptingDemuxerStream::DecryptingDemuxerStream(
     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) {
@@ -46,12 +45,19 @@ DecryptingDemuxerStream::DecryptingDemuxerStream(
 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) {
@@ -68,13 +74,8 @@ void DecryptingDemuxerStream::Read(const ReadCB& read_cb) {
 }
 
 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());
@@ -104,19 +105,19 @@ void DecryptingDemuxerStream::Reset(const base::Closure& closure) {
 
 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() {
@@ -125,25 +126,6 @@ void DecryptingDemuxerStream::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());
@@ -165,19 +147,7 @@ void DecryptingDemuxerStream::SetDecryptor(Decryptor* decryptor) {
 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());
@@ -198,11 +168,12 @@ void DecryptingDemuxerStream::DoDecryptBuffer(
 
   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;
@@ -226,25 +197,13 @@ void DecryptingDemuxerStream::DecryptPendingBuffer() {
   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);
@@ -311,20 +270,22 @@ void DecryptingDemuxerStream::DoReset() {
 }
 
 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(),
@@ -339,7 +300,7 @@ void DecryptingDemuxerStream::SetDecoderConfig(
 
     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(),