Pause EME player whose external surface is stolen by another player

Pause EME player whose external surface is stolen by another player

Background: A surface is "external" when it is composited externally.
As for now, we cannot have two external surfaces at once because risk
of Z-fighting. This is why we abort the first EME video when a second
EME video is started.

Problem: User sees a black box but no "Play"-button on aborted video.

Solution: Pause aborted player (but not if it's going to fullscreen).

Example: Say player A is currently playing, player A's external surface
will be released in these three cases:
  1. Player B starts.
  2. Player B enters fullscreen.
  3. Player A enters fullscreen.
This commit pauses player A in case 1, 2 but not in case 3.

BUG=431318, 431705
TEST=Manual,
 1. Open a webpage with two EME <video>s next to each other.
 2. Start video A.
 3. Start video B.
 4. Notice: video A stops (its "Play"-button appears).

TEST=Manual,
 1. Open a webpage with two EME <video>s next to each other.
 2. Start video A.
 3. Click the fullscreen icon of video B.
 4. Notice: video A stops (it will not Z-fight, i.e not seen on top).

Committed: https://crrev.com/27e091ab97b6bd36c3a19c263d27d440bb71a9e0
Cr-Commit-Position: refs/heads/master@{#309918}

[xhwang, 2014-11-10 17:05:12]

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

[xhwang, 2014-11-10 17:05:13]

+qinmin

[qinmin, 2014-11-10 18:37:24]

https://codereview.chromium.org/714643002/diff/1/content/browser/media/androi...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/714643002/diff/1/content/browser/media/androi...
content/browser/media/android/browser_media_player_manager.cc:405:
fullscreen_player_id_ = player_id;
this logic is bad, we setting fullscreen_player_id_ because the
DetachExternalVideoSurface() will need that, it is hard for people to understand
this.

I would suggest add a player_id() function to external_video_surface_container_,
and call that function to check if the player_id is equal to the external id, if
so, do nothing. Otherwise, call pause() on that player, so we don't need the
change in DetachExternalVideoSurface().

[Hugo Holgersson, 2014-11-10 22:22:18]

https://chromiumcodereview.appspot.com/714643002/diff/1/content/browser/media...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://chromiumcodereview.appspot.com/714643002/diff/1/content/browser/media...
content/browser/media/android/browser_media_player_manager.cc:405:
fullscreen_player_id_ = player_id;
On 2014/11/10 18:37:24, qinmin wrote:
> this logic is bad, we setting fullscreen_player_id_ because the
> DetachExternalVideoSurface() will need that, it is hard for people to
understand
> this.
> 
> I would suggest add a player_id() function to
external_video_surface_container_,
> and call that function to check if the player_id is equal to the external id,
if
> so, do nothing. Otherwise, call pause() on that player, so we don't need the
> change in DetachExternalVideoSurface().

You are right!

I used this logic: the external surface's player should be paused in two cases:
A. When another player enters fullscreen.
B. The "external surface" was manually released.
That was why I wanted to ensure we pause in DetachExternalVideoSurface(). If we
only do pause() here, we would miss case B.

Case B happens more often if we land: https://codereview.chromium.org/709003004/
which enforces only one "external surface" at any time.

[Hugo Holgersson, 2014-11-20 12:48:44]

>for the other change, we'd better do release
>rather than simply pause

PTAL
I now do player->Release() instead of player->Pause(). Is that OK?

I understand that calling ReleasePlayer() is the convention. But calling
ReleasePlayer() from DetachExternalVideoSurface() could create an infinite loop:
ReleaseExternalVideoSurface() would take us, via Java, back to
DetachExternalVideoSurface() again. 

I cleared up the the CL description again.

[qinmin, 2014-11-21 00:23:50]

https://codereview.chromium.org/714643002/diff/40001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/714643002/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:404:
fullscreen_player_id_ = player_id;
As I mentioned earlier, using fullscreen_player_id to do the reentrance trick is
really bad. If another developer look at this code, he will see that
fullscreen_player_id is set to -1 on line 410, and will remove this line. And
that will break your logic.

Since there can be at most one player_id associated with
external_video_surface_container, can't we just do sth like
external_video_surface_container_->ReleaseCurrentExternalVideoSurfacePlayer()?

[Hugo Holgersson, 2014-12-01 15:56:18]

> Since there can be at most one player_id associated with
> external_video_surface_container, can't we just do sth like
> external_video_surface_container_->ReleaseCurrentExternalVideoSurfacePlayer()?
Done.

> I would suggest add a player_id() function to
external_video_surface_container_,
> and call that function to check if the player_id is equal to the external id,
if
> so, do nothing. Otherwise, call pause() on that player.
Done. I named the function GetCurrentPlayerId(). Is the naming OK?

> so we don't need the change in DetachExternalVideoSurface().
Done. I have moved the "pause-and-release logic" to a new method:
ReleasePlayerOfExternalVideoSurfaceIfNeeded(). This method is called every time
we know that the current external surface might be released. If we find out that
the external surface is about to be released, we release its player Before
releasing the external surface.

> using fullscreen_player_id to do the reentrance trick is really bad.
Done. Instead of using fullscreen_player_id as a temporary variable we pass the
id of the upcoming (future active player) to
ReleasePlayerOfExternalVideoSurfaceIfNeeded().

[Hugo Holgersson, 2014-12-03 08:22:53]

hugo.holgersson@sonymobile.com changed reviewers:
+ boliu@chromium.org

[qinmin, 2014-12-04 19:31:09]

lgtm

[boliu, 2014-12-04 23:33:26]

+ycheo fyi

Note just adding a reviewer or uploading a new patch set does not send an email
to the reviewer. You need to explicitly click "Publish+Mail Comments" for that
to happen.

I only saw this when Min approved

https://codereview.chromium.org/714643002/diff/80001/android_webview/java/src...
File
android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java
(right):

https://codereview.chromium.org/714643002/diff/80001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:134:
return mPlayerId;
Is it safe to assert mPlayerId != INVALID_PLAYER_ID here? Maybe there is already
a copy of INVALID_PLAYER_ID somewhere in native code?

I'm concerned that the implementation in content/ is making assumptions about
the value of a constant defined in android_webview/

https://codereview.chromium.org/714643002/diff/80001/android_webview/native/e...
File android_webview/native/external_video_surface_container_impl.h (right):

https://codereview.chromium.org/714643002/diff/80001/android_webview/native/e...
android_webview/native/external_video_surface_container_impl.h:32: virtual void
ReleaseCurrentExternalVideoSurface() override;
Is this always the same as ReleaseExternalVideoSurface(GetCurrentPlayerId())?

Can we just do that from content instead of adding a new method?

[boliu, 2014-12-04 23:35:59]

Super duper awesome CL description btw :)

I'm gonna start linking to this for how to write good CL descriptions. Kudos

[Hugo Holgersson, 2014-12-08 16:41:08]

https://codereview.chromium.org/714643002/diff/80001/android_webview/java/src...
File
android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java
(right):

https://codereview.chromium.org/714643002/diff/80001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:134:
return mPlayerId;
On 2014/12/04 23:33:26, boliu wrote:
> Is it safe to assert mPlayerId != INVALID_PLAYER_ID here?
My idea is that getCurrentPlayerId() should return mPlayerId, even if it is
INVALID_PLAYER_ID or something else. The user,
ReleasePlayerOfExternalVideoSurfaceIfNeeded(), interpretates if the value is
sane or not.

> I'm concerned that the implementation in content/ is making assumptions about
> the value of a constant defined in android_webview/
The value of the constant, INVALID_PLAYER_ID in android_webview, should not
matter to content. The first player has id 0 because RendererMediaPlayerManager
starts with next_media_player_id_(0).
ReleasePlayerOfExternalVideoSurfaceIfNeeded() depends on this convention: if a
player id is smaller than zero, it is not a valid player id so
ReleasePlayerOfExternalVideoSurfaceIfNeeded() has nothing to do.

https://codereview.chromium.org/714643002/diff/80001/android_webview/native/e...
File android_webview/native/external_video_surface_container_impl.h (right):

https://codereview.chromium.org/714643002/diff/80001/android_webview/native/e...
android_webview/native/external_video_surface_container_impl.h:32: virtual void
ReleaseCurrentExternalVideoSurface() override;
On 2014/12/04 23:33:26, boliu wrote:
> Is this always the same as ReleaseExternalVideoSurface(GetCurrentPlayerId())?
> 
> Can we just do that from content instead of adding a new method?
Yes. That is true. Actually, ReleaseCurrentExternalVideoSurface() is just a
helper. Since it is not really needed for this bugfix I removed it from this CL.
Done.

[boliu, 2014-12-08 17:51:47]

https://codereview.chromium.org/714643002/diff/80001/android_webview/java/src...
File
android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java
(right):

https://codereview.chromium.org/714643002/diff/80001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:134:
return mPlayerId;
On 2014/12/08 16:41:08, Hugo Holgersson wrote:
> On 2014/12/04 23:33:26, boliu wrote:
> > Is it safe to assert mPlayerId != INVALID_PLAYER_ID here?
> My idea is that getCurrentPlayerId() should return mPlayerId, even if it is
> INVALID_PLAYER_ID or something else. The user,
> ReleasePlayerOfExternalVideoSurfaceIfNeeded(), interpretates if the value is
> sane or not.
> 
> > I'm concerned that the implementation in content/ is making assumptions
about
> > the value of a constant defined in android_webview/
> The value of the constant, INVALID_PLAYER_ID in android_webview, should not
> matter to content. The first player has id 0 because
RendererMediaPlayerManager
> starts with next_media_player_id_(0).
> ReleasePlayerOfExternalVideoSurfaceIfNeeded() depends on this convention: if a
> player id is smaller than zero, it is not a valid player id so
> ReleasePlayerOfExternalVideoSurfaceIfNeeded() has nothing to do.

You just outlined exactly how content code depends on INVALID_PLAYER_ID

https://codereview.chromium.org/714643002/diff/100001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/714643002/diff/100001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:382:
external_video_surface_container_->ReleaseExternalVideoSurface(player_id);
Shouldn't this be the player id of the one holding the surface, not the one
that's about to go full screen?

https://codereview.chromium.org/714643002/diff/100001/content/public/browser/...
File content/public/browser/android/external_video_surface_container.h (right):

https://codereview.chromium.org/714643002/diff/100001/content/public/browser/...
content/public/browser/android/external_video_surface_container.h:36: virtual
int GetCurrentPlayerId() = 0;
How about define INVALID_PLAYER_ID in this class. Then
external_video_surface_container_impl.cc can convert the java one in
android_webivew/ to the native one in content/?

Update the comment about this too of course.

[Hugo Holgersson, 2014-12-09 15:35:28]

https://codereview.chromium.org/714643002/diff/100001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/714643002/diff/100001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:382:
external_video_surface_container_->ReleaseExternalVideoSurface(player_id);
> Shouldn't this be the player id of the one holding the surface, not the one
> that's about to go full screen?

Yes, you are right about when entering fullscreen we always need to release the
external surface (no matter who owns it).
- ReleasePlayerOfExternalVideoSurfaceIfNeeded(player_id) will release the
surface if it IS NOT held by player_id.
- ReleaseExternalVideoSurface(player_id) will release the surface if it IS held
by player_id.

https://codereview.chromium.org/714643002/diff/100001/content/public/browser/...
File content/public/browser/android/external_video_surface_container.h (right):

https://codereview.chromium.org/714643002/diff/100001/content/public/browser/...
content/public/browser/android/external_video_surface_container.h:36: virtual
int GetCurrentPlayerId() = 0;
> How about define INVALID_PLAYER_ID in this class.
Done.

> Then
> external_video_surface_container_impl.cc can convert the java one in
> android_webivew/ to the native one in content/?
> 
> Update the comment about this too of course.

Sure! Done. GetCurrentPlayerId() now returns kInvalidPlayerId if it finds
INVALID_PLAYER_ID. I don't know JNI very well, so PTAL.

[boliu, 2014-12-09 18:15:18]

android_webview % last comment

Min, you probably want to scan the content change although I think it's ok.

For chromecast, doesn't the changes in external_video_surface_container_impl.cc
need to be made to chromecast copy as well?

https://codereview.chromium.org/714643002/diff/100001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/714643002/diff/100001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:382:
external_video_surface_container_->ReleaseExternalVideoSurface(player_id);
On 2014/12/09 15:35:28, Hugo Holgersson wrote:
> > Shouldn't this be the player id of the one holding the surface, not the one
> > that's about to go full screen?
> 
> Yes, you are right about when entering fullscreen we always need to release
the
> external surface (no matter who owns it).
> - ReleasePlayerOfExternalVideoSurfaceIfNeeded(player_id) will release the
> surface if it IS NOT held by player_id.
> - ReleaseExternalVideoSurface(player_id) will release the surface if it IS
held
> by player_id.

o_o

https://codereview.chromium.org/714643002/diff/120001/android_webview/native/...
File android_webview/native/external_video_surface_container_impl.cc (right):

https://codereview.chromium.org/714643002/diff/120001/android_webview/native/...
android_webview/native/external_video_surface_container_impl.cc:50: int
invalid_player = static_cast<int>(env->GetStaticIntField(cls, fid));
We basically never write manual jni code in chromium. The java/native classes
are generally considered tightly coupled so assuming INVALID_PLAYER_ID = -1 in
native code is ok. (It's certainly better than assuming there's a static int
member variable called INVALID_PLAYER_ID)

But content/ and things outside of content/ have a strict API in content/public,
so should make them loosely coupled.

https://codereview.chromium.org/714643002/diff/120001/android_webview/native/...
android_webview/native/external_video_surface_container_impl.cc:56: if
(current_player == invalid_player)
You can just do if (current_player < 0) here.

[boliu, 2014-12-09 18:15:39]

err, meant to say

android_webview lgtm % last comment

[Hugo Holgersson, 2014-12-10 09:44:34]

hugo.holgersson@sonymobile.com changed reviewers:
+ gunsch@chromium.org

[Hugo Holgersson, 2014-12-10 09:51:51]

> We basically never write manual jni code in chromium.
Done. I removed the JNI. Thanks!

> You can just do if (current_player < 0) here.
Done.

> Min, you probably want to scan the content change although I think it's ok.
@qinmin, PTAL.

> For chromecast, doesn't the changes in
external_video_surface_container_impl.cc
> need to be made to chromecast copy as well?
@gunsch, PTAL.

[gunsch, 2014-12-10 16:45:52]

chromecast/ rs lgtm

[Hugo Holgersson, 2014-12-22 09:47:05]

The CQ bit was checked by hugo.holgersson@sonymobile.com

[commit-bot: I haz the power, 2014-12-22 09:47:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/714643002/140001

[commit-bot: I haz the power, 2014-12-22 09:53:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-22 09:53:51]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Hugo Holgersson, 2014-12-22 10:08:01]

hugo.holgersson@sonymobile.com changed reviewers:
+ mkosiba@chromium.org

[Hugo Holgersson, 2014-12-22 10:15:05]

@mkosiba, could you look at
content/public/browser/android/external_video_surface_container.h ?
chromium_presubmit reminded me that an OWNER never lgtm'ed this file.

[Hugo Holgersson, 2015-01-05 11:42:10]

hugo.holgersson@sonymobile.com changed reviewers:
+ tedchoc@chromium.org

[Hugo Holgersson, 2015-01-05 11:44:35]

I imagine mkosiba is out of office.
@tedchoc, would you mind reviewing content/public/browser/android ?

[mkosiba (inactive), 2015-01-05 14:58:24]

On 2014/12/22 10:15:05, Hugo Holgersson wrote:
> @mkosiba, could you look at
> content/public/browser/android/external_video_surface_container.h ?
> chromium_presubmit reminded me that an OWNER never lgtm'ed this file.

sorry for not replying. I must have missed this when I came back. LGTM + some
drive-bys

https://codereview.chromium.org/714643002/diff/140001/chromecast/browser/andr...
File
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/ExternalVideoSurfaceContainer.java
(right):

https://codereview.chromium.org/714643002/diff/140001/chromecast/browser/andr...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/ExternalVideoSurfaceContainer.java:129:
mPlayerId = playerId;
assert playerId > 0 ?

https://codereview.chromium.org/714643002/diff/140001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/714643002/diff/140001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:343: int
current_player = -1;
maybe explicitly initialize to kInvalidPlayerId ?

[Hugo Holgersson, 2015-01-05 15:56:24]

Patchset #9 (id:160001) has been deleted

[Hugo Holgersson, 2015-01-05 16:02:24]

Thanks!

https://codereview.chromium.org/714643002/diff/140001/chromecast/browser/andr...
File
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/ExternalVideoSurfaceContainer.java
(right):

https://codereview.chromium.org/714643002/diff/140001/chromecast/browser/andr...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/ExternalVideoSurfaceContainer.java:129:
mPlayerId = playerId;
On 2015/01/05 14:58:23, mkosiba wrote:
> assert playerId > 0 ?
The only value of playerId this class cannot support would be INVALID_PLAYER_ID.
I added this assert. Done.

https://codereview.chromium.org/714643002/diff/140001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/714643002/diff/140001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:343: int
current_player = -1;
On 2015/01/05 14:58:24, mkosiba wrote:
> maybe explicitly initialize to kInvalidPlayerId ?

Done.

[Hugo Holgersson, 2015-01-05 17:04:00]

The CQ bit was checked by hugo.holgersson@sonymobile.com

[commit-bot: I haz the power, 2015-01-05 17:04:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/714643002/200001

[commit-bot: I haz the power, 2015-01-05 17:52:57]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2015-01-05 17:53:37]

Patchset 10 (id:??) landed as
https://crrev.com/27e091ab97b6bd36c3a19c263d27d440bb71a9e0
Cr-Commit-Position: refs/heads/master@{#309918}