media: Refactor MediaCrypto creation and notification.

media: Refactor MediaCrypto creation and notification.

In Java MediaDrmBridge, when MediaCrypto creation failed, always release
everything and notify native code with a null MediaCrypto.

Remove Java GetMediaCrypto() function. MediaCrypto will always be notified to
native code immediately after creation succeeded or failed.

BUG=564246
TEST=Tested MediaCrypto creation success and failure cases.

Committed: https://crrev.com/6815c88392493ddbc7db1afa9babd6a3cca338ff
Cr-Commit-Position: refs/heads/master@{#364825}

[xhwang, 2015-12-11 00:37:05]

xhwang@chromium.org changed reviewers:
+ qinmin@chromium.org, timav@chromium.org

[xhwang, 2015-12-11 00:37:06]

PTAL

[Tima Vaisburd, 2015-12-11 02:15:21]

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/j...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/j...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:231: return
false;
Is this an error condition?

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/m...
File media/base/android/media_codec_player.cc (right):

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/m...
media/base/android/media_codec_player.cc:942: // TODO(timav): Fail playback
nicely here.
Wouldn't this has to be taken care of in this CL or a prerequisite CL? Min,
WDYT? The same consideration applies to MediaSourcePlayer.

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/m...
File media/base/android/media_drm_bridge.h (right):

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/m...
media/base/android/media_drm_bridge.h:289: // j_media_crypto_->is_null():
MediaCrypto notified but creation failed.
nit: I'd change the order: "MediaCrypto creation failed and it has been
notified"

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/m...
File media/base/android/media_source_player.cc (right):

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/m...
media/base/android/media_source_player.cc:272:
DCHECK(static_cast<MediaDrmBridge*>(cdm_.get())->GetMediaCrypto());
Shall we propagate |media_crypto| parameter to MediaDecoderJob and get rid of
GetMediaCrypto() altogether?

[xhwang, 2015-12-11 05:27:30]

rebase only

[xhwang, 2015-12-11 05:50:19]

PTAL again.

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/j...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/j...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:231: return
false;
On 2015/12/11 02:15:21, Tima Vaisburd wrote:
> Is this an error condition?

Good question. This should never happen. Replaced with an assert.

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/m...
File media/base/android/media_codec_player.cc (right):

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/m...
media/base/android/media_codec_player.cc:942: // TODO(timav): Fail playback
nicely here.
On 2015/12/11 02:15:21, Tima Vaisburd wrote:
> Wouldn't this has to be taken care of in this CL or a prerequisite CL? Min,
> WDYT? The same consideration applies to MediaSourcePlayer.

I don't think so. This CL doesn't regress anything. 

Previously if MediaCrypto creation failed MediaCodecPlayer won't get notified,
and we'll just be waiting forever. This CL doesn't change this.

Sometimes it takes multiple steps to fix one problem. I don't think we should
just have one big CL to fix everything.

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/m...
File media/base/android/media_drm_bridge.h (right):

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/m...
media/base/android/media_drm_bridge.h:289: // j_media_crypto_->is_null():
MediaCrypto notified but creation failed.
On 2015/12/11 02:15:21, Tima Vaisburd wrote:
> nit: I'd change the order: "MediaCrypto creation failed and it has been
> notified"

Done.

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/m...
File media/base/android/media_source_player.cc (right):

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/m...
media/base/android/media_source_player.cc:272:
DCHECK(static_cast<MediaDrmBridge*>(cdm_.get())->GetMediaCrypto());
On 2015/12/11 02:15:21, Tima Vaisburd wrote:
> Shall we propagate |media_crypto| parameter to MediaDecoderJob and get rid of
> GetMediaCrypto() altogether?

Yes, that would be the correct fix.

However, since MediaSourcePlayer is going away soon (right?), I am not going to
spend time fixing it. Once we drop MediaSourcePlayer, we should be able to get
rid of GetMediaCrypto().

https://chromiumcodereview.appspot.com/1512173003/diff/20001/media/base/andro...
File media/base/android/media_drm_bridge.cc (right):

https://chromiumcodereview.appspot.com/1512173003/diff/20001/media/base/andro...
media/base/android/media_drm_bridge.cc:784:
j_object_ptr->Reset(AttachCurrentThread(), object);
timav / qinmin:

CreateJavaObjectPtr() can be called on different threads (see the call in
OnMediaCryptoReady() on line 543. Even though ScopedJavaGlovalRef is safe to be
passed across threads, I wonder whether we should call AttachCurrentThread()
here.

Can I just pass in nullptr here instead? One example of using nullptr is here:
https://code.google.com/p/chromium/codesearch#chromium/src/base/android/scope...

[xhwang, 2015-12-11 05:51:08]

comments addressed

[Tima Vaisburd, 2015-12-11 19:31:24]

lgtm with comments.

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/m...
File media/base/android/media_codec_player.cc (right):

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/m...
media/base/android/media_codec_player.cc:942: // TODO(timav): Fail playback
nicely here.
On 2015/12/11 05:50:19, xhwang wrote:

> [...] This CL doesn't regress anything. 
> 
> Previously if MediaCrypto creation failed MediaCodecPlayer won't get notified,
> and we'll just be waiting forever. This CL doesn't change this.

Good point, and good explanation! I think it's worth putting into the CL
description (before it just never get notified, now it is notified as error).
You are right and I take my objection back.

However, the only thing I would do to "fallback nicely" is to go to the error
state and report an error to the upper level. Both things are done with one line
  GetMediaTaskRunner()->PostTask(FROM_HERE, internal_error_cb_);

Would you be able to add it if media_crypto is null? Then there would be no
TODO.

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/m...
File media/base/android/media_source_player.cc (right):

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/m...
media/base/android/media_source_player.cc:272:
DCHECK(static_cast<MediaDrmBridge*>(cdm_.get())->GetMediaCrypto());
On 2015/12/11 05:50:19, xhwang wrote:
> On 2015/12/11 02:15:21, Tima Vaisburd wrote:
> > Shall we propagate |media_crypto| parameter to MediaDecoderJob and get rid
of
> > GetMediaCrypto() altogether?
> 
> Yes, that would be the correct fix.
> 
> However, since MediaSourcePlayer is going away soon (right?), I am not going
to
> spend time fixing it. Once we drop MediaSourcePlayer, we should be able to get
> rid of GetMediaCrypto().

As far as I know the switching between MediaSourcePlayer and MediaCodecPlayer
was designed to be reversible (with finch configuration) in case we will
encounter unexpected problems. Besides, the decision to switch forward has not
been made yet. Because of that it has been my understanding that we have to
maintain both players until there is an explicit decision to drop the former.

I can make this propagation if you do not feel comfortable to do it.

https://chromiumcodereview.appspot.com/1512173003/diff/80001/media/base/andro...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://chromiumcodereview.appspot.com/1512173003/diff/80001/media/base/andro...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:250: //
Notify the native code that MediaCrypto is ready.
nit: Remove or change this comment?

https://chromiumcodereview.appspot.com/1512173003/diff/80001/media/base/andro...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:260: return
false;
My first instinctive desire would be to refactor createMediaCrypto() to have one
return point and keep onMediaCryptoReady() call within this method. Than, maybe,
we can get rid of mMediaCrypto as member and pass it as a local var to
onMediaCryptoReady().

But I haven't seen any good way to refactor, so I'm just mentioning that to you
in case you'd like this idea. No need to change anything.

https://chromiumcodereview.appspot.com/1512173003/diff/80001/media/base/andro...
File media/base/android/media_source_player.cc (right):

https://chromiumcodereview.appspot.com/1512173003/diff/80001/media/base/andro...
media/base/android/media_source_player.cc:275: // TODO(xhwang): Fail playback
nicely here.
The normal reaction to internal error seems to be
    Release();
    manager()->OnError(player_id(), MEDIA_ERROR_DECODE);

Should we put it here and this would conclude TODO?

[qinmin, 2015-12-11 19:39:31]

lgtm % nit

https://codereview.chromium.org/1512173003/diff/80001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://codereview.chromium.org/1512173003/diff/80001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:578:
onMediaCryptoReady();
You can remove onMediaCryptoReady() and just call
nativeOnMediaCryptoReady(mNativeMediaDrmBridge, mMediaCrypto) here now

[xhwang, 2015-12-11 20:54:04]

comments addressed

[Tima Vaisburd, 2015-12-11 21:03:48]

On 2015/12/11 20:54:04, xhwang wrote:
> comments addressed

lgtm

[xhwang, 2015-12-11 21:45:16]

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/m...
File media/base/android/media_codec_player.cc (right):

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/m...
media/base/android/media_codec_player.cc:942: // TODO(timav): Fail playback
nicely here.
On 2015/12/11 19:31:24, Tima Vaisburd wrote:
> On 2015/12/11 05:50:19, xhwang wrote:
> 
> > [...] This CL doesn't regress anything. 
> > 
> > Previously if MediaCrypto creation failed MediaCodecPlayer won't get
notified,
> > and we'll just be waiting forever. This CL doesn't change this.
> 
> Good point, and good explanation! I think it's worth putting into the CL
> description (before it just never get notified, now it is notified as error).
> You are right and I take my objection back.
> 
> However, the only thing I would do to "fallback nicely" is to go to the error
> state and report an error to the upper level. Both things are done with one
line
>   GetMediaTaskRunner()->PostTask(FROM_HERE, internal_error_cb_);
> 
> Would you be able to add it if media_crypto is null? Then there would be no
> TODO.

Thanks. As discussed offline, I'd like this TODO be addressed in a different CL.

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/m...
File media/base/android/media_source_player.cc (right):

https://chromiumcodereview.appspot.com/1512173003/diff/1/media/base/android/m...
media/base/android/media_source_player.cc:272:
DCHECK(static_cast<MediaDrmBridge*>(cdm_.get())->GetMediaCrypto());
On 2015/12/11 19:31:24, Tima Vaisburd wrote:
> On 2015/12/11 05:50:19, xhwang wrote:
> > On 2015/12/11 02:15:21, Tima Vaisburd wrote:
> > > Shall we propagate |media_crypto| parameter to MediaDecoderJob and get rid
> of
> > > GetMediaCrypto() altogether?
> > 
> > Yes, that would be the correct fix.
> > 
> > However, since MediaSourcePlayer is going away soon (right?), I am not going
> to
> > spend time fixing it. Once we drop MediaSourcePlayer, we should be able to
get
> > rid of GetMediaCrypto().
> 
> As far as I know the switching between MediaSourcePlayer and MediaCodecPlayer
> was designed to be reversible (with finch configuration) in case we will
> encounter unexpected problems. Besides, the decision to switch forward has not
> been made yet. Because of that it has been my understanding that we have to
> maintain both players until there is an explicit decision to drop the former.
> 
> I can make this propagation if you do not feel comfortable to do it.

Yes, in that case, it's nice to make this change. But I'd consider it low
priority because it don't provide any behavior change.

https://chromiumcodereview.appspot.com/1512173003/diff/80001/media/base/andro...
File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (right):

https://chromiumcodereview.appspot.com/1512173003/diff/80001/media/base/andro...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:250: //
Notify the native code that MediaCrypto is ready.
On 2015/12/11 19:31:24, Tima Vaisburd wrote:
> nit: Remove or change this comment?

Done.

https://chromiumcodereview.appspot.com/1512173003/diff/80001/media/base/andro...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:260: return
false;
On 2015/12/11 19:31:24, Tima Vaisburd wrote:
> My first instinctive desire would be to refactor createMediaCrypto() to have
one
> return point and keep onMediaCryptoReady() call within this method. Than,
maybe,
> we can get rid of mMediaCrypto as member and pass it as a local var to
> onMediaCryptoReady().
> 
> But I haven't seen any good way to refactor, so I'm just mentioning that to
you
> in case you'd like this idea. No need to change anything.

This CL is really more about MediaCrypto notification. My next CL will make more
changes in MediaCrypto creation. You'll see that soon.

https://chromiumcodereview.appspot.com/1512173003/diff/80001/media/base/andro...
media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:578:
onMediaCryptoReady();
On 2015/12/11 19:39:31, qinmin wrote:
> You can remove onMediaCryptoReady() and just call
> nativeOnMediaCryptoReady(mNativeMediaDrmBridge, mMediaCrypto) here now 

Good point! However, in the next CL I may call onMediaCryptoReady() in other
places. I am not super sure whether this statement will still be true. I'll
evaluate it in the next CL.

https://chromiumcodereview.appspot.com/1512173003/diff/80001/media/base/andro...
File media/base/android/media_source_player.cc (right):

https://chromiumcodereview.appspot.com/1512173003/diff/80001/media/base/andro...
media/base/android/media_source_player.cc:275: // TODO(xhwang): Fail playback
nicely here.
On 2015/12/11 19:31:24, Tima Vaisburd wrote:
> The normal reaction to internal error seems to be
>     Release();
>     manager()->OnError(player_id(), MEDIA_ERROR_DECODE);
> 
> Should we put it here and this would conclude TODO? 

As discussed offline. Refined the TODO and we'll consider it in a separate CL.

[xhwang, 2015-12-11 21:45:31]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2015-12-11 21:45:32]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1512173003/#ps120001 (title: "update TODO
comments")

[commit-bot: I haz the power, 2015-12-11 21:46:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1512173003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1512173003/120001

[commit-bot: I haz the power, 2015-12-11 22:58:51]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-12-11 22:59:42]

Description was changed from

==========
media: Refactor MediaCrypto creation and notification.

In Java MediaDrmBridge, when MediaCrypto creation failed, always release
everything and notify native code with a null MediaCrypto.

Remove Java GetMediaCrypto() function. MediaCrypto will always be notified to
native code immediately after creation succeeded or failed.

BUG=564246
TEST=Tested MediaCrypto creation success and failure cases.
==========

to

==========
media: Refactor MediaCrypto creation and notification.

In Java MediaDrmBridge, when MediaCrypto creation failed, always release
everything and notify native code with a null MediaCrypto.

Remove Java GetMediaCrypto() function. MediaCrypto will always be notified to
native code immediately after creation succeeded or failed.

BUG=564246
TEST=Tested MediaCrypto creation success and failure cases.

Committed: https://crrev.com/6815c88392493ddbc7db1afa9babd6a3cca338ff
Cr-Commit-Position: refs/heads/master@{#364825}
==========

[commit-bot: I haz the power, 2015-12-11 22:59:43]

Patchset 7 (id:??) landed as
https://crrev.com/6815c88392493ddbc7db1afa9babd6a3cca338ff
Cr-Commit-Position: refs/heads/master@{#364825}