Fix leaked external surface when several EME videos coexist

Fix leaked external surface when several EME videos coexist

Background: In the code we see that BrowserMediaPlayerManager can only hold
one ExternalVideoSurfaceContainer and a ExternalVideoSurfaceContainer can
only hold one SurfaceView.

Problem: When ExternalVideoSurfaceContainer starts holding a new surface,
createSurfaceView() will change mSurfaceView to the new SurfaceView. It
does not release the old SurfaceView before it looses its reference to it.
Leaving a SurfaceView behind could cause Z-fights with other SurfaceViews,
such as fullscreen video playback.

Solution: By deallocating player A's SurfaceView when player B requests one
we ensure that no SurfaceView is leaked. To ensure deallocation we always
call setActiveContainer(this) before createSurfaceView().

BUG=431318
TEST=Flip between two EME videos, see crbug.com/431318 for detailed info.

Committed: https://crrev.com/50f8b8e49b1aac03afe9ae00747812cdd0a118b0
Cr-Commit-Position: refs/heads/master@{#304191}

[Hugo Holgersson, 2014-11-07 15:55:58]

hugo.holgersson@sonymobile.com changed reviewers:
+ wonsik@chromium.org, ycheo@chromium.org

[Hugo Holgersson, 2014-11-07 16:10:25]

I would be happy if you could have a look at my upload :)

[Hugo Holgersson, 2014-11-10 14:46:21]

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

[Hugo Holgersson, 2014-11-10 14:50:47]

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

[Hugo Holgersson, 2014-11-10 14:50:48]

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

[xhwang, 2014-11-10 17:03:26]

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

[xhwang, 2014-11-10 17:03:27]

+qinmin, who knows this code best.

[boliu, 2014-11-10 18:03:39]

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

https://codereview.chromium.org/709003004/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:170:
mSurfaceView = new NoPunchingSurfaceView(mContentViewCore.getContext());
Can you also add an assert here that mSurfaceView is null?

[ycheo (away), 2014-11-10 22:58:46]

ycheo@chromium.org changed reviewers:
+ gunsch@chromium.org

[ycheo (away), 2014-11-10 22:58:46]

@gunsch, could you check if the change has no side-effects on ChromeCast side?

https://codereview.chromium.org/709003004/diff/1/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/709003004/diff/1/AUTHORS#newcode183
AUTHORS:183: Hugo Holgersson <hugo.holgersson@sonymobile.com>
You need to sign CLA:
http://www.chromium.org/developers/contributing-code/external-contributor-che...
In addition, you'd better split the CL only for the author.

[boliu, 2014-11-10 23:20:16]

@gunsch: Should the same change be made to
//src/chromecast/browser/android/apk/src/org/chromium/chromecast/shell/ExternalVideoSurfaceContainer.java
?

Can the ExternalVideoSurfaceContainer code be factored out into a component so
it can be shared betweena android_webview and chromecast?

[gunsch, 2014-11-10 23:51:59]

On 2014/11/10 23:20:16, boliu wrote:
> @gunsch: Should the same change be made to
>
//src/chromecast/browser/android/apk/src/org/chromium/chromecast/shell/ExternalVideoSurfaceContainer.java
> ?
> 
> Can the ExternalVideoSurfaceContainer code be factored out into a component so
> it can be shared betweena android_webview and chromecast?

@boliu: IIRC last time this question was asked, the answer was that content/
OWNERS were not comfortable with this implementation living in content/ to be
shared, so we duplicated it instead. I have no objection to refactoring it to
live either in content/ or a new component/, however.

CL itself: I'm not sure if this is the correct solution. We encountered some
similar problems on Chromecast (see crbug.com/422158) and this should be fixed
by https://codereview.chromium.org/651243003. If not, can we investigate why the
previous surface is not torn down before the new one is requested?

[boliu, 2014-11-11 00:05:48]

On 2014/11/10 23:51:59, gunsch wrote:
> @boliu: IIRC last time this question was asked, the answer was that content/
> OWNERS were not comfortable with this implementation living in content/ to be
> shared, so we duplicated it instead. I have no objection to refactoring it to
> live either in content/ or a new component/, however.

I meant component. Filed crbug.com/431928 for this. Let's not derail this CL
more :p

> 
> CL itself: I'm not sure if this is the correct solution. We encountered some
> similar problems on Chromecast (see crbug.com/422158) and this should be fixed
> by https://codereview.chromium.org/651243003. If not, can we investigate why
the
> previous surface is not torn down before the new one is requested?

crbug.com/422158 is not a public bug. It doesn't look like the same issue. That
bug is about a stuck decoder. This fix is about a stuck SurfaceView. Min or
anyone knowledgeable, correct me if I'm wrong?

[boliu, 2014-11-11 00:07:19]

On 2014/11/10 23:51:59, gunsch wrote:
> If not, can we investigate why the
> previous surface is not torn down before the new one is requested?

Because of the bug this CL is trying to fix :)

[Hugo Holgersson, 2014-11-11 15:35:10]

I assumed we should fix also for Chromecast (so files does not divert before
factoring out).

https://codereview.chromium.org/709003004/diff/1/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/709003004/diff/1/AUTHORS#newcode183
AUTHORS:183: Hugo Holgersson <hugo.holgersson@sonymobile.com>
On 2014/11/10 22:58:46, Yuncheol Heo wrote:
> You need to sign CLA:
>
http://www.chromium.org/developers/contributing-code/external-contributor-che...
> In addition, you'd better split the CL only for the author.

Done.

[gunsch, 2014-11-11 16:29:52]

@boliu: My concern is that this is just a symptom. There are other things that
are released and then reacquired, e.g. the MediaCodec and/or MediaDrm instances,
which can also behavior incorrectly if two of them are being used (secure
MediaCodec instances, for example). So I'm more interested in asking what the
root cause is that is triggering a second surface being allocated before the
first is released, because I worry there are other side effects of the same root
cause.

qinmin@ should really weigh in on this.

[qinmin, 2014-11-12 00:25:49]

On 2014/11/11 16:29:52, gunsch wrote:
> @boliu: My concern is that this is just a symptom. There are other things that
> are released and then reacquired, e.g. the MediaCodec and/or MediaDrm
instances,
> which can also behavior incorrectly if two of them are being used (secure
> MediaCodec instances, for example). So I'm more interested in asking what the
> root cause is that is triggering a second surface being allocated before the
> first is released, because I worry there are other side effects of the same
root
> cause.
> 
> qinmin@ should really weigh in on this.

For EME, android only supports 1 encrypted video playing at a time. 
So if 2nd video starts playing, we should release the first one(rather than
pause it).
For MSE, we should do the same if the video is using external surface.
This is because there will be more than 1 surface view doing the hole punching,
and android doesn't support that well.
I am ok with this change, but for the other change, we'd better do release
rather than simply pause

[boliu, 2014-11-12 00:28:32]

android_webview lgtm

[gunsch, 2014-11-12 00:29:58]

chromecast lgtm

[Hugo Holgersson, 2014-11-12 09:29:40]

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

[commit-bot: I haz the power, 2014-11-12 09:30:58]

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

[commit-bot: I haz the power, 2014-11-12 09:34:21]

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

[commit-bot: I haz the power, 2014-11-12 09:34:22]

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

[boliu, 2014-11-14 01:45:54]

On 2014/11/12 09:34:22, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Pre-emptive lgtm for AUTHORS change (ie pulling in
https://codereview.chromium.org/716143002/ into this CL)

I checked that sonymobile already signed a corporate CLA

[Hugo Holgersson, 2014-11-14 09:33:21]

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

[commit-bot: I haz the power, 2014-11-14 09:33:59]

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

[commit-bot: I haz the power, 2014-11-14 10:22:18]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-11-14 10:23:02]

Patchset 3 (id:??) landed as
https://crrev.com/50f8b8e49b1aac03afe9ae00747812cdd0a118b0
Cr-Commit-Position: refs/heads/master@{#304191}