Encrypted Media: Add config change support in DecryptingVideoDecoder.

media/filters/decrypting_video_decoder.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_video_decoder.h"
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/debug/trace_event.h"
 #include "base/location.h"
@@ skipping 68 matching lines @@
   }
 
   state_ = kPendingDemuxerRead;
   ReadFromDemuxerStream();
 }
 
 void DecryptingVideoDecoder::Reset(const base::Closure& closure) {
   DVLOG(2) << "Reset() - state: " << state_;
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK(state_ == kIdle ||
+         state_ == kPendingConfigChange ||
          state_ == kPendingDemuxerRead ||
          state_ == kPendingDecode ||
          state_ == kWaitingForKey ||
          state_ == kDecodeFinished) << state_;
   DCHECK(init_cb_.is_null());  // No Reset() during pending initialization.
   DCHECK(reset_cb_.is_null());
 
   reset_cb_ = BindToCurrentLoop(closure);
 
   decryptor_->ResetDecoder(Decryptor::kVideo);
 
   // 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 DecryptAndDecodeBuffer() and
   // DeliverFrame().
-  if (state_ == kPendingDemuxerRead || state_ == kPendingDecode) {
+  if (state_ == kPendingConfigChange ||
+      state_ == kPendingDemuxerRead ||
+      state_ == kPendingDecode) {
     DCHECK(!read_cb_.is_null());
     return;
   }
 
   if (state_ == kWaitingForKey) {
     DCHECK(!read_cb_.is_null());
     pending_buffer_to_decode_ = NULL;
     base::ResetAndReturn(&read_cb_).Run(kOk, NULL);
   }
 
@@ skipping 73 matching lines @@
   }
 
   decryptor_->RegisterNewKeyCB(Decryptor::kVideo, BindToCurrentLoop(
       base::Bind(&DecryptingVideoDecoder::OnKeyAdded, this)));
 
   // Success!
   state_ = kIdle;
   base::ResetAndReturn(&init_cb_).Run(PIPELINE_OK);
 }
 
+void DecryptingVideoDecoder::FinishConfigChange(bool success) {
+  DVLOG(2) << "FinishConfigChange()";
+  DCHECK(message_loop_->BelongsToCurrentThread());
+
+  if (state_ == kStopped)
+    return;
+
+  DCHECK_EQ(state_, kPendingConfigChange) << state_;
+  DCHECK(!read_cb_.is_null());
+
+  if (success) {
+    if (reset_cb_.is_null()) {

[ddorwin, 2013/01/04 17:59:01]

Could you also eliminate this indent by moving 222-226 up there and inverting
the logic?

[xhwang, 2013/01/05 00:09:55]

Done.

+      state_ = kPendingDemuxerRead;
+      ReadFromDemuxerStream();
+    } else {

[acolwell GONE FROM CHROMIUM, 2013/01/04 16:03:20]

nit: add return above and drop else

[ddorwin, 2013/01/04 17:59:01]

Maybe even invert the logic to if (!reset_cb_.is_null()), which seems to be the
more common pattern in this file.

[xhwang, 2013/01/05 00:09:55]

Done.

[xhwang, 2013/01/05 00:09:55]

Done.

+      base::ResetAndReturn(&read_cb_).Run(kOk, NULL);
+      DoReset();
+    }
+    return;
+  }
+
+  // Config change failed.
+  base::ResetAndReturn(&read_cb_).Run(kDecodeError, NULL);
+  state_ = kDecodeFinished;
+  if (!reset_cb_.is_null())
+    base::ResetAndReturn(&reset_cb_).Run();
+}
+
 void DecryptingVideoDecoder::ReadFromDemuxerStream() {
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK_EQ(state_, kPendingDemuxerRead) << state_;
   DCHECK(!read_cb_.is_null());
 
   demuxer_stream_->Read(
       base::Bind(&DecryptingVideoDecoder::DecryptAndDecodeBuffer, this));
 }
 
 void DecryptingVideoDecoder::DecryptAndDecodeBuffer(
     DemuxerStream::Status status,
     const scoped_refptr<DecoderBuffer>& buffer) {
   DVLOG(3) << "DecryptAndDecodeBuffer()";
   DCHECK(message_loop_->BelongsToCurrentThread());
 
   if (state_ == kStopped)
     return;
 
   DCHECK_EQ(state_, kPendingDemuxerRead) << state_;
   DCHECK(!read_cb_.is_null());
   DCHECK_EQ(buffer != NULL, status == DemuxerStream::kOk) << status;
 
-  if (!reset_cb_.is_null()) {
-    base::ResetAndReturn(&read_cb_).Run(kOk, NULL);
-    if (!reset_cb_.is_null())
-      DoReset();
+  if (status == DemuxerStream::kConfigChanged) {
+    DVLOG(2) << "DecryptAndDecodeBuffer() - kConfigChanged";
+
+    scoped_ptr<VideoDecoderConfig> scoped_config(new VideoDecoderConfig());
+    scoped_config->CopyFrom(demuxer_stream_->video_decoder_config());
+
+    state_ = kPendingConfigChange;
+    decryptor_->DeinitializeDecoder(Decryptor::kVideo);
+    decryptor_->InitializeVideoDecoder(

[acolwell GONE FROM CHROMIUM, 2013/01/04 16:03:20]

nit: In a different CL, consider changing the signature of this method to take a
const VideoDecoderConfig& instead of a scoped_ptr<VideoDecoderConfig>. The
conversion to a scoped_ptr above seems like an annoying dance for this caller to
go through with no apparent benefit.

[xhwang, 2013/01/05 00:09:55]

The original reason that I use the scoped_ptr<> here is that XxxDecoderConfig is
not copyable. I don't really see why we chose to do that. Probably we can
eliminate that restriction and then remove the use of scoped_ptr<> here.

[acolwell GONE FROM CHROMIUM, 2013/01/05 01:17:12]

I don't understand. If you make the parameter const& a copy isn't needed. If
code in InitializeVideoDecoder() needs to save a copy then it can use CopyFrom()
just like you do here. It seemed cleaner to me to have the callee do the copy if
necessary instead of forcing the caller to do it.

[xhwang, 2013/01/05 04:03:51]

I remember the original reason is that in PpapiDecryptor I need to do a post,
where the parameters needs to be copied:
http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/media/crypto/ppapi_...

I agree that even in this case the callee could manually copy the config. But
backing a bit, why in the first place we make them uncopiable? Is it because it
could have extra_data which requires deep copy?

+        scoped_config.Pass(), BindToCurrentLoop(base::Bind(
+            &DecryptingVideoDecoder::FinishConfigChange, this)));
     return;
   }
 
+  if (!reset_cb_.is_null()) {
+    base::ResetAndReturn(&read_cb_).Run(kOk, NULL);
+    DoReset();
+    return;
+  }
+
   if (status == DemuxerStream::kAborted) {
     DVLOG(2) << "DecryptAndDecodeBuffer() - kAborted";
     state_ = kIdle;
     base::ResetAndReturn(&read_cb_).Run(kOk, NULL);
     return;
   }
 
-  if (status == DemuxerStream::kConfigChanged) {
-    // TODO(xhwang): Add config change support.
-    // The |state_| is chosen to be kDecodeFinished here to be consistent with
-    // the implementation of FFmpegVideoDecoder.
-    DVLOG(2) << "DecryptAndDecodeBuffer() - kConfigChanged";
-    state_ = kDecodeFinished;
-    base::ResetAndReturn(&read_cb_).Run(kDecodeError, NULL);
-    return;
-  }
-
   DCHECK_EQ(status, DemuxerStream::kOk);
   pending_buffer_to_decode_ = buffer;
   state_ = kPendingDecode;
   DecodePendingBuffer();
 }
 
 void DecryptingVideoDecoder::DecodePendingBuffer() {
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK_EQ(state_, kPendingDecode) << state_;
   TRACE_EVENT_ASYNC_BEGIN0(
@@ skipping 102 matching lines @@
 }
 
 void DecryptingVideoDecoder::DoReset() {
   DCHECK(init_cb_.is_null());
   DCHECK(read_cb_.is_null());
   state_ = kIdle;
   base::ResetAndReturn(&reset_cb_).Run();
 }
 
 }  // namespace media