Encrypted Media: Add config change support in DecryptingVideoDecoder.

media/filters/decrypting_video_decoder.cc

Index: media/filters/decrypting_video_decoder.cc
diff --git a/media/filters/decrypting_video_decoder.cc b/media/filters/decrypting_video_decoder.cc
index 7117d753384fa7ab80d6323ceabe14a627f6a6fe..9d3111cc5cf474c4206871664eb87a8c3b4c91c9 100644
--- a/media/filters/decrypting_video_decoder.cc
+++ b/media/filters/decrypting_video_decoder.cc
@@ -86,6 +86,7 @@ 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 ||
@@ -101,7 +102,9 @@ void DecryptingVideoDecoder::Reset(const base::Closure& closure) {
   // 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;
   }
@@ -195,6 +198,34 @@ void DecryptingVideoDecoder::FinishInitialization(bool success) {
   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_;
@@ -217,10 +248,23 @@ void DecryptingVideoDecoder::DecryptAndDecodeBuffer(
   DCHECK(!read_cb_.is_null());
   DCHECK_EQ(buffer != NULL, status == DemuxerStream::kOk) << status;
 
+  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);
-    if (!reset_cb_.is_null())
-      DoReset();
+    DoReset();
     return;
   }
 
@@ -231,16 +275,6 @@ void DecryptingVideoDecoder::DecryptAndDecodeBuffer(
     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;