Call NotifyDemuxerReady() only once while keys are added.

Currently, NotifyDemuxerReady() is called whenever key is added,                
which made problem when multiple keys are added.                                
This CL is to make sure NotifyDemuxerReady() once.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207783

[kjyoun, 2013-06-20 13:13:42]

Can you review this CL?

[xhwang, 2013-06-20 17:30:50]

General comment:

Using key-added as an indication that we can start to decrypt is a little hacky.
There are a lot of cases that this isn't true, just to name a few (copy from an
inline comment):

- There may be multiple key_id in the stream while we only added one key, e.g.
audio and video requires different
keys.
- The "key" added may just be some message that the server sent to the content
decryption module which doesn't necessarily contains a decryption key.

What will happen if we declare "can_start_decrypt" while the decoder (or whoever
does decryption for GTV) cannot decrypt? Will that be an error?

Fundamentally, only the real decryptor knows if it can decrypt a buffer, and
only after it sees the buffer (key_id). If it cannot decrypt the buffer, it
should enter the "wait for a key" state
(https://dvcs.w3.org/hg/html-media/raw-file/tip/encrypted-media/encrypted-medi...).
If the spec is implemented correctly, the demuxer should not worry about if the
decryptor can decrypt the stream. The decryptor/decoder will ask for buffers
from the demuxer. If the decryptor can decrypt the buffers, it'll ask more.
Otherwise, it'll hold the buffers it cannot decrypt until the correct key is
added.

That being said, if this CL is blocking GTV we can still commit it (after the
comments are resolved). But really we should remove the hack and implement the
spec correctly.

https://codereview.chromium.org/17449020/diff/3001/webkit/renderer/media/andr...
File webkit/renderer/media/android/media_source_delegate.cc (right):

https://codereview.chromium.org/17449020/diff/3001/webkit/renderer/media/andr...
webkit/renderer/media/android/media_source_delegate.cc:335:
NotifyDemuxerReady();
It appears that we NotifyDemuxerReady() multiple times until I see the check on
line 373. Can we do:

if (CanNotifyDemuxerReady)
  NotifyDemuxerReady();

and remove the check on line 373?

https://codereview.chromium.org/17449020/diff/3001/webkit/renderer/media/andr...
webkit/renderer/media/android/media_source_delegate.cc:373: if (!demuxer_ ||
!CanNotifyDemuxerReady())
I prefer DCHECK to "if". Can |demuxer| be NULL here? In which case it can be
NULL? If it should not be NULL, DCHECK it?

https://codereview.chromium.org/17449020/diff/3001/webkit/renderer/media/andr...
File webkit/renderer/media/android/media_source_delegate.h (right):

https://codereview.chromium.org/17449020/diff/3001/webkit/renderer/media/andr...
webkit/renderer/media/android/media_source_delegate.h:111: void
NotifyDemuxerReady(const std::string& key_system = std::string());
Our style guide doesn't allow default arguments in general. Actually I don't see
|key_system| is used anywhere. Drop it?

https://codereview.chromium.org/17449020/diff/3001/webkit/renderer/media/andr...
webkit/renderer/media/android/media_source_delegate.h:162: bool
can_start_decrypt_;
Key added doesn't mean we can decrypt. There may be multiple key_id in the
stream while we only added one key, e.g. audio and video requires different
keys. Or the "key" added may just be some message that the server sent to the
decryption module which doesn't necessarily contains a decryption key. I think
something like "key_added_" would be more accurate here.

https://codereview.chromium.org/17449020/diff/3001/webkit/renderer/media/andr...
File webkit/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/17449020/diff/3001/webkit/renderer/media/andr...
webkit/renderer/media/android/webmediaplayer_android.cc:1040:
current_key_system_.utf8(), session_id);
Why is MSD interested in what key system it is?

[kjyoun, 2013-06-21 00:05:38]

PTAL

@Xiaohan
Thanks for the detailed explanation, and agreed with your explanation.
This CL is to make current EME implementation on chrome-for-tv not break. We
will switch to more formal implementation that you are doing, when it's ready.

https://codereview.chromium.org/17449020/diff/3001/webkit/renderer/media/andr...
File webkit/renderer/media/android/media_source_delegate.cc (right):

https://codereview.chromium.org/17449020/diff/3001/webkit/renderer/media/andr...
webkit/renderer/media/android/media_source_delegate.cc:335:
NotifyDemuxerReady();
On 2013/06/20 17:30:50, xhwang wrote:
> It appears that we NotifyDemuxerReady() multiple times until I see the check
on
> line 373. Can we do:
> 
> if (CanNotifyDemuxerReady)
>   NotifyDemuxerReady();
> 
> and remove the check on line 373?

Done.

https://codereview.chromium.org/17449020/diff/3001/webkit/renderer/media/andr...
webkit/renderer/media/android/media_source_delegate.cc:373: if (!demuxer_ ||
!CanNotifyDemuxerReady())
On 2013/06/20 17:30:50, xhwang wrote:
> I prefer DCHECK to "if". Can |demuxer| be NULL here? In which case it can be
> NULL? If it should not be NULL, DCHECK it?

Done.

https://codereview.chromium.org/17449020/diff/3001/webkit/renderer/media/andr...
File webkit/renderer/media/android/media_source_delegate.h (right):

https://codereview.chromium.org/17449020/diff/3001/webkit/renderer/media/andr...
webkit/renderer/media/android/media_source_delegate.h:111: void
NotifyDemuxerReady(const std::string& key_system = std::string());
On 2013/06/20 17:30:50, xhwang wrote:
> Our style guide doesn't allow default arguments in general. Actually I don't
see
> |key_system| is used anywhere. Drop it?

Done.

https://codereview.chromium.org/17449020/diff/3001/webkit/renderer/media/andr...
webkit/renderer/media/android/media_source_delegate.h:162: bool
can_start_decrypt_;
renamed it to key_added_

On 2013/06/20 17:30:50, xhwang wrote:
> Key added doesn't mean we can decrypt. There may be multiple key_id in the
> stream while we only added one key, e.g. audio and video requires different
> keys. Or the "key" added may just be some message that the server sent to the
> decryption module which doesn't necessarily contains a decryption key. I think
> something like "key_added_" would be more accurate here.

https://codereview.chromium.org/17449020/diff/3001/webkit/renderer/media/andr...
File webkit/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/17449020/diff/3001/webkit/renderer/media/andr...
webkit/renderer/media/android/webmediaplayer_android.cc:1040:
current_key_system_.utf8(), session_id);
MSD needs to know key_system in order to pass it to demuxer_stream_player, 
which pass this info to platform's video path.

FYI, key_system and other crypto-related info is wrapped in
MediaPlayerHostMsg_DemuxerReady_Params 
On 2013/06/20 17:30:50, xhwang wrote:
> Why is MSD interested in what key system it is?

[xhwang, 2013-06-21 00:24:55]

Thanks! LGTM

[qinmin, 2013-06-21 00:37:11]

lgtm % nit

https://codereview.chromium.org/17449020/diff/10001/webkit/renderer/media/and...
File webkit/renderer/media/android/media_source_delegate.cc (right):

https://codereview.chromium.org/17449020/diff/10001/webkit/renderer/media/and...
webkit/renderer/media/android/media_source_delegate.cc:353: if (!key_added_) {
nit:remove nested loops.
if (key_added_)
  return;
key_added_ = true;
...............

[commit-bot: I haz the power, 2013-06-21 03:17:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kjyoun@google.com/17449020/18001

[commit-bot: I haz the power, 2013-06-21 10:04:02]

Change committed as 207783