Update PluginInstance for audio support for content decryption.

Update PluginInstance for audio support for content decryption.

BUG=123421
TEST=none


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

[xhwang, 2012-10-19 17:07:33]

Hi, last piece at chrome side to support audio in media pipeline. I haven't
tested the audio D&D path since I don't have a fake decoder yet. I'll work on
that.

PTAL!

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:397: bool
MakeAudioFrames(PP_Resource audio_frames,
probably need comments here.

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2787: if (audio_frames == 0) {  //
End-of-stream frame.
We can remove this check if we all agree that EOS frame should always be a
zero-size buffer in audio_frames.

[xhwang, 2012-10-19 17:08:12]

On 2012/10/19 17:07:33, xhwang wrote:
> Hi, last piece at chrome side to support audio in media pipeline. I haven't
> tested the audio D&D path since I don't have a fake decoder yet. I'll work on
> that.
> 
> PTAL!
> 
>
http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
> File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):
> 
>
http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
> webkit/plugins/ppapi/ppapi_plugin_instance.cc:397: bool
> MakeAudioFrames(PP_Resource audio_frames,
> probably need comments here.
> 
>
http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
> webkit/plugins/ppapi/ppapi_plugin_instance.cc:2787: if (audio_frames == 0) { 
//
> End-of-stream frame.
> We can remove this check if we all agree that EOS frame should always be a
> zero-size buffer in audio_frames.

In case it's not obvious, this is based on Tom's CL 11144011.

[Tom Finegan, 2012-10-19 22:53:52]

Mostly LG...

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:397: bool
MakeAudioFrames(PP_Resource audio_frames,
On 2012/10/19 17:07:33, xhwang wrote:
> probably need comments here.

Agreed, but I don't think it needs anything too big. How about:

Deserializes audio data stored in |audio_frames| into individual audio buffers
in |frames|. Returns true upon success.

I don't know.... writing that makes me think the function should be named
DeserializeAudioFrames(). WDYT?

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:433: while (bytes_left > 0);
Move the while up on the same line with the closing brace for the do.

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1609: // DecryptAndDecodeAudio()
and DecryptAndDecodeVideo().
We should do this when we refactor and add the delegate.

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1807: DVLOG(2) <<
"DecryptAndDecode() - request_id " << request_id;
s/DecryptAndDecode/DecryptAndDecodeAudio/

(Here and below)

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2787: if (audio_frames == 0) {  //
End-of-stream frame.
On 2012/10/19 17:07:33, xhwang wrote:
> We can remove this check if we all agree that EOS frame should always be a
> zero-size buffer in audio_frames.

This isn't necessarily and end of stream frame. The decoder might need more data
to produce output.

It might be simpler to just give in and add PP_DECRYPTRESULT_NEEDMOREDATA.

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.h:279: // Note: These methods can be
used with an unencrypted frame.
s/an unencrypted frame/unencrypted data/

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.h:605: void
CancelDecode(media::Decryptor::StreamType stream_type);
This seems like kind of a random place for this method... should we keep it near
the PPP CDM methods? Did you even mean to have it here-- I see CancelDecrypt
where I expect it to be up above.

[ddorwin, 2012-10-20 00:14:36]

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:397: bool
MakeAudioFrames(PP_Resource audio_frames,
On 2012/10/19 17:07:33, xhwang wrote:
> probably need comments here.

Especially since the out parameter type is "buffers". :)

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:412: if (bytes_left < 16)
why?
16 is a magic number here and below. 8 too. sizeof()?

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:419: bytes_left -= 16;
(8 + 8) would be clearer, esp. when using sizeof().

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:421: if (frame_size < 0 ||
bytes_left < frame_size)
DCHECK? Is this a bug if we see it?

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1638: uint32_t&
pending_decrypt_request_id =
DCHECK stream_type,
Would a switch statement be equiv or less code and less confusing?

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1658: bool
PluginInstance::CancelDecrypt(media::Decryptor::StreamType stream_type) {
Is it easy for the client to know whether to call CancelDecrypt vs.
DeinitializeDecoder (and thus CancelDecode)? Do we really need separate
functions or could we just cancel whatever we are doing for audio/video based on
stream_type? If we can't do that, should we at least standardize on Cancel or
Deinitialize?

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1665: decrypt_cb =
base::ResetAndReturn(&pending_audio_decrypt_cb_);
Does this work if pending_audio_decrypt_cb_ is null/empty?

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1746: media::Decryptor::kSuccess,
media::Decryptor::AudioBuffers());
odd that video passes NULL but this doesn't. I assume there is a reason.

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1775: bool
PluginInstance::ResetDecoder(media::Decryptor::StreamType stream_type) {
Does just a Reset() for decrypt & D&D make sense?

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2638: <<
block_info->tracking_info.request_id << " no found";
s/no/not/

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2638: <<
block_info->tracking_info.request_id << " no found";
you already have request_id local var you can use.

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2760: DCHECK(audio_frames);
decrypted_block can be null above but this can't? or just a missing DCHECK
above?

[xhwang, 2012-10-21 19:05:06]

Comments resolved and updated CdmWrapper. PTAL!

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:397: bool
MakeAudioFrames(PP_Resource audio_frames,
On 2012/10/20 00:14:36, ddorwin wrote:
> On 2012/10/19 17:07:33, xhwang wrote:
> > probably need comments here.
> 
> Especially since the out parameter type is "buffers". :)

Done.

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:397: bool
MakeAudioFrames(PP_Resource audio_frames,
On 2012/10/19 22:53:52, Tom Finegan wrote:
> On 2012/10/19 17:07:33, xhwang wrote:
> > probably need comments here.
> 
> Agreed, but I don't think it needs anything too big. How about:
> 
> Deserializes audio data stored in |audio_frames| into individual audio buffers
> in |frames|. Returns true upon success.
> 
> I don't know.... writing that makes me think the function should be named
> DeserializeAudioFrames(). WDYT?

Hmm, maybe we should rename media::Decryptor::AudioBuffers to
media::Decryptor::AudioFrames ?

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:412: if (bytes_left < 16)
On 2012/10/20 00:14:36, ddorwin wrote:
> why?
> 16 is a magic number here and below. 8 too. sizeof()?

Done.

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:419: bytes_left -= 16;
On 2012/10/20 00:14:36, ddorwin wrote:
> (8 + 8) would be clearer, esp. when using sizeof().

Done.

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:421: if (frame_size < 0 ||
bytes_left < frame_size)
On 2012/10/20 00:14:36, ddorwin wrote:
> DCHECK? Is this a bug if we see it?

As I said, I don't trust CDMs. The renderer should not crash if the CDM returns
garbage. WDYT?

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:433: while (bytes_left > 0);
On 2012/10/19 22:53:52, Tom Finegan wrote:
> Move the while up on the same line with the closing brace for the do.

Done.

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1609: // DecryptAndDecodeAudio()
and DecryptAndDecodeVideo().
On 2012/10/19 22:53:52, Tom Finegan wrote:
> We should do this when we refactor and add the delegate.

Agreed.

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1638: uint32_t&
pending_decrypt_request_id =
On 2012/10/20 00:14:36, ddorwin wrote:
> DCHECK stream_type,
> Would a switch statement be equiv or less code and less confusing?

Done.

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1658: bool
PluginInstance::CancelDecrypt(media::Decryptor::StreamType stream_type) {
On 2012/10/20 00:14:36, ddorwin wrote:
> Is it easy for the client to know whether to call CancelDecrypt vs.
> DeinitializeDecoder (and thus CancelDecode)? Do we really need separate
> functions or could we just cancel whatever we are doing for audio/video based
on
> stream_type? If we can't do that, should we at least standardize on Cancel or
> Deinitialize?

I agree it's confusing.

For decoder, we have ResetDecoder and DeinitializeDecoder. For decryptor, we
choose "Cancel" because the decryptor is stateless, so we don't really need to
reset the decryptor other than canceling the cb. Also, we call it
Cancel*Decrypt* so it's clear that we are cancelling the decryption operation
scheduled by Decrypt(). 

It would be nice to consolidate CancelDecryptor and ResetDecoder. To do that,
first we need a proper name, e.g. Reset(stream_type). Not sure if it's clearer
for the caller though. Second, we need two extra variables to remember which
path (DO or D&D) the PluginInstance has chosen. 

One thing I don't like about consolidated functions for DO and D&D is that we
have to document and enforce in PI that DO and D&D methods can't live together.
For example, if Decrypt() is called, InitializeVideoDecoder() should not be
called. For now, we don't need to worry about this.

WDYT?

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1665: decrypt_cb =
base::ResetAndReturn(&pending_audio_decrypt_cb_);
On 2012/10/20 00:14:36, ddorwin wrote:
> Does this work if pending_audio_decrypt_cb_ is null/empty?

Yes. Line 1676 checks this condition.

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1746: media::Decryptor::kSuccess,
media::Decryptor::AudioBuffers());
On 2012/10/20 00:14:36, ddorwin wrote:
> odd that video passes NULL but this doesn't. I assume there is a reason.

VideoDecodeCB takes scoped_refptr<VideoFrame>, so we use NULL.
AudioDecodeCB takes std::list<Buffer>, so we pass empty list.

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1775: bool
PluginInstance::ResetDecoder(media::Decryptor::StreamType stream_type) {
On 2012/10/20 00:14:36, ddorwin wrote:
> Does just a Reset() for decrypt & D&D make sense?

We can discuss about this. See above comment.

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1807: DVLOG(2) <<
"DecryptAndDecode() - request_id " << request_id;
On 2012/10/19 22:53:52, Tom Finegan wrote:
> s/DecryptAndDecode/DecryptAndDecodeAudio/
> 
> (Here and below)

Done.

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2638: <<
block_info->tracking_info.request_id << " no found";
On 2012/10/20 00:14:36, ddorwin wrote:
> you already have request_id local var you can use.

Done.

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2760: DCHECK(audio_frames);
On 2012/10/20 00:14:36, ddorwin wrote:
> decrypted_block can be null above but this can't? or just a missing DCHECK
> above?

Done.

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2787: if (audio_frames == 0) {  //
End-of-stream frame.
On 2012/10/19 22:53:52, Tom Finegan wrote:
> On 2012/10/19 17:07:33, xhwang wrote:
> > We can remove this check if we all agree that EOS frame should always be a
> > zero-size buffer in audio_frames.
> 
> This isn't necessarily and end of stream frame. The decoder might need more
data
> to produce output.
> 
> It might be simpler to just give in and add PP_DECRYPTRESULT_NEEDMOREDATA.

Yeah, done in another CL: 11234019

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.h:279: // Note: These methods can be
used with an unencrypted frame.
On 2012/10/19 22:53:52, Tom Finegan wrote:
> s/an unencrypted frame/unencrypted data/

Done.

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.h:605: void
CancelDecode(media::Decryptor::StreamType stream_type);
On 2012/10/19 22:53:52, Tom Finegan wrote:
> This seems like kind of a random place for this method... should we keep it
near
> the PPP CDM methods? Did you even mean to have it here-- I see CancelDecrypt
> where I expect it to be up above.

The reason it's here is that it's a private method. So I added it at the bottom
of private methods. All other EME methods are public.

http://codereview.chromium.org/11189082/diff/7001/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/cdm_wrapper.cc (right):

http://codereview.chromium.org/11189082/diff/7001/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:906: decrypted_block_info.result =
PP_DECRYPTRESULT_SUCCESS;
Need to update this when NeedMoreData patch (11234019) is landed.

http://codereview.chromium.org/11189082/diff/7001/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/content_decryption_module.h (right):

http://codereview.chromium.org/11189082/diff/7001/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/content_decryption_module.h:332: DecryptedBlock*
audio_frames) = 0;
Need this change because cdm::Buffer doesn't support set_buffer().

[ddorwin, 2012-10-22 17:47:11]

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:397: bool
MakeAudioFrames(PP_Resource audio_frames,
On 2012/10/21 19:05:06, xhwang wrote:
> On 2012/10/19 22:53:52, Tom Finegan wrote:
> > On 2012/10/19 17:07:33, xhwang wrote:
> > > probably need comments here.
> > 
> > Agreed, but I don't think it needs anything too big. How about:
> > 
> > Deserializes audio data stored in |audio_frames| into individual audio
buffers
> > in |frames|. Returns true upon success.
> > 
> > I don't know.... writing that makes me think the function should be named
> > DeserializeAudioFrames(). WDYT?
> 
> Hmm, maybe we should rename media::Decryptor::AudioBuffers to
> media::Decryptor::AudioFrames ?

SGTM (in another CL).

http://codereview.chromium.org/11189082/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:421: if (frame_size < 0 ||
bytes_left < frame_size)
On 2012/10/21 19:05:06, xhwang wrote:
> On 2012/10/20 00:14:36, ddorwin wrote:
> > DCHECK? Is this a bug if we see it?
> 
> As I said, I don't trust CDMs. The renderer should not crash if the CDM
returns
> garbage. WDYT?

A DCHECK just tells us something is wrong in a debug build. So we can
investigate. We should still have the error handling for prod builds.

http://codereview.chromium.org/11189082/diff/10003/webkit/media/crypto/ppapi/...
File webkit/media/crypto/ppapi/cdm_wrapper.cc (right):

http://codereview.chromium.org/11189082/diff/10003/webkit/media/crypto/ppapi/...
webkit/media/crypto/ppapi/cdm_wrapper.cc:481: const LinkedDecryptedBlock&
audio_frames,
Type should be Samples/AudioFrames?
It sounds like DecryptedBlock has a timestamp we don't need. What is the
downside of having a separate type without it?

http://codereview.chromium.org/11189082/diff/10003/webkit/media/crypto/ppapi/...
webkit/media/crypto/ppapi/cdm_wrapper.cc:696: if (decoder_type ==
PP_DECRYPTORSTREAMTYPE_AUDIO) {
We should check for an invalid value somewhere. I like switch statements for
ensuring all enums are handled (not sure if that build switch is set).

http://codereview.chromium.org/11189082/diff/10003/webkit/media/crypto/ppapi/...
webkit/media/crypto/ppapi/cdm_wrapper.cc:898:
decrypted_block_info.tracking_info.timestamp = audio_frames->timestamp();
What is this timestamp? Do we need it? We should probably at least comment that
is is the original timestamp.

http://codereview.chromium.org/11189082/diff/10003/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/11189082/diff/10003/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:417: const int header_size =
sizeof(timestamp) + sizeof(frame_size);
kHeaderSize

http://codereview.chromium.org/11189082/diff/10003/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:428: bytes_left -=
sizeof(bytes_left);
sizeof(frame_size)

[xhwang, 2012-10-23 01:53:12]

comments resolved. PTAL!

http://codereview.chromium.org/11189082/diff/10003/webkit/media/crypto/ppapi/...
File webkit/media/crypto/ppapi/cdm_wrapper.cc (right):

http://codereview.chromium.org/11189082/diff/10003/webkit/media/crypto/ppapi/...
webkit/media/crypto/ppapi/cdm_wrapper.cc:481: const LinkedDecryptedBlock&
audio_frames,
On 2012/10/22 17:47:11, ddorwin wrote:
> Type should be Samples/AudioFrames?
> It sounds like DecryptedBlock has a timestamp we don't need. What is the
> downside of having a separate type without it?

Added AudioFrame class. But we need a ppapi change for this. So in ppapi I am
still using PP_DecryptedBlockInfo. Added a TODO for that.

http://codereview.chromium.org/11189082/diff/10003/webkit/media/crypto/ppapi/...
webkit/media/crypto/ppapi/cdm_wrapper.cc:696: if (decoder_type ==
PP_DECRYPTORSTREAMTYPE_AUDIO) {
On 2012/10/22 17:47:11, ddorwin wrote:
> We should check for an invalid value somewhere. I like switch statements for
> ensuring all enums are handled (not sure if that build switch is set). 

Done.

http://codereview.chromium.org/11189082/diff/10003/webkit/media/crypto/ppapi/...
webkit/media/crypto/ppapi/cdm_wrapper.cc:898:
decrypted_block_info.tracking_info.timestamp = audio_frames->timestamp();
On 2012/10/22 17:47:11, ddorwin wrote:
> What is this timestamp? Do we need it? We should probably at least comment
that
> is is the original timestamp.

Done.

http://codereview.chromium.org/11189082/diff/10003/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/11189082/diff/10003/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:417: const int header_size =
sizeof(timestamp) + sizeof(frame_size);
On 2012/10/22 17:47:11, ddorwin wrote:
> kHeaderSize

Done.

http://codereview.chromium.org/11189082/diff/10003/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:428: bytes_left -=
sizeof(bytes_left);
On 2012/10/22 17:47:11, ddorwin wrote:
> sizeof(frame_size)

Done.

[ddorwin, 2012-10-23 02:37:51]

lgtm % one thing

http://codereview.chromium.org/11189082/diff/13002/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/11189082/diff/13002/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2807: return;
as discussed offline, NOTREACHED()

[xhwang, 2012-10-23 02:46:54]

http://codereview.chromium.org/11189082/diff/13002/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/11189082/diff/13002/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2807: return;
On 2012/10/23 02:37:51, ddorwin wrote:
> as discussed offline, NOTREACHED()

Done.

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

lgtm

Thanks. One suggestion.

http://codereview.chromium.org/11189082/diff/9002/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/11189082/diff/9002/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2806: // This should not happen if
the CDM serialized the buffer correctly.
Can we have NOTREACHED() << "CDM did not serialize the buffer correctly."

[xhwang, 2012-10-23 03:14:18]

http://codereview.chromium.org/11189082/diff/9002/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/11189082/diff/9002/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2806: // This should not happen if
the CDM serialized the buffer correctly.
On 2012/10/23 02:52:17, ddorwin wrote:
> Can we have NOTREACHED() << "CDM did not serialize the buffer correctly."

Good point. Done. Thanks.

[Tom Finegan, 2012-10-23 03:15:27]

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

http://codereview.chromium.org/11189082/diff/9002/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:929: // cdm::AudioFrames.
Why do we need PP_DecryptedSamplesInfo? Does the media side need more
information than what will be serialized in the pp::Buffer_Dev?

http://codereview.chromium.org/11189082/diff/9002/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:933:
decrypted_block_info.tracking_info.timestamp = 0;
Ditto about PP_DSI, and setting this to 0 will be fine when the timestamps come
out of the pp::Buffer_Dev, right?

[xhwang, 2012-10-23 03:25:09]

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

http://codereview.chromium.org/11189082/diff/9002/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:929: // cdm::AudioFrames.
On 2012/10/23 03:15:27, Tom Finegan wrote:
> Why do we need PP_DecryptedSamplesInfo? Does the media side need more
> information than what will be serialized in the pp::Buffer_Dev?

We still need the decrypt result and request ID. We can keep using
PP_DecryptTrackingInfo and make sure the media side ignores the timestamp in the
trackinginfo. WDYT?

http://codereview.chromium.org/11189082/diff/9002/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:933:
decrypted_block_info.tracking_info.timestamp = 0;
On 2012/10/23 03:15:27, Tom Finegan wrote:
> Ditto about PP_DSI, and setting this to 0 will be fine when the timestamps
come
> out of the pp::Buffer_Dev, right?

ditto

[xhwang, 2012-10-23 03:31:15]

viettrungluu,

Another CL for your OWNERS review. I need to rebase this on  top of 11234019. So
please review that CL before reviewing this one. Thanks!

xhwang

[Tom Finegan, 2012-10-23 03:35:45]

lgtm

On 2012/10/23 03:25:09, xhwang wrote:
>
http://codereview.chromium.org/11189082/diff/9002/webkit/media/crypto/ppapi/c...
> File webkit/media/crypto/ppapi/cdm_wrapper.cc (right):
> 
>
http://codereview.chromium.org/11189082/diff/9002/webkit/media/crypto/ppapi/c...
> webkit/media/crypto/ppapi/cdm_wrapper.cc:929: // cdm::AudioFrames.
> On 2012/10/23 03:15:27, Tom Finegan wrote:
> > Why do we need PP_DecryptedSamplesInfo? Does the media side need more
> > information than what will be serialized in the pp::Buffer_Dev?
> 
> We still need the decrypt result and request ID. We can keep using
> PP_DecryptTrackingInfo and make sure the media side ignores the timestamp in
the
> trackinginfo. WDYT?
> 
> 

sgtm

http://codereview.chromium.org/11189082/diff/9002/webkit/media/crypto/ppapi/c...
> webkit/media/crypto/ppapi/cdm_wrapper.cc:933:
> decrypted_block_info.tracking_info.timestamp = 0;
> On 2012/10/23 03:15:27, Tom Finegan wrote:
> > Ditto about PP_DSI, and setting this to 0 will be fine when the timestamps
> come
> > out of the pp::Buffer_Dev, right?
> 
> ditto

[xhwang, 2012-10-23 20:24:03]

viettrungluu,

Just rebased. Please do an OWNERS review on ppapi_plugin_instance.* files.
Thanks!

xhwang

[viettrungluu, 2012-10-24 16:59:20]

http://codereview.chromium.org/11189082/diff/21001/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/11189082/diff/21001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:421: timestamp =
*(reinterpret_cast<const int64*>(cur));
You should probably use bit_cast (see basictypes.h).

http://codereview.chromium.org/11189082/diff/21001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:425: frame_size =
*(reinterpret_cast<const int64*>(cur));
"

http://codereview.chromium.org/11189082/diff/21001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:435:
frame->SetTimestamp(base::TimeDelta::FromMicroseconds(timestamp));
You don't set the timestamp if frame_size == 0 (even though you have one)?

http://codereview.chromium.org/11189082/diff/21001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1797: switch (stream_type) {
It'd be nice (in the future) to figure out a way to reduce the amount of
boilerplate code like this.

[xhwang, 2012-10-24 18:57:14]

Thanks for the review. Comments resolved. PTAL again!

http://codereview.chromium.org/11189082/diff/21001/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/11189082/diff/21001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:421: timestamp =
*(reinterpret_cast<const int64*>(cur));
On 2012/10/24 16:59:20, viettrungluu wrote:
> You should probably use bit_cast (see basictypes.h).

It seems bit_cast only works on casting two types with the same byte length. So
it doesn't apply here.
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/basictypes.h&exact_pa...

http://codereview.chromium.org/11189082/diff/21001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:425: frame_size =
*(reinterpret_cast<const int64*>(cur));
On 2012/10/24 16:59:20, viettrungluu wrote:
> "

ditto

http://codereview.chromium.org/11189082/diff/21001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:435:
frame->SetTimestamp(base::TimeDelta::FromMicroseconds(timestamp));
On 2012/10/24 16:59:20, viettrungluu wrote:
> You don't set the timestamp if frame_size == 0 (even though you have one)?

It's okay not to set timestamp to an empty frame. But after 11234019 is landed,
we do not allow empty frame in this list any more. Updated this part and thanks
for catching that.

http://codereview.chromium.org/11189082/diff/21001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1797: switch (stream_type) {
On 2012/10/24 16:59:20, viettrungluu wrote:
> It'd be nice (in the future) to figure out a way to reduce the amount of
> boilerplate code like this.

Added TODO.

[vtl, 2012-10-24 19:41:43]

lgtm

[commit-bot: I haz the power, 2012-10-24 19:46:22]

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

[commit-bot: I haz the power, 2012-10-24 19:46:41]

Presubmit check for 11189082-33001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    webkit/plugins/ppapi

Presubmit checks took 2.2s to calculate.

[viettrungluu, 2012-10-24 20:15:04]

lgtm

[commit-bot: I haz the power, 2012-10-24 20:20:04]

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

[commit-bot: I haz the power, 2012-10-24 22:34:02]

Change committed as 163931