Add DecryptingDemuxerStream.

Add DecryptingDemuxerStream.

DecryptingDemuxerStream transforms an encrypted demuxer stream into a clear demuxer stream with the same stream type (audio/video) and codec config. It may be blocking because it relies on a Decryptor for decryption. Thus other than implementing the DemuxerStream interface, it also exposes a Reset() methods so that caller can cancel the decryption operation.

BUG=123421, 141786
TEST=media_unittests pass

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167852

[xhwang, 2012-11-05 23:41:10]

Please compare to patch set 1 for a comparison between DDS and DAD, which may
make reviewing new files a little easier. PTAL!

[ddorwin, 2012-11-13 01:08:49]

Reviewed everything but the new unit test. Thanks for making the new files easy
to review!

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_au...
File media/filters/decrypting_audio_decoder_unittest.cc (right):

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_au...
media/filters/decrypting_audio_decoder_unittest.cc:89: : decoder_(new
DecryptingAudioDecoder(
Just curious, why did you drop StrictMock?

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_au...
media/filters/decrypting_audio_decoder_unittest.cc:102:
buffer->SetDataSize(kFakeAudioFrameSize);
// Frame contains random data.

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_au...
media/filters/decrypting_audio_decoder_unittest.cc:103: decoded_frame_ = buffer;
Do we even need the local variable |buffer|?

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_de...
File media/filters/decrypting_demuxer_stream.cc (right):

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_de...
media/filters/decrypting_demuxer_stream.cc:111: void
DecryptingDemuxerStream::EnableBitstreamConverter() {}
Should this ever be called?

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_de...
File media/filters/decrypting_demuxer_stream.h (right):

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_de...
media/filters/decrypting_demuxer_stream.h:22: // demuxer stream to a clear
demuxer stream.
Does it really convert the stream? Is the stream encrypted or the blocks within
it?

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_de...
media/filters/decrypting_demuxer_stream.h:99: // Callback for the |decryptor_|
to notify the DecryptingDemuxerStream that
s/the DecryptingDemuxerStream/this object/
We should make similar changes in DVD and DAD.

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_de...
media/filters/decrypting_demuxer_stream.h:106: // Returns Decryptor::StreamType
converted from |stream_type_|.
Should we just move Type to a common location in media/base?

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_de...
media/filters/decrypting_demuxer_stream.h:114: // Current state of the
DecryptingDemuxerStream.
s/DecryptingDemuxerStream/object/
But you can probably just remove this comment since that's what State/state_
should be.

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_de...
media/filters/decrypting_demuxer_stream.h:140: bool
key_added_while_pending_decrypt_;
Does "...decrypt_pending_" make more sense? "... while a decrypt is pending".
The way it is sounds like the class is pending (waiting for?) a decrypt.

[xhwang, 2012-11-13 21:10:21]

PTAL again!

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_au...
File media/filters/decrypting_audio_decoder_unittest.cc (right):

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_au...
media/filters/decrypting_audio_decoder_unittest.cc:89: : decoder_(new
DecryptingAudioDecoder(
On 2012/11/13 01:08:49, ddorwin wrote:
> Just curious, why did you drop StrictMock?

DecryptingAudioDecoder is a concrete class, not a mock. I put StrickMock here by
mistake.

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_au...
media/filters/decrypting_audio_decoder_unittest.cc:102:
buffer->SetDataSize(kFakeAudioFrameSize);
On 2012/11/13 01:08:49, ddorwin wrote:
> // Frame contains random data.

Done.

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_au...
media/filters/decrypting_audio_decoder_unittest.cc:103: decoded_frame_ = buffer;
On 2012/11/13 01:08:49, ddorwin wrote:
> Do we even need the local variable |buffer|?

We can only call SetDataSize() on a DataBuffer, not a Buffer. Changed the name
to (hopefully) make this more clear.

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_de...
File media/filters/decrypting_demuxer_stream.cc (right):

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_de...
media/filters/decrypting_demuxer_stream.cc:111: void
DecryptingDemuxerStream::EnableBitstreamConverter() {}
On 2012/11/13 01:08:49, ddorwin wrote:
> Should this ever be called?

Good question. GpuVideoDecoder will call this if the codec is H264
(http://code.google.com/searchframe#OAMlx_jo-ck/src/media/filters/gpu_video_de...).

In that case we should call the corresponding function in the input demuxer
stream.

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_de...
File media/filters/decrypting_demuxer_stream.h (right):

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_de...
media/filters/decrypting_demuxer_stream.h:22: // demuxer stream to a clear
demuxer stream.
On 2012/11/13 01:08:49, ddorwin wrote:
> Does it really convert the stream? Is the stream encrypted or the blocks
within
> it?

Hmm, let's say the video elementary stream is compressed, it actually mean all
frames(buffers/blocks) are compressed. Similarly, an encrypted stream means that
all blocks within it are encrypted. WDYT?

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_de...
media/filters/decrypting_demuxer_stream.h:99: // Callback for the |decryptor_|
to notify the DecryptingDemuxerStream that
On 2012/11/13 01:08:49, ddorwin wrote:
> s/the DecryptingDemuxerStream/this object/
> We should make similar changes in DVD and DAD.

Done.

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_de...
media/filters/decrypting_demuxer_stream.h:106: // Returns Decryptor::StreamType
converted from |stream_type_|.
On 2012/11/13 01:08:49, ddorwin wrote:
> Should we just move Type to a common location in media/base?

Agreed. In another CL? TOTO added in decryptor.h

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_de...
media/filters/decrypting_demuxer_stream.h:114: // Current state of the
DecryptingDemuxerStream.
On 2012/11/13 01:08:49, ddorwin wrote:
> s/DecryptingDemuxerStream/object/
> But you can probably just remove this comment since that's what State/state_
> should be.

Done.

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_de...
media/filters/decrypting_demuxer_stream.h:140: bool
key_added_while_pending_decrypt_;
On 2012/11/13 01:08:49, ddorwin wrote:
> Does "...decrypt_pending_" make more sense? "... while a decrypt is pending".
> The way it is sounds like the class is pending (waiting for?) a decrypt.

Done.

[ddorwin, 2012-11-13 22:52:33]

lgtm

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_de...
File media/filters/decrypting_demuxer_stream.h (right):

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_de...
media/filters/decrypting_demuxer_stream.h:22: // demuxer stream to a clear
demuxer stream.
On 2012/11/13 21:10:21, xhwang wrote:
> On 2012/11/13 01:08:49, ddorwin wrote:
> > Does it really convert the stream? Is the stream encrypted or the blocks
> within
> > it?
> 
> Hmm, let's say the video elementary stream is compressed, it actually mean all
> frames(buffers/blocks) are compressed. Similarly, an encrypted stream means
that
> all blocks within it are encrypted. WDYT?

Not _all_ blocks within the encrypted stream (at least the stream this demuxer
receives/produces) are encrypted. This class is created for the entire life of
the stream. Some blocks may be unencrypted.

The comment is fine and gets the point across.

http://codereview.chromium.org/11342031/diff/18001/media/filters/decrypting_d...
File media/filters/decrypting_demuxer_stream_unittest.cc (right):

http://codereview.chromium.org/11342031/diff/18001/media/filters/decrypting_d...
media/filters/decrypting_demuxer_stream_unittest.cc:112: // The following
functions are used to test stream-type-neutral logics in
s/logics/logic/

[xhwang, 2012-11-14 18:01:52]

scherkus@: please also take a look! (Sorry for minor rebase noise.)

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_de...
File media/filters/decrypting_demuxer_stream.h (right):

http://codereview.chromium.org/11342031/diff/8001/media/filters/decrypting_de...
media/filters/decrypting_demuxer_stream.h:22: // demuxer stream to a clear
demuxer stream.
On 2012/11/13 22:52:33, ddorwin wrote:
> On 2012/11/13 21:10:21, xhwang wrote:
> > On 2012/11/13 01:08:49, ddorwin wrote:
> > > Does it really convert the stream? Is the stream encrypted or the blocks
> > within
> > > it?
> > 
> > Hmm, let's say the video elementary stream is compressed, it actually mean
all
> > frames(buffers/blocks) are compressed. Similarly, an encrypted stream means
> that
> > all blocks within it are encrypted. WDYT?
> 
> Not _all_ blocks within the encrypted stream (at least the stream this demuxer
> receives/produces) are encrypted. This class is created for the entire life of
> the stream. Some blocks may be unencrypted.
> 
> The comment is fine and gets the point across.

Right. Added "potentially" to be accurate.

http://codereview.chromium.org/11342031/diff/18001/media/filters/decrypting_d...
File media/filters/decrypting_demuxer_stream_unittest.cc (right):

http://codereview.chromium.org/11342031/diff/18001/media/filters/decrypting_d...
media/filters/decrypting_demuxer_stream_unittest.cc:112: // The following
functions are used to test stream-type-neutral logics in
On 2012/11/13 22:52:33, ddorwin wrote:
> s/logics/logic/

Done.

[scherkus (not reviewing), 2012-11-14 22:22:30]

lgtm w/ nits

good stuff!

http://codereview.chromium.org/11342031/diff/7018/media/filters/decrypting_de...
File media/filters/decrypting_demuxer_stream.cc (right):

http://codereview.chromium.org/11342031/diff/7018/media/filters/decrypting_de...
media/filters/decrypting_demuxer_stream.cc:49: DCHECK(state_ == kIdle) <<
state_;
EQ

http://codereview.chromium.org/11342031/diff/7018/media/filters/decrypting_de...
File media/filters/decrypting_demuxer_stream.h (right):

http://codereview.chromium.org/11342031/diff/7018/media/filters/decrypting_de...
media/filters/decrypting_demuxer_stream.h:30: // Callback to notify decryptor
creation.
blank lines between // comments

[xhwang, 2012-11-15 00:09:59]

http://codereview.chromium.org/11342031/diff/7018/media/filters/decrypting_de...
File media/filters/decrypting_demuxer_stream.cc (right):

http://codereview.chromium.org/11342031/diff/7018/media/filters/decrypting_de...
media/filters/decrypting_demuxer_stream.cc:49: DCHECK(state_ == kIdle) <<
state_;
On 2012/11/14 22:22:30, scherkus wrote:
> EQ

Done.

http://codereview.chromium.org/11342031/diff/7018/media/filters/decrypting_de...
File media/filters/decrypting_demuxer_stream.h (right):

http://codereview.chromium.org/11342031/diff/7018/media/filters/decrypting_de...
media/filters/decrypting_demuxer_stream.h:30: // Callback to notify decryptor
creation.
On 2012/11/14 22:22:30, scherkus wrote:
> blank lines between // comments

Done.

[commit-bot: I haz the power, 2012-11-15 00:11:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/11342031/12016

[commit-bot: I haz the power, 2012-11-15 00:12:01]

Failed to apply patch for media/filters/decrypting_audio_decoder_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file media/filters/decrypting_audio_decoder_unittest.cc
  Hunk #4 FAILED at 98.
  Hunk #6 FAILED at 131.
  Hunk #7 FAILED at 165.
  Hunk #8 FAILED at 180.
  Hunk #9 FAILED at 189.
  Hunk #10 FAILED at 210.
  Hunk #12 FAILED at 350.
  Hunk #13 FAILED at 368.
  Hunk #14 FAILED at 396.
  Hunk #15 FAILED at 458.
  10 out of 15 hunks FAILED -- saving rejects to file
media/filters/decrypting_audio_decoder_unittest.cc.rej

Patch:       media/filters/decrypting_audio_decoder_unittest.cc
Index: media/filters/decrypting_audio_decoder_unittest.cc
diff --git a/media/filters/decrypting_audio_decoder_unittest.cc
b/media/filters/decrypting_audio_decoder_unittest.cc
index
24e9eba876b5edee1847fa56425acb2e51b9e010..cbb1a5f75d40a39d65498da7a6e50fb3b77f510a
100644
--- a/media/filters/decrypting_audio_decoder_unittest.cc
+++ b/media/filters/decrypting_audio_decoder_unittest.cc
@@ -21,7 +21,6 @@
 
 using ::testing::_;
 using ::testing::AtMost;
-using ::testing::Invoke;
 using ::testing::IsNull;
 using ::testing::ReturnRef;
 using ::testing::SaveArg;
@@ -72,7 +71,7 @@ MATCHER(IsEndOfStream, "end of stream") {
 class DecryptingAudioDecoderTest : public testing::Test {
  public:
   DecryptingAudioDecoderTest()
-      : decoder_(new StrictMock<DecryptingAudioDecoder>(
+      : decoder_(new DecryptingAudioDecoder(
             base::Bind(&Identity<scoped_refptr<base::MessageLoopProxy> >,
                        message_loop_.message_loop_proxy()),
             base::Bind(
@@ -84,10 +83,10 @@ class DecryptingAudioDecoderTest : public testing::Test {
         decoded_frame_(NULL),
         end_of_stream_frame_(new DataBuffer(0)),
         decoded_frame_list_() {
-    // TODO(xhwang): Fix this after DataBuffer(data, size) is public.
-    scoped_refptr<DataBuffer> buffer = new DataBuffer(kFakeAudioFrameSize);
-    buffer->SetDataSize(kFakeAudioFrameSize);
-    decoded_frame_ = buffer;
+    scoped_refptr<DataBuffer> data_buffer = new
DataBuffer(kFakeAudioFrameSize);
+    data_buffer->SetDataSize(kFakeAudioFrameSize);
+    // |decoded_frame_| contains random data.
+    decoded_frame_ = data_buffer;
     decoded_frame_list_.push_back(decoded_frame_);
   }
 
@@ -99,7 +98,7 @@ class DecryptingAudioDecoderTest : public testing::Test {
     decoder_->Initialize(demuxer_, NewExpectedStatusCB(status),
                          base::Bind(&MockStatisticsCB::OnStatistics,
                                     base::Unretained(&statistics_cb_)));
-    message_loop_.RunAllPending();
+    message_loop_.RunUntilIdle();
   }
 
   void Initialize() {
@@ -111,10 +110,10 @@ class DecryptingAudioDecoderTest : public testing::Test {
     EXPECT_CALL(*decryptor_, RegisterKeyAddedCB(Decryptor::kAudio, _))
         .WillOnce(SaveArg<1>(&key_added_cb_));
 
-    config_.Initialize(kCodecVorbis, 16, CHANNEL_LAYOUT_STEREO, 44100,
-                       NULL, 0, true, true);
+    AudioDecoderConfig config(kCodecVorbis, 16, CHANNEL_LAYOUT_STEREO, 44100,
+                              NULL, 0, true);
+    InitializeAndExpectStatus(config, PIPELINE_OK);
 
-    InitializeAndExpectStatus(config_, PIPELINE_OK);
     EXPECT_EQ(16, decoder_->bits_per_channel());
     EXPECT_EQ(CHANNEL_LAYOUT_STEREO, decoder_->channel_layout());
     EXPECT_EQ(44100, decoder_->samples_per_second());
@@ -132,7 +131,7 @@ class DecryptingAudioDecoderTest : public testing::Test {
 
     decoder_->Read(base::Bind(&DecryptingAudioDecoderTest::FrameReady,
                               base::Unretained(this)));
-    message_loop_.RunAllPending();
+    message_loop_.RunUntilIdle();
   }
 
   // Sets up expectations and actions to put DecryptingAudioDecoder in an
@@ -166,7 +165,7 @@ class DecryptingAudioDecoderTest : public testing::Test {
         .WillOnce(SaveArg<0>(&pending_demuxer_read_cb_));
     decoder_->Read(base::Bind(&DecryptingAudioDecoderTest::FrameReady,
                               base::Unretained(this)));
-    message_loop_.RunAllPending();
+    message_loop_.RunUntilIdle();
     // Make sure the Read() on the decoder triggers a Read() on the demuxer.
     EXPECT_FALSE(pending_demuxer_read_cb_.is_null());
   }
@@ -181,7 +180,7 @@ class DecryptingAudioDecoderTest : public testing::Test {
 
     decoder_->Read(base::Bind(&DecryptingAudioDecoderTest::FrameReady,
                               base::Unretained(this)));
-    message_loop_.RunAllPending();
+    message_loop_.RunUntilIdle();
     // Make sure the Read() on the decoder triggers a DecryptAndDecode() on the
     // decryptor.
     EXPECT_FALSE(pending_audio_decode_cb_.is_null());
@@ -190,12 +189,12 @@ class DecryptingAudioDecoderTest : public testing::Test {
   void EnterWaitingForKeyState() {
     EXPECT_CALL(*demuxer_, Read(_))
         .WillRepeatedly(ReturnBuffer(encrypted_buffer_));
-    EXPECT_CALL(*decryptor_, DecryptAndDecodeAudio(_, _))
+    EXPECT_CALL(*decryptor_, DecryptAndDecodeAudio(encrypted_buffer_, _))
         .WillRepeatedly(RunCallback<1>(Decryptor::kNoKey,
                                        Decryptor::AudioBuffers()));
     decoder_->Read(base::Bind(&DecryptingAudioDecoderTest::FrameReady,
                               base::Unretained(this)));
-    message_loop_.RunAllPending();
+    message_loop_.RunUntilIdle();
   }
 
   void AbortPendingAudioDecodeCB() {
@@ -211,7 +210,7 @@ class DecryptingAudioDecoderTest : public testing::Test {
             this, &DecryptingAudioDecoderTest::AbortPendingAudioDecodeCB));
 
     decoder_->Reset(NewExpectedClosure());
-    message_loop_.RunAllPending();
+    message_loop_.RunUntilIdle();
   }
 
   MOCK_METHOD1(RequestDecryptorNotification,
@@ -221,11 +220,10 @@ class DecryptingAudioDecoderTest : public testing::Test {
                                 const scoped_refptr<Buffer>&));
 
   MessageLoop message_loop_;
-  scoped_refptr<StrictMock<DecryptingAudioDecoder> > decoder_;
+  scoped_refptr<DecryptingAudioDecoder> decoder_;
   scoped_ptr<StrictMock<MockDecryptor> > decryptor_;
   scoped_refptr<StrictMock<MockDemuxerStream> > demuxer_;
   MockStatisticsCB statistics_cb_;
-  AudioDecoderConfig config_;
 
   DemuxerStream::ReadCB pending_demuxer_read_cb_;
   Decryptor::DecoderInitCB pending_init_cb_;
@@ -352,7 +350,7 @@ TEST_F(DecryptingAudioDecoderTest,
KeyAdded_DuringWaitingForKey) {
   EXPECT_CALL(statistics_cb_, OnStatistics(_));
   EXPECT_CALL(*this, FrameReady(AudioDecoder::kOk, decoded_frame_));
   key_added_cb_.Run();
-  message_loop_.RunAllPending();
+  message_loop_.RunUntilIdle();
 }
 
 // Test the case where the a key is added when the decryptor is in
@@ -370,7 +368,7 @@ TEST_F(DecryptingAudioDecoderTest,
KeyAdded_DruingPendingDecode) {
   key_added_cb_.Run();
   base::ResetAndReturn(&pending_audio_decode_cb_).Run(
       Decryptor::kNoKey, Decryptor::AudioBuffers());
-  message_loop_.RunAllPending();
+  message_loop_.RunUntilIdle();
 }
 
 // Test resetting when the decoder is in kIdle state but has not decoded any
@@ -398,7 +396,7 @@ TEST_F(DecryptingAudioDecoderTest,
Reset_DuringPendingDemuxerRead) {
   Reset();
   base::ResetAndReturn(&pending_demuxer_read_cb_).Run(DemuxerStream::kOk,
                                                       encrypted_buffer_);
-  message_loop_.RunAllPending();
+  message_loop_.RunUntilIdle();
 }
 
 // Test resetting when the decoder is in kPendingDecode state.
@@ -460,7 +458,7 @@ TEST_F(DecryptingAudioDecoderTest,
DemuxerRead_AbortedDuringReset) {
   Reset();
   base::ResetAndReturn(&pending_demuxer_read_cb_).Run(DemuxerStream::kAborted,
                                                       NULL);
-  message_loop_.RunAllPending();
+  message_loop_.RunUntilIdle();
 }
 
 // Test config change on the demuxer stream.

[commit-bot: I haz the power, 2012-11-15 00:19:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/11342031/12017

[commit-bot: I haz the power, 2012-11-15 03:57:59]

Change committed as 167852