Re: Support fullscreen for non-video elements in the WebView. (issue 618013003 by igsolla@chromium.org)

[sie...@chromium.org]

Why do we need a fullscreen video view when you go into fullscreen for a
non-video element?
I thought that normally works through compositing with the UI being hidden.


https://codereview.chromium.org/618013003/

[sie...@chromium.org]

https://codereview.chromium.org/618013003/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/618013003/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode365
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:365:
private ContentVideoView mVideoView;
Can you avoid holding this ref here?

https://codereview.chromium.org/618013003/

[igs...@chromium.org]

It turns that we can. There can only be at most 1 single ContentVidewView
at any
one time, and ContentVideoView.getContentVideoView returns that instance.

https://codereview.chromium.org/618013003/

[igs...@chromium.org]

On 2014/10/10 02:06:15, sievers wrote:
> Why do we need a fullscreen video view when you go into fullscreen for a
> non-video element?
> I thought that normally works through compositing with the UI being
> hidden.

In short, because we want to avoid having two different Android APIs for
fullscreen, one for video and one for non-video.

By creating a fullscreen view apps that support fullscreen video will start
supporting fullscreen for non-video transparently without having to perform
any
changes to their app, as WebChromeClient.{onShow|onHide}CustomView will
already
work for non-video.

From the point of view of the app it is probably also simpler to
attach/detach a
completely new view than having to move the existing WebView around.

https://codereview.chromium.org/618013003/

[igs...@chromium.org]

https://codereview.chromium.org/618013003/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/618013003/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode365
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:365:
private ContentVideoView mVideoView;
On 2014/10/10 02:15:25, sievers wrote:
> Can you avoid holding this ref here?

To avoid holding this ref we would need to return a boolean from
FullscreenController::did{Enter|Exit}FullScreen() to tell us whether the
element is a video, and then either propagate it in the IPC or only send
the IPC for non-video elements.

Android is the only platform that needs this, so I would prefer not
modifying blink's API as we can already use this mechanism to detect
whether the element is a video.

https://codereview.chromium.org/618013003/

[igs...@chromium.org]

https://codereview.chromium.org/618013003/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/618013003/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode365
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:365:
private ContentVideoView mVideoView;

On 2014/10/10 09:23:41, Ignacio Solla wrote:
> On 2014/10/10 02:15:25, sievers wrote:
> > Can you avoid holding this ref here?

> To avoid holding this ref we would need to return a boolean from
> FullscreenController::did{Enter|Exit}FullScreen() to tell us whether
the element
> is a video, and then either propagate it in the IPC or only send the
IPC for
> non-video elements.

> Android is the only platform that needs this, so I would prefer not
modifying
> blink's API as we can already use this mechanism to detect whether the
element
> is a video.

Please ignore this comment, I have published it by mistake. It turns out
that there is a way in which we can avoid holding this ref, see the
changes or the other comment.

https://codereview.chromium.org/618013003/

[igs...@chromium.org]

Also note that we don't create a fullscreen video view (ContentVideoView)
for
non-video. We only create a FullscreenView (for video this renders the html5
controls, for non-video this renders the element in fullscreen). A
FullscreenView is only created in the WebView. Clank behaves as you
described.

https://codereview.chromium.org/618013003/

[sie...@chromium.org]

On 2014/10/10 09:23:41, Ignacio Solla wrote:

https://codereview.chromium.org/618013003/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> File

content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (right):


https://codereview.chromium.org/618013003/diff/60001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode365

> Android is the only platform that needs this, so I would prefer not
> modifying
> blink's API as we can already use this mechanism to detect whether the
> element
> is a video.

I'm confused. This whole issue is about HTML5 fullscreen, right?
Chrome incl. Chrome for Android supports this, only Android WebView does
not it
seems.

This comes from Blink (FullscreenController::enterFullScreenForElement())
and
goes to WebContentsDelegate::ToggleFullscreenModeForTab() in the browser.
android_webview/native/aw_web_contents_delegate.cc seems to implement
something
there. The embedder can then hide the UI and resize the view. And you can
give
the View to the WebView embedder if you want to do it that way. What's not
working for you with the existing content API that works for all other
embedders?

I would not conflate HTML5 fullscreen with WMPAndroid fullscreen like I
*think*
this patch does.
They are different things. The latter one is an Android implementation
detail
for DRM (hole punching with the compositor) and maybe a perf optimization
for
pure media elements where we can put video directly into an overlay without
compositing.

If WebView exposes wants to expose both features to its embedder in a
similar
fashion, that sounds fine. But should be an implementation detail there and
not
cause things to get mixed up in the shared code. And it shouldn't expose
another
fullscreen API like here in ContentViewClient.





https://codereview.chromium.org/618013003/

[igs...@chromium.org]

In short, the existing content API does not notify the embedder about
whether we
are
entering fullscreen for a video element or not and the WebView needs to know
this.

First, let me explain the background behind this change and hopefully clear
your confusion. Also, I will propose an alternative solution that I'd be
happy
to implement if you prefer.

Currently, the Android WebView does only support fullscreen for video
elements.
Whenever we want to enter/exit fullscreen we notify the app by calling
WebChromeClient.onShowCustomView/onHideCustomView:

http://developer.android.com/reference/android/webkit/WebChromeClient.html

The app then decides where they want to place the custom view, it might
occupy
the whole screen, or just a part of it. Note that the WebView does not own
the
view
hierarchy so we'are not allowed to modify it (this is different to Clank).
In
the
WebView it is up to the app to decide what to do when entering fullscreen.

As you noted, in order to support the MSE/EME extensions we need hardware
acceleration. For
that we create the ContentVideoView. So to enter fullscreen we call
onShowCustomView and pass
a ViewGroup containing the ContentVideoView (to render the video frames)
and a
FullscreenView
(to render the html5 video controls).

Now we want to add support for fullscreen for non-video elements and we
need to
ensure that:
- the above still applies, only the app can choose where to place the custom
view
- the same API is shared between video and non-video
- support for non-video does not break backwards compatibility
- (nice to have) support for non-video works out of the box and does not
require
modifications
to the apps

Given those constraints we'd like to reuse the existing WebView API. So for
non-video we will need to call onShowCustomView and pass just a
FullscreenView
(to render the
non-video element in fullscreen). So in summary, THE WEBVIEW NEEDS TO KNOW
WHETHER WE ARE GOING
FULLSCREEN ON A VIDEO ELEMENT OR NOT BECAUSE:

- for video we need to call onShowCustomView with a ViewGroup containing a
FullscreenView and
a ContentVideoView
- for non-video we need to call onShowCustomView with just a FullscreenView.

With the existing content API we don't know whether we are going fullscreen
on a
video element
or not. I could modify the ToggleFullscreenMode API to take an extra
argument
(bool isVideo) as
shown here:
https://codereview.chromium.org/652673002/
https://codereview.chromium.org/646403002/

Let me know if you prefer that solution and I'd happily implement it here.
It is
not a clear-cut
decision, but there are reasons why I prefer the approach in this patch.
Let me
explain those.

With the current approach only ContentViewCore needs to be aware of the
distinction between
video and non-video. I believe that this makes the code simpler and easier
to
understand. It also
makes the order of events consistent:

1. toggleFullscreenMode is called
2. didEnterFullscreenMode is called
3. (only for video) ContentVideoView is created
4. and finally onShowCustomView is called

If instead we modify the ToggleFullscreenMode API then we need to propagate
the
isVideo argument
from blink to the embedder, which means that blink, IPCs, etc. also need to
be
aware of the distinction.
It also makes the order in which events happen inconsistent between video
and
non video:

- for video:

1. ToggleFullscreen is called
2. DidEnterFullscreen is called
3. ContentVideoView is created
3. onShowCustomView is called

- for non-video:

1. ToggleFullscreen is called
2. onShowCustomView is called
3. DidEnterFullscreen is called


So in summary I think that the current approach makes the code simpler. But
if
you prefer
I could modify the ToggleFullscreen API instead. A different solution that I
haven't explored is
to create the ContentVideoView before
AwWebContentsDelegate.toggleFullscreenModeForTab is called,
but I suspect that that's not possible because we should only create the
ContentVideoView when
FullscreenController.didEnterFullscreen is called.

Please do let me know if things are clearer now.


https://codereview.chromium.org/618013003/

[sie...@chromium.org]

Maybe it might be easier to sort this out in chat or or quick video meeting?
I feel like you are still mixing up WebView requirements and decisions with
regard to that API with ones that concern the common code.

The only two requirements I can distill out of this easily are:

1) need to know whether you are entering or exiting fullscreen.
Well all platforms need to know this and they somehow manage with
the 'toggle'
API. I assume it's because you can simply cache what state you are in.

2) need to differentiate between the special video fullscreen mode and
video-element only Android-specific optimization.
Chrome for Android manages to do that just fine with the current API.



https://codereview.chromium.org/618013003/

[igs...@chromium.org]

I have invited you to a quick vc meeting tomorrow to discuss. The current
content API does not provide enough information for us to implement the
WebView
API. I think that you are mixing Clank requirements with WebView
requirements.
WebView requirements are more complex because the WebView framework does
not own
the view hiearchy.

https://codereview.chromium.org/618013003/

[igs...@chromium.org]

It turns out that I missed one option that Daniel pointed out. When the
browser
receives the ToggleFullscreenModeForTab IPC, we ask the app to show a custom
ViewGroup. If later a ContentVideoView is created then we add it to the
custom
ViewGroup. This simplifies things. Thanks Daniel!

Please PTAL.

https://codereview.chromium.org/618013003/

[sie...@chromium.org]

https://codereview.chromium.org/618013003/diff/310001/content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/618013003/diff/310001/content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java#newcode431
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:431:
contentViewCore.getContentVideoViewClient());
nit: contentViewCore.getContentVideoViewClient() -> client

https://codereview.chromium.org/618013003/diff/310001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/618013003/diff/310001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode2709
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2709:
mWebContents.exitFullscreen();
Is this needed? Doesn't AwContents have access to the WebContents?

We've really been wanting to make ContentViewCore less of a poorly
defined super-blob of API surface (mixed public and private). So for
anything that can simply go through existing functions in WebContents or
RWHV or something else, that'd be much preferred :)

https://codereview.chromium.org/618013003/

[igs...@chromium.org]

https://codereview.chromium.org/618013003/diff/310001/content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/618013003/diff/310001/content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java#newcode431
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:431:
contentViewCore.getContentVideoViewClient());

On 2014/10/15 18:16:38, sievers wrote:
> nit: contentViewCore.getContentVideoViewClient() -> client

Done.

https://codereview.chromium.org/618013003/diff/310001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/618013003/diff/310001/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java#newcode2709
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2709:
mWebContents.exitFullscreen();

On 2014/10/15 18:16:38, sievers wrote:
> Is this needed? Doesn't AwContents have access to the WebContents?

> We've really been wanting to make ContentViewCore less of a poorly
defined
> super-blob of API surface (mixed public and private). So for anything
that can
> simply go through existing functions in WebContents or RWHV or
something else,
> that'd be much preferred :)

This function was already here when I started with this change. When I
rebased I assumed that it had been deleted. I did not realize that it
was moved. Using the version in WebContents now.

https://codereview.chromium.org/618013003/

[sie...@chromium.org]

lgtm

https://codereview.chromium.org/618013003/

[commi...@chromium.org]

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


https://codereview.chromium.org/618013003/

[commi...@chromium.org]

Try jobs failed on following builders:
android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tests_recipe/builds/18877)

https://codereview.chromium.org/618013003/

[igs...@chromium.org]

https://codereview.chromium.org/618013003/diff/350001/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java
(left):

https://codereview.chromium.org/618013003/diff/350001/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java#oldcode49
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java:49:
@DisableHardwareAccelerationForTest
FYI: playing fullscreen video in the WebView requires hardware
acceleration. See the documentation in
http://developer.android.com/reference/android/webkit/WebView.html or
b/15021171.

The reason why this test was passing before is because we have changed
the timing of events. The hardware check only happens when we first try
to draw into the custom ViewGroup. We now try to draw into the custom
ViewGroup before creating the ContentVideoView, the check fails, so we
don't create the ContentVideoView. Before we were creating the
ContentVideoView before the first draw.

https://codereview.chromium.org/618013003/

[commi...@chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/patch-status/618013003/350001


https://codereview.chromium.org/618013003/

[commi...@chromium.org]

Committed patchset #9 (id:350001)

https://codereview.chromium.org/618013003/

[commi...@chromium.org]

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

https://codereview.chromium.org/618013003/

[bo...@chromium.org]

https://codereview.chromium.org/618013003/diff/350001/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java
(left):

https://codereview.chromium.org/618013003/diff/350001/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java#oldcode49
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java:49:
@DisableHardwareAccelerationForTest

On 2014/10/16 13:44:19, Ignacio Solla wrote:
> FYI: playing fullscreen video in the WebView requires hardware
acceleration. See
> the documentation in
> http://developer.android.com/reference/android/webkit/WebView.html or
> b/15021171.

> The reason why this test was passing before is because we have changed
the
> timing of events. The hardware check only happens when we first try to
draw into
> the custom ViewGroup. We now try to draw into the custom ViewGroup
before
> creating the ContentVideoView, the check fails, so we don't create the
> ContentVideoView. Before we were creating the ContentVideoView before
the first
> draw.

We should add a test that video in software mode doesn't cause deadlocks
etc.

https://codereview.chromium.org/618013003/

[igs...@chromium.org]

But fullscreen video in software mode is not supported.

https://codereview.chromium.org/618013003/

[bo...@chromium.org]

Not supported doesn't mean we can deadlock in that case.

Not supported here just means the video itself won't show up, and that
should be
the only issue. If a page loads a <video> and makes it go to full screen, we
should not have any deadlocks even in software mode. Plus full screen video
used
to worked in software mode, ie the video would show up, as that didn't
require
reading and compositing a texture, so no need for hardware acceleration.

https://codereview.chromium.org/618013003/

[igs...@chromium.org]

Fair enough. I'll add a test for that in a follow-up patch.

https://codereview.chromium.org/618013003/