Support subtitles for native fullscreen video

Support subtitles for native fullscreen video

In native fullscreen video mode(used by Android), videos are rendered into a separate hardware overlay that is outside of jurisdiction of our compositor. This patch adds support for subtitle by making the WebLayerTreeView transparent, and let the RenderVideo becomes the root layer, so the media controls and subtitles rendered in Blink can be overlaid on top of the hardware overlay.

In detail, the patch does the following:

* Force fullscreen video to be composited.

* Force fullscreen video to create stacking context. This is necessary, otherwise the descendant layers won't be attached under its layer.

* Add a m_currentFullscreenRenderer property in main frame RenderLayerCompositor.

* When building GraphicsLayer tree, RenderLayerCompositor will rearrange m_currentFullscreenRenderer as the root layer if it is a fullscreen video.

* RenderLayerBacking will turn off visibility of the layer if it is a fullscreen video.

* WebViewImpl will change WebLayerTreeView transparency if a media element enters fullscreen mode.

* MediaControlsChromiumAndroid will always allow media control hiding in fullscreen (ignoring hover status).

* Native fullscreen video will be moved under RenderFullScreen as well. This step was previously skipped, and is necessary to layout media controls and subtitles correctly.

* Convert USE(NATIVE_FULLSCREEN_VIDEO) to a runtime enable feature

* A layout test virtual suite is added to simulate Android behavior on Linux.

Depends on chromium patch: https://codereview.chromium.org/23597036/

BUG=262945
R=aelias,esprehn,falken,abarth

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158166

[trchen, 2013-08-07 07:45:42]

Hello James,
This patch implements subtitles for native fullscreen video. Please let me know
if it conceptually looks right to you.

As for tests, it will be difficult to test if we hide most of the code behind
USE(NATIVE_FULLSCREEN_VIDEO). I'm thinking maybe we should remove the macro and
make it a Settings instead?

[trchen, 2013-08-22 00:01:51]

Hello falken,
Here is a fullscreen video implementation for Android. Please take a look and
let me know if you have any concern. Thanks!

[falken, 2013-08-22 12:40:34]

Hey trchen,

I still think I'm missing a lot of context before I can understand your
approach.

This is how I understand it. You're drawing media controls and subtitles
over the hardware overlay. In Blink's render tree, the media controls
and subtitles are descendants of RenderVideo. The problem is you can't
just draw the media controls and subtitles over the overlay without
drawing the rest of the document(??). So you're making all layers
invisible except for media controls and subtitles: the WebLayerTreeView
which shows the normal render tree is made invisible, and RenderVideo is
made the root layer so it's exempt from that invisibility. However, we
don't want RenderVideo to be visible(??), so RenderLayerBacking makes it
invisible, but the media controls and subtitles remain visible.

Is that close?

My primary concern is whether this locks us further into the
RenderFullScreen implementation, making it harder to migrate to top
layer for fullscreen.  It looks like this isn't really dependent on
RenderFullScreen though.

https://codereview.chromium.org/22454003/diff/1/Source/core/page/FrameView.h
File Source/core/page/FrameView.h (right):

https://codereview.chromium.org/22454003/diff/1/Source/core/page/FrameView.h#...
Source/core/page/FrameView.h:340: // Only called for main frame. Input object
can be cross frame.
Can you elaborate on this comment, it's a bit cryptic to me.

[trchen, 2013-08-22 21:55:40]

On 2013/08/22 12:40:34, falken wrote:
> Hey trchen,
> 
> I still think I'm missing a lot of context before I can understand your
> approach.
> 
> This is how I understand it. You're drawing media controls and subtitles
> over the hardware overlay. In Blink's render tree, the media controls
> and subtitles are descendants of RenderVideo. The problem is you can't
> just draw the media controls and subtitles over the overlay without
> drawing the rest of the document(??). So you're making all layers
> invisible except for media controls and subtitles: the WebLayerTreeView
> which shows the normal render tree is made invisible, and RenderVideo is
> made the root layer so it's exempt from that invisibility. However, we
> don't want RenderVideo to be visible(??), so RenderLayerBacking makes it
> invisible, but the media controls and subtitles remain visible.
> 
> Is that close?

Yes, that's 99% accurate. Except that we don't create a separate
WebLayerTreeView nor make the existing one invisible. We simply skip all other
layers when constructing the compositing layer tree.

> My primary concern is whether this locks us further into the
> RenderFullScreen implementation, making it harder to migrate to top
> layer for fullscreen.  It looks like this isn't really dependent on
> RenderFullScreen though.

Right, as long as the RenderVideo is re-layout with fullscreen width/height, we
don't care whether it's contained by RenderFullscreen or promoted to top layer.

> https://codereview.chromium.org/22454003/diff/1/Source/core/page/FrameView.h
> File Source/core/page/FrameView.h (right):
> 
>
https://codereview.chromium.org/22454003/diff/1/Source/core/page/FrameView.h#...
> Source/core/page/FrameView.h:340: // Only called for main frame. Input object
> can be cross frame.
> Can you elaborate on this comment, it's a bit cryptic to me.

This function should be only called on main frame . Calling it on a sub-frame
doesn't make sense since all it does is to replace the frame's root layer with
the input object. The input object could be from a sub-frame, e.g. a video in an
iframe can be made fullscreen.

[falken, 2013-08-23 10:32:35]

OK cool, I think the approach lg to me. Just some more questions to
make sure I understand:

- Currently, nothing is drawn over the hardware overlay, so we
never had to mess with the root layer, transparencies, etc. like
this until now?
- What happens if the RenderVideo is not made invisible? What
does it look like?

Regarding,
> we don't create a separate
> WebLayerTreeView nor make the existing one invisible

I think I was confused by the description about
"making the WebLayerTreeView transparent". I guess, you are
making the background transparent, so the hardware overlay
is seen underneath it?

On 2013/08/22 21:55:40, trchen wrote:
> On 2013/08/22 12:40:34, falken wrote:
> > Hey trchen,
> > 
> > I still think I'm missing a lot of context before I can understand your
> > approach.
> > 
> > This is how I understand it. You're drawing media controls and subtitles
> > over the hardware overlay. In Blink's render tree, the media controls
> > and subtitles are descendants of RenderVideo. The problem is you can't
> > just draw the media controls and subtitles over the overlay without
> > drawing the rest of the document(??). So you're making all layers
> > invisible except for media controls and subtitles: the WebLayerTreeView
> > which shows the normal render tree is made invisible, and RenderVideo is
> > made the root layer so it's exempt from that invisibility. However, we
> > don't want RenderVideo to be visible(??), so RenderLayerBacking makes it
> > invisible, but the media controls and subtitles remain visible.
> > 
> > Is that close?
> 
> Yes, that's 99% accurate. Except that we don't create a separate
> WebLayerTreeView nor make the existing one invisible. We simply skip all other
> layers when constructing the compositing layer tree.
> 
> > My primary concern is whether this locks us further into the
> > RenderFullScreen implementation, making it harder to migrate to top
> > layer for fullscreen.  It looks like this isn't really dependent on
> > RenderFullScreen though.
> 
> Right, as long as the RenderVideo is re-layout with fullscreen width/height,
we
> don't care whether it's contained by RenderFullscreen or promoted to top
layer.
> 
> > https://codereview.chromium.org/22454003/diff/1/Source/core/page/FrameView.h
> > File Source/core/page/FrameView.h (right):
> > 
> >
>
https://codereview.chromium.org/22454003/diff/1/Source/core/page/FrameView.h#...
> > Source/core/page/FrameView.h:340: // Only called for main frame. Input
object
> > can be cross frame.
> > Can you elaborate on this comment, it's a bit cryptic to me.
> 
> This function should be only called on main frame . Calling it on a sub-frame
> doesn't make sense since all it does is to replace the frame's root layer with
> the input object. The input object could be from a sub-frame, e.g. a video in
an
> iframe can be made fullscreen.

[trchen, 2013-08-23 11:24:13]

On 2013/08/23 10:32:35, falken wrote:
> OK cool, I think the approach lg to me. Just some more questions to
> make sure I understand:
> 
> - Currently, nothing is drawn over the hardware overlay, so we
> never had to mess with the root layer, transparencies, etc. like
> this until now?

Currently we don't support subtitles or blink media controls. We completely hide
blink during fullscreen playback.

> - What happens if the RenderVideo is not made invisible? What
> does it look like?

It displays the last frame right before fullscreen mode is entered.

> Regarding,
> > we don't create a separate
> > WebLayerTreeView nor make the existing one invisible
> 
> I think I was confused by the description about
> "making the WebLayerTreeView transparent". I guess, you are
> making the background transparent, so the hardware overlay
> is seen underneath it?

Yes exactly.

[falken, 2013-08-24 05:25:40]

lgtm

Is it possible to write a test for this?

https://codereview.chromium.org/22454003/diff/1/Source/core/rendering/RenderL...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/22454003/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerCompositor.cpp:446:
childList.append(backing->graphicsLayer());
Should you check if backing was set to 0 above?

[trchen, 2013-08-24 05:33:31]

On 2013/08/24 05:25:40, falken wrote:
> lgtm
> 
> Is it possible to write a test for this?

Yes. I'll convert the USE(NATIVE_FULLSCREEN_VIDEO) macro to a Settings member,
and write a test for Linux.

>
https://codereview.chromium.org/22454003/diff/1/Source/core/rendering/RenderL...
> File Source/core/rendering/RenderLayerCompositor.cpp (right):
> 
>
https://codereview.chromium.org/22454003/diff/1/Source/core/rendering/RenderL...
> Source/core/rendering/RenderLayerCompositor.cpp:446:
> childList.append(backing->graphicsLayer());
> Should you check if backing was set to 0 above?

Yes I should. Good catch!

[trchen, 2013-09-12 10:55:28]

+jochen for Tools/ and public/

Hello jamesr, PTAL at the RenderLayerCompositor change. Thanks!

This new patch converts the USE(NATIVE_FULLSCREEN_VIDEO) macro to a runtime
enabled feature, allowing us to test this feature on Linux.

Enabling LayoutTests/fullscreen in an Android virtual suite should provide
reasonable test coverage for this patch.

Hooking up the flags and enabling alpha channel require chromium side patch
landed first: https://codereview.chromium.org/23597036/

[trchen, 2013-09-12 21:56:27]

+esprehn, will you be able to look at the fullscreen related changes under
Source/core? Thanks!

[esprehn, 2013-09-13 02:46:07]

Keeping RenderObjects across frames is _very_ scary.

https://codereview.chromium.org/22454003/diff/13001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/22454003/diff/13001/Source/core/dom/Element.c...
Source/core/dom/Element.cpp:1358: if (FrameView* view = document().view())
All the null checks for view() look wrong in here. None of this code should even
run if there's no FrameView.

https://codereview.chromium.org/22454003/diff/13001/Source/core/dom/Element.c...
Source/core/dom/Element.cpp:3655: if (!view)
It should be impossible to get here if there's no FrameView, you don't need
those checks.

https://codereview.chromium.org/22454003/diff/13001/Source/core/page/FrameVie...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/22454003/diff/13001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:3399: if (RenderView* renderView =
this->renderView())
Going through here if there's no RenderView sounds like a bug, you can
ASSERT(renderView()) if you want, but null checking it doesn't look right.

https://codereview.chromium.org/22454003/diff/13001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/22454003/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:452: RenderLayerModelObject*
renderer = toRenderLayerModelObject(m_currentFullscreenRenderer);
SVG elements can never be full screen? They're not RenderLayerMOdelObjects so if
they can this doesn't seem safe.

https://codereview.chromium.org/22454003/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:453: RenderLayerBacking* backing
= renderer->layer() ? renderer->layer()->backing() : 0;
You should just add a method to RenderLayerModelObject like backing() and get
rid of the ternary here.

https://codereview.chromium.org/22454003/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:2781:
m_currentFullscreenRenderer = renderer;
I'm not a fan of keeping RenderObjects across frames or at all like this. I'd
rather you kept a RefPtr to a Node and called renderer() whenever you needed it.
That way you can never have a stale renderer.

https://codereview.chromium.org/22454003/diff/13001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.h (right):

https://codereview.chromium.org/22454003/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.h:210: void
setCurrentFullscreenRenderer(RenderObject*);
This doesn't look safe at all, passing a RenderObject between frames creates
really scary looking conditions where one frame goes through a styleRecalc and
the cleanup is not correct here and now you have a use-after-free.

https://codereview.chromium.org/22454003/diff/13001/Source/web/FullscreenCont...
File Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/22454003/diff/13001/Source/web/FullscreenCont...
Source/web/FullscreenController.cpp:89: if (Element* element =
FullscreenElementStack::currentFullScreenElementFrom(doc)) {
How can this Element be null? If this document is full screen shouldn't this
always be non-null per the top level if statement? It's very suspect that we
allow a Document to do didEnterFullScreen but have a null full screen element.

https://codereview.chromium.org/22454003/diff/13001/Source/web/FullscreenCont...
Source/web/FullscreenController.cpp:161: // FIXME: There is no embedder-side
handling in layout test mode.
What does this mean?

https://codereview.chromium.org/22454003/diff/13001/Source/web/WebViewImpl.h
File Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/22454003/diff/13001/Source/web/WebViewImpl.h#...
Source/web/WebViewImpl.h:563: WebLayerTreeView* layerTreeView() { return
m_layerTreeView; };
const ?

[trchen, 2013-09-13 05:14:57]

https://codereview.chromium.org/22454003/diff/13001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/22454003/diff/13001/Source/core/dom/Element.c...
Source/core/dom/Element.cpp:1358: if (FrameView* view = document().view())
On 2013/09/13 02:46:07, esprehn wrote:
> All the null checks for view() look wrong in here. None of this code should
even
> run if there's no FrameView.

Okay I'll change it to ASSERT.

https://codereview.chromium.org/22454003/diff/13001/Source/core/dom/Element.c...
Source/core/dom/Element.cpp:3655: if (!view)
On 2013/09/13 02:46:07, esprehn wrote:
> It should be impossible to get here if there's no FrameView, you don't need
> those checks.

Will change to ASSERT too.

https://codereview.chromium.org/22454003/diff/13001/Source/core/page/FrameVie...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/22454003/diff/13001/Source/core/page/FrameVie...
Source/core/page/FrameView.cpp:3399: if (RenderView* renderView =
this->renderView())
On 2013/09/13 02:46:07, esprehn wrote:
> Going through here if there's no RenderView sounds like a bug, you can
> ASSERT(renderView()) if you want, but null checking it doesn't look right.

This can happen during frame destruction. RenderView get detached before
everything else.

https://codereview.chromium.org/22454003/diff/13001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/22454003/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:452: RenderLayerModelObject*
renderer = toRenderLayerModelObject(m_currentFullscreenRenderer);
On 2013/09/13 02:46:07, esprehn wrote:
> SVG elements can never be full screen? They're not RenderLayerMOdelObjects so
if
> they can this doesn't seem safe.

This special code path only applies for RenderMedia. SVG elements use the usual
fullscreen code path.

https://codereview.chromium.org/22454003/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:453: RenderLayerBacking* backing
= renderer->layer() ? renderer->layer()->backing() : 0;
On 2013/09/13 02:46:07, esprehn wrote:
> You should just add a method to RenderLayerModelObject like backing() and get
> rid of the ternary here.

Done.

https://codereview.chromium.org/22454003/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:2781:
m_currentFullscreenRenderer = renderer;
On 2013/09/13 02:46:07, esprehn wrote:
> I'm not a fan of keeping RenderObjects across frames or at all like this. I'd
> rather you kept a RefPtr to a Node and called renderer() whenever you needed
it.
> That way you can never have a stale renderer.

That is one alternative way I have thought about. My concern is that it would be
a layer violation for the core/rendering to know anything about core/dom.
Actually I think keeping a RenderObject is safe. The only place we
construct/destruct render object is in Element::attach/detach, and I believe it
is guaranteed by design.

If in doubt, we can guard this code path with
REF::overlayFullscreenVideoEnabled() too, so we have some damage control if this
approach is in fact problematic.

https://codereview.chromium.org/22454003/diff/13001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.h (right):

https://codereview.chromium.org/22454003/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.h:210: void
setCurrentFullscreenRenderer(RenderObject*);
On 2013/09/13 02:46:07, esprehn wrote:
> This doesn't look safe at all, passing a RenderObject between frames creates
> really scary looking conditions where one frame goes through a styleRecalc and
> the cleanup is not correct here and now you have a use-after-free.

See answer in RLC.cpp

https://codereview.chromium.org/22454003/diff/13001/Source/web/FullscreenCont...
File Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/22454003/diff/13001/Source/web/FullscreenCont...
Source/web/FullscreenController.cpp:89: if (Element* element =
FullscreenElementStack::currentFullScreenElementFrom(doc)) {
On 2013/09/13 02:46:07, esprehn wrote:
> How can this Element be null? If this document is full screen shouldn't this
> always be non-null per the top level if statement? It's very suspect that we
> allow a Document to do didEnterFullScreen but have a null full screen element.

I'll change it to an ASSERT too. ;)

https://codereview.chromium.org/22454003/diff/13001/Source/web/FullscreenCont...
Source/web/FullscreenController.cpp:161: // FIXME: There is no embedder-side
handling in layout test mode.
On 2013/09/13 02:46:07, esprehn wrote:
> What does this mean?

In layout test mode there is no real hardware overlaid media player, so
mediaElement->player()->canShowFullscreenOverlay() will always return false. As
a result, the video won't be able to enter fullscreen in a layout test. It is
okay for us to use the regular IPC in the test, since the special IPC only does
extra things that are irrelevant to the renderer.

https://codereview.chromium.org/22454003/diff/13001/Source/web/WebViewImpl.h
File Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/22454003/diff/13001/Source/web/WebViewImpl.h#...
Source/web/WebViewImpl.h:563: WebLayerTreeView* layerTreeView() { return
m_layerTreeView; };
On 2013/09/13 02:46:07, esprehn wrote:
> const ?

Done.

[scherkus (not reviewing), 2013-09-13 18:14:19]

drive-by: you mention subtitles ... but I'm not seeing much in the way of
subtitle/track code being touched

if I understand this CL correctly, doesn't this enable layering any HTML content
over a native fullscreen video?

OOC how is scrolling handled?

[trchen, 2013-09-13 20:05:15]

On 2013/09/13 18:14:19, scherkus wrote:
> drive-by: you mention subtitles ... but I'm not seeing much in the way of
> subtitle/track code being touched

Hmm, I may have to change the CL title... It describes the goal but not exactly
what the patch does.

> if I understand this CL correctly, doesn't this enable layering any HTML
content
> over a native fullscreen video?

The current implementation in Android puts the video overlay on top of
everything, and adds a browser-side media control. The goal of this change is to
put the video overlay below the blink framebuffer, and let blink renders the
media control and subtitles for us. To do this correctly, we have to layout the
media control and subtitles in fullscreen size, and hide everything else on the
page. This is done by promoting the RenderVideo as the root graphics layer.

> OOC how is scrolling handled?

Scrolling will be ignored.

[trchen, 2013-09-20 01:38:23]

Hello esprehn,
I just added the dangling pointer test you recommended. Due to the way it is
implemented, it will require an extra bitfield in RenderObject. I'm only making
it a debug ASSERT for now. The debug layout tests look healthy with it.

[esprehn, 2013-09-20 22:01:08]

This really doesn't feel right adding all this logic into recalcStyle and
keeping RenderObject* pointers across frame boundaries. We should just keep a
RefPtr<Element> to the current full screen element and when you want the
renderer you should just go get it. You can do this by calling
webkitFullscreenElement()->renderer(), why do you need to keep this ptr in the
RenderLayerCompositor?

If we do land this the ASSERT should also be in RenderObject::destroy() and it
should actually check the real ptr is not |this| (or it should clear it). Doing
this inside Element::detach doesn't look right.

https://codereview.chromium.org/22454003/diff/37001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/22454003/diff/37001/Source/core/dom/Document....
Source/core/dom/Document.cpp:2004:
renderView()->setCurrentFullscreenRenderer(0);
We're going to destroy the RenderView right after this (see a few lines down)
which should clear this right?

https://codereview.chromium.org/22454003/diff/37001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/22454003/diff/37001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.cpp:248: ASSERT(!m_isCurrentFullscreen);
Instead of this bitfield you should add

ASSERT(renderView()->currentFullScreenRenderer() != this)

[esprehn, 2013-09-21 00:25:31]

LGTM please make LayoutTests/virtual/android/fullscreen/video-controls-override
a text test though, all the output is text :)

[trchen, 2013-09-21 00:36:41]

Hello abarth,
Could you take a look at the Source/web and public/web part of the patch? Thank
you!

[aelias_OOO_until_Jul13, 2013-09-21 02:30:18]

Source/web lgtm

[esprehn, 2013-09-21 02:54:49]

ojan had wondered why I had been like "this patch doesn't feel right" and then
LGTM'ed it. The reason is that I discussed with trchen through chat and we
decided to take a different approach which is what is in Patch Set 5 "Remove
scary cross-frame pointer" and looks great.

[abarth-chromium, 2013-09-21 20:56:56]

https://codereview.chromium.org/22454003/diff/104001/public/web/WebRuntimeFea...
File public/web/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/22454003/diff/104001/public/web/WebRuntimeFea...
public/web/WebRuntimeFeatures.h:149: WEBKIT_EXPORT static void
enableOverlayFullscreenVideo(bool);
public/web <-- LGTM

[commit-bot: I haz the power, 2013-09-23 04:07:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/22454003/118001

[commit-bot: I haz the power, 2013-09-23 08:11:38]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-09-23 08:14:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/22454003/118001

[commit-bot: I haz the power, 2013-09-23 08:22:18]

Change committed as 158166