Update Decryptor interface to support audio decoding.

Update Decryptor interface to support audio decoding.

We were discussing the use of
decryptor_->DeinitializeDecoder(Decryptor::kVideo);
  - vs -
decryptor_->DeinitializeVideoDecoder();

I choose to use parameterized methods so that we have less code (duplication). The non-parameterized methods are cleaner for callers. But I feel the former is slightly better in general.

Later on we'll have two Decryptors (ProxyDecryptor will not be a Decryptor anymore):
1) AesDecryptor, which doesn't care the stream type at all, not even for CancelDecrypt().
2) PpapiDecryptor, which merely passes similar calls to the PluginInstance which then make similar PPP calls. It seems to me that for passing calls it's slightly cleaner to use parameterized methods. Also we have parameterized PPP calls already for the same reason (less functions through multiple layers).

So in both cases, using parameterized methods results in less code.

It's unfortunate that InitializeXXXDecoder and DecryptAndDecodeXXX are exceptions, due to different types they take/return.

BUG=123421
TEST=media_unittest, content_unittest

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

[xhwang, 2012-10-15 23:47:33]

Decryptor interface change only. I'll fix header includes and update Decryptor
implementations shortly. If you have time, please take a look. Early comments
are always welcome :)

[xhwang, 2012-10-16 20:05:52]

Ready for review. PTAL!

[xhwang, 2012-10-17 00:18:59]

PTAL!

http://codereview.chromium.org/11144036/diff/9002/media/base/decryptor.h
File media/base/decryptor.h (right):

http://codereview.chromium.org/11144036/diff/9002/media/base/decryptor.h#newc...
media/base/decryptor.h:134: typedef std::list<scoped_refptr<Buffer> >
AudioBuffers;
One status per decode call should be sufficient. Drop extra status to simplify
implementation.

[ddorwin, 2012-10-17 02:52:33]

LG. Just minor stuff.

http://codereview.chromium.org/11144036/diff/9002/media/base/decryptor.h
File media/base/decryptor.h (right):

http://codereview.chromium.org/11144036/diff/9002/media/base/decryptor.h#newc...
media/base/decryptor.h:146: //   buffer. In this case the second parameter must
be NULL.
s/second/buffer/ (or data or something)
I realize the name is not consistent, but position could easily get out of data
upon refactoring.
Same two times below.

http://codereview.chromium.org/11144036/diff/9002/media/base/decryptor.h#newc...
media/base/decryptor.h:162: // These two methods can only be called after the
corresponding decoder has
s/two//

http://codereview.chromium.org/11144036/diff/9002/media/base/decryptor.h#newc...
media/base/decryptor.h:172: // decrypt-and-decode operations and fires any
pending
A comma before a >2 item "and" would probably be helpful here and the next
paragraph since they are so long and detailed.

http://codereview.chromium.org/11144036/diff/9002/media/base/decryptor.h#newc...
media/base/decryptor.h:178: // Uninitializes the decoder and sets it to an
uninitialized state, cancels
// Frees decoder resources and...?

http://codereview.chromium.org/11144036/diff/9002/media/base/decryptor.h#newc...
media/base/decryptor.h:181: // false or kError/NULL respectively.
It's not clear how these map respectively since the numbers mismatch.

http://codereview.chromium.org/11144036/diff/9002/media/filters/decrypting_vi...
File media/filters/decrypting_video_decoder.cc (right):

http://codereview.chromium.org/11144036/diff/9002/media/filters/decrypting_vi...
media/filters/decrypting_video_decoder.cc:125:
decryptor_->DeinitializeDecoder(Decryptor::kVideo);
nit: Might be less bug-prone to have a private helper function. Just a thought.

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/clear_key_cdm.cc (right):

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/clear_key_cdm.cc:216: // We don't care what stream
type it is here. So just pick video.
I've been wondering why we need stream type in Decrypt(). Is there anything we
might actually do with it?

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/ppapi_d...
File webkit/media/crypto/ppapi_decryptor.cc (right):

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/ppapi_d...
webkit/media/crypto/ppapi_decryptor.cc:142: //   
base::ResetAndReturn(&audio_decoder_init_cb_).Run(false);
Uncomment this instead of 146? Then move 145 up to 137.

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/ppapi_d...
webkit/media/crypto/ppapi_decryptor.cc:190: //   audio_decode_cb.Run(kError,
NULL);
same.
And why does 192 have scoped_ptr instead of just NULL?

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/ppapi_d...
webkit/media/crypto/ppapi_decryptor.cc:248: if (stream_type == kAudio) {
Should we use a switch?

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/proxy_d...
File webkit/media/crypto/proxy_decryptor.cc (right):

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/proxy_d...
webkit/media/crypto/proxy_decryptor.cc:160: DCHECK_EQ(stream_type, kVideo);  //
Only support video decrypt-only for now.
Is there anything that doesn't work? Maybe there's nothing that calls this, but
we shouldn't DCHECK here unless we need to.

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/proxy_d...
webkit/media/crypto/proxy_decryptor.cc:191: DCHECK_EQ(stream_type, kVideo);  //
Only support video decrypt-only for now.
same

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/proxy_d...
webkit/media/crypto/proxy_decryptor.cc:309: kVideo,  // Only support video
decrypt-only for now.
If we really need this parameter (see earlier comment), why not just add the
parameter to this call and remove the DCHECKs/comments?

[ddorwin, 2012-10-17 07:16:05]

a few more nits

http://codereview.chromium.org/11144036/diff/9002/media/base/decryptor.h
File media/base/decryptor.h (right):

http://codereview.chromium.org/11144036/diff/9002/media/base/decryptor.h#newc...
media/base/decryptor.h:178: // Uninitializes the decoder and sets it to an
uninitialized state, cancels
On 2012/10/17 02:52:33, ddorwin wrote:
> // Frees decoder resources and...?

s/Frees/Releases/

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/ppapi_d...
File webkit/media/crypto/ppapi_decryptor.cc (right):

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/ppapi_d...
webkit/media/crypto/ppapi_decryptor.cc:220: DVLOG(2) << "ResetDecoder()";
Log stream_type throughout?

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/ppapi_d...
webkit/media/crypto/ppapi_decryptor.cc:221: cdm_plugin_->ResetDecoder();
TODO: Add type param?

[xhwang, 2012-10-17 22:29:06]

PTAL again!

http://codereview.chromium.org/11144036/diff/9002/media/base/decryptor.h
File media/base/decryptor.h (right):

http://codereview.chromium.org/11144036/diff/9002/media/base/decryptor.h#newc...
media/base/decryptor.h:146: //   buffer. In this case the second parameter must
be NULL.
On 2012/10/17 02:52:33, ddorwin wrote:
> s/second/buffer/ (or data or something)
> I realize the name is not consistent, but position could easily get out of
data
> upon refactoring.
> Same two times below.

Done.

http://codereview.chromium.org/11144036/diff/9002/media/base/decryptor.h#newc...
media/base/decryptor.h:162: // These two methods can only be called after the
corresponding decoder has
On 2012/10/17 02:52:33, ddorwin wrote:
> s/two//

Done.

http://codereview.chromium.org/11144036/diff/9002/media/base/decryptor.h#newc...
media/base/decryptor.h:172: // decrypt-and-decode operations and fires any
pending
On 2012/10/17 02:52:33, ddorwin wrote:
> A comma before a >2 item "and" would probably be helpful here and the next
> paragraph since they are so long and detailed.

Done.

http://codereview.chromium.org/11144036/diff/9002/media/base/decryptor.h#newc...
media/base/decryptor.h:178: // Uninitializes the decoder and sets it to an
uninitialized state, cancels
On 2012/10/17 07:16:05, ddorwin wrote:
> On 2012/10/17 02:52:33, ddorwin wrote:
> > // Frees decoder resources and...?
> 
> s/Frees/Releases/

Done.

http://codereview.chromium.org/11144036/diff/9002/media/base/decryptor.h#newc...
media/base/decryptor.h:178: // Uninitializes the decoder and sets it to an
uninitialized state, cancels
On 2012/10/17 02:52:33, ddorwin wrote:
> // Frees decoder resources and...?

Done.

http://codereview.chromium.org/11144036/diff/9002/media/base/decryptor.h#newc...
media/base/decryptor.h:181: // false or kError/NULL respectively.
On 2012/10/17 02:52:33, ddorwin wrote:
> It's not clear how these map respectively since the numbers mismatch.

Done.

http://codereview.chromium.org/11144036/diff/9002/media/filters/decrypting_vi...
File media/filters/decrypting_video_decoder.cc (right):

http://codereview.chromium.org/11144036/diff/9002/media/filters/decrypting_vi...
media/filters/decrypting_video_decoder.cc:125:
decryptor_->DeinitializeDecoder(Decryptor::kVideo);
On 2012/10/17 02:52:33, ddorwin wrote:
> nit: Might be less bug-prone to have a private helper function. Just a
thought.

I'll hold here as fischman is trying to remove this part all together :)

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/clear_key_cdm.cc (right):

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/clear_key_cdm.cc:216: // We don't care what stream
type it is here. So just pick video.
On 2012/10/17 02:52:33, ddorwin wrote:
> I've been wondering why we need stream type in Decrypt(). Is there anything we
> might actually do with it?

Yep. Remember that we need to be able to cancel only one stream? To do that, we
first need to track which stream is doing the decryption.

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/ppapi_d...
File webkit/media/crypto/ppapi_decryptor.cc (right):

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/ppapi_d...
webkit/media/crypto/ppapi_decryptor.cc:142: //   
base::ResetAndReturn(&audio_decoder_init_cb_).Run(false);
On 2012/10/17 02:52:33, ddorwin wrote:
> Uncomment this instead of 146? Then move 145 up to 137.

Done.

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/ppapi_d...
webkit/media/crypto/ppapi_decryptor.cc:190: //   audio_decode_cb.Run(kError,
NULL);
On 2012/10/17 02:52:33, ddorwin wrote:
> same.
> And why does 192 have scoped_ptr instead of just NULL?

It's not relevant now. But for your question, because scoped_ptr(T* p) is
explicit. I don't know why we enforce that though.

Done.

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/ppapi_d...
webkit/media/crypto/ppapi_decryptor.cc:220: DVLOG(2) << "ResetDecoder()";
On 2012/10/17 07:16:05, ddorwin wrote:
> Log stream_type throughout?

Done.

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/ppapi_d...
webkit/media/crypto/ppapi_decryptor.cc:221: cdm_plugin_->ResetDecoder();
On 2012/10/17 07:16:05, ddorwin wrote:
> TODO: Add type param?

Done.

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/ppapi_d...
webkit/media/crypto/ppapi_decryptor.cc:248: if (stream_type == kAudio) {
On 2012/10/17 02:52:33, ddorwin wrote:
> Should we use a switch?

Done.

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/proxy_d...
File webkit/media/crypto/proxy_decryptor.cc (right):

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/proxy_d...
webkit/media/crypto/proxy_decryptor.cc:160: DCHECK_EQ(stream_type, kVideo);  //
Only support video decrypt-only for now.
On 2012/10/17 02:52:33, ddorwin wrote:
> Is there anything that doesn't work? Maybe there's nothing that calls this,
but
> we shouldn't DCHECK here unless we need to.

Audio decrypt-only is not implemented yet. Later when we implement it we'll not
use ProxyDecryptor::Decrypt anyway. 
This DCHECK makes it clear that we really only have one stream type, which
ensures the corectness of all the following code (pending_decrypt_cb etc).
Otherwise, we'll need two sets of them.

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/proxy_d...
webkit/media/crypto/proxy_decryptor.cc:191: DCHECK_EQ(stream_type, kVideo);  //
Only support video decrypt-only for now.
On 2012/10/17 02:52:33, ddorwin wrote:
> same

ditto

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/proxy_d...
webkit/media/crypto/proxy_decryptor.cc:309: kVideo,  // Only support video
decrypt-only for now.
On 2012/10/17 02:52:33, ddorwin wrote:
> If we really need this parameter (see earlier comment), why not just add the
> parameter to this call and remove the DCHECKs/comments?

see above reply.

http://codereview.chromium.org/11144036/diff/5003/media/base/decryptor.h
File media/base/decryptor.h (right):

http://codereview.chromium.org/11144036/diff/5003/media/base/decryptor.h#newc...
media/base/decryptor.h:152: typedef base::Callback<void(Status, const
AudioBuffers&)> AudioDecodeCB;
Remove scoped_ptr<> as it makes the unittest really hard. Copying a list of
scoped_refptr's shouldn't be a big deal, especially for us the list should be
pretty small for most (if not all) cases.

[ddorwin, 2012-10-17 23:26:30]

LGTM.
Question unrelated to the code.

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/clear_key_cdm.cc (right):

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/clear_key_cdm.cc:216: // We don't care what stream
type it is here. So just pick video.
On 2012/10/17 22:29:06, xhwang wrote:
> On 2012/10/17 02:52:33, ddorwin wrote:
> > I've been wondering why we need stream type in Decrypt(). Is there anything
we
> > might actually do with it?
> 
> Yep. Remember that we need to be able to cancel only one stream? To do that,
we
> first need to track which stream is doing the decryption.

When/where will that be implemented?

[scherkus (not reviewing), 2012-10-18 02:08:36]

lgtm with tiny, tiny nits

can you also document in the CL description the motivation for using StreamType
as opposed to duplicating the methods?

it's asymmetric with InitializeXXXDecoder() and DecryptAndDecodeXXX() and from
the caller's perspective it's less pithy:
decryptor_->DeinitializeDecoder(Decryptor::kVideo);
  - vs -
decryptor_->DeinitializeVideoDecoder();

...but it seems that ProxyDecryptor makes use of the parameterized methods
(somehow?)

you don't have to change it but I do want to see the decision documented

http://codereview.chromium.org/11144036/diff/5003/media/base/decryptor.h
File media/base/decryptor.h (right):

http://codereview.chromium.org/11144036/diff/5003/media/base/decryptor.h#newc...
media/base/decryptor.h:152: typedef base::Callback<void(Status, const
AudioBuffers&)> AudioDecodeCB;
On 2012/10/17 22:29:06, xhwang wrote:
> Remove scoped_ptr<> as it makes the unittest really hard. Copying a list of
> scoped_refptr's shouldn't be a big deal, especially for us the list should be
> pretty small for most (if not all) cases.

Did you mean removing scoped_refptr<>?

I'm not quite following which scoped_ptr<> was removed.

http://codereview.chromium.org/11144036/diff/5003/webkit/media/crypto/ppapi_d...
File webkit/media/crypto/ppapi_decryptor.cc (right):

http://codereview.chromium.org/11144036/diff/5003/webkit/media/crypto/ppapi_d...
webkit/media/crypto/ppapi_decryptor.cc:139: //  if
(!cdm_plugin_->InitializeAudioDecoder(
use #if 0 instead

http://codereview.chromium.org/11144036/diff/5003/webkit/media/crypto/ppapi_d...
webkit/media/crypto/ppapi_decryptor.cc:189: // if
(!cdm_plugin_->DecryptAndDecodeAudio(encrypted, audio_decode_cb))
use #if 0 instead

[xhwang, 2012-10-18 04:33:16]

For discussion about
decryptor_->DeinitializeDecoder(Decryptor::kVideo);
  - vs -
decryptor_->DeinitializeVideoDecoder();

I choose to use parameterized methods merely to have less code (duplication):

At the end of the day, we'll only have two Decryptors:
1) AesDecryptor, which doesn't care the stream type at all, not even for
CancelDecrypt().
2) PpapiDecryptor, which merely passes similar calls to the PluginInstance which
then make similar PPP calls. It seems to me that for passing calls it's slightly
cleaner to use parameterized methods. Also we have parameterized PPP calls
already for the same reason (less functions through multiple layers).

So in both cases, using parameterized methods results in less code.

It's unfortunate that InitializeXXXDecoder and DecryptAndDecodeXXX are
exceptions, due to different types they take/return. I also agree that it's not
as clean for callers. But I feel it's slightly better in general. WDYT?

I'll put the above comments in the CL description if it sgty.

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/clear_key_cdm.cc (right):

http://codereview.chromium.org/11144036/diff/9002/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/clear_key_cdm.cc:216: // We don't care what stream
type it is here. So just pick video.
On 2012/10/17 23:26:31, ddorwin wrote:
> On 2012/10/17 22:29:06, xhwang wrote:
> > On 2012/10/17 02:52:33, ddorwin wrote:
> > > I've been wondering why we need stream type in Decrypt(). Is there
anything
> we
> > > might actually do with it?
> > 
> > Yep. Remember that we need to be able to cancel only one stream? To do that,
> we
> > first need to track which stream is doing the decryption.
> 
> When/where will that be implemented?

After we have decrypt-and-decode working, I'll add DecryptingDemuxerStream,
which will be our new solution for decrypt-only for both audio and video.

http://codereview.chromium.org/11144036/diff/5003/media/base/decryptor.h
File media/base/decryptor.h (right):

http://codereview.chromium.org/11144036/diff/5003/media/base/decryptor.h#newc...
media/base/decryptor.h:152: typedef base::Callback<void(Status, const
AudioBuffers&)> AudioDecodeCB;
On 2012/10/18 02:08:36, scherkus wrote:
> On 2012/10/17 22:29:06, xhwang wrote:
> > Remove scoped_ptr<> as it makes the unittest really hard. Copying a list of
> > scoped_refptr's shouldn't be a big deal, especially for us the list should
be
> > pretty small for most (if not all) cases.
> 
> Did you mean removing scoped_refptr<>?
> 
> I'm not quite following which scoped_ptr<> was removed.

I was using:
typedef base::Callback<void(Status, scoped_ptr<AudioBuffers>)> AudioDecodeCB
So this scoped_ptr was dropped.

[commit-bot: I haz the power, 2012-10-18 17:49:38]

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

[commit-bot: I haz the power, 2012-10-18 20:05:44]

Change committed as 162768