Exclude more CSS properties for fullscreen video on Android.

Exclude more CSS properties for fullscreen video on Android.

This CL overrides additional CSS properties that prevent accelerated overlays from being used:
  - -webkit-filter
  - opacity
  - border-radius
  - mix-blend-mode

And two that are rendered incorrectly:
  - background-image
  - border

This is especially important because there is no non-overlay fallback.

This mechanism is expected to be replaced by compositor changes in M53.

BUG=618753
Committed: https://crrev.com/f33109c4d3bdafe0a0c07c59bd1bf1639595dc8a
Cr-Commit-Position: refs/heads/master@{#401923}

[sandersd (OOO until July 31), 2016-06-09 22:01:08]

sandersd@chromium.org changed reviewers:
+ foolip@chromium.org

[sandersd (OOO until July 31), 2016-06-09 22:03:32]

Description was changed from

==========
Exclude more CSS properties for fullscreen video.

This CL overrides additional CSS properties that prevent accelerated
overlays from being used.
  - -webkit-filter
  - opacity
  - border-radius
  - mix-blend-mode

This is especially important on Android where there is no non-overlay
fallback.

BUG=618753
==========

to

==========
Exclude more CSS properties for fullscreen video.

This CL overrides additional CSS properties that prevent accelerated
overlays from being used:
  - -webkit-filter
  - opacity
  - border-radius
  - mix-blend-mode

This is especially important on Android where there is no non-overlay
fallback.

BUG=618753
==========

[foolip, 2016-06-10 05:38:48]

I'm OOO today, but to begin with can you file an issue at
https://github.com/whatwg/fullscreen/issues/new to have these added to the spec?

[sandersd (OOO until July 31), 2016-06-10 21:23:05]

On 2016/06/10 05:38:48, Philip J. (OOO June 10) wrote:
> I'm OOO today, but to begin with can you file an issue at
> https://github.com/whatwg/fullscreen/issues/new to have these added to the
spec?

Is there a way to get the same effect as this CL, but only for Android?

I'm dubious about standardizing a some of these, but changing the compositor
code has not been fruitful.

[foolip, 2016-06-13 14:00:26]

On 2016/06/10 21:23:05, sandersd wrote:
> On 2016/06/10 05:38:48, Philip J. (OOO June 10) wrote:
> > I'm OOO today, but to begin with can you file an issue at
> > https://github.com/whatwg/fullscreen/issues/new to have these added to the
> spec?
> 
> Is there a way to get the same effect as this CL, but only for Android?
> 
> I'm dubious about standardizing a some of these, but changing the compositor
> code has not been fruitful.

Oh, is this actually a compositor bug? Would other browsers behave differently
given the same computed style for all elements?

If limiting this to Android it would still be a web-observable change, so that
wouldn't really help.

Is there a specific code path where accelerated overlays kick in where it's a
bit more obvious why this change works? Might it be fixed by
https://bugs.chromium.org/p/chromium/issues/detail?id=240576 too?

[sandersd (OOO until July 31), 2016-06-13 17:55:36]

On 2016/06/13 14:00:26, Philip Jägenstedt wrote:
> On 2016/06/10 21:23:05, sandersd wrote:
> > On 2016/06/10 05:38:48, Philip J. (OOO June 10) wrote:
> > > I'm OOO today, but to begin with can you file an issue at
> > > https://github.com/whatwg/fullscreen/issues/new to have these added to the
> > spec?
> > 
> > Is there a way to get the same effect as this CL, but only for Android?
> > 
> > I'm dubious about standardizing a some of these, but changing the compositor
> > code has not been fruitful.
> 
> Oh, is this actually a compositor bug? Would other browsers behave differently
> given the same computed style for all elements?

I suppose that is a technically correct categorization.

> If limiting this to Android it would still be a web-observable change, so that
> wouldn't really help.

Unfortunately, it's wrong either way, and the way it is wrong without this
change is worse. The problem on Android is that once the decoder has chosen to
output zero-copy frames, they can in fact only be displayed as hardware
underlays. Therefore any path that disables hardware overlays (any kind of
post-processed effect) results in the video disappearing. (More precisely, the
compositor can only punch holes in the root render pass.)

Since we are forcing this mode for fullscreen H.264 for power reasons, we need
to be sure that these kinds of effect won't be applied for fullscreen video on
Android.

> Is there a specific code path where accelerated overlays kick in where it's a
> bit more obvious why this change works? Might it be fixed by
> https://bugs.chromium.org/p/chromium/issues/detail?id=240576 too?

Yes, when the new unified media pipeline is enabled (default), one of these
properties is applied, and the video element is fullscreened, the video
disappears.

I do not believe that the top layer change will make a difference, because we
are already changing the root rendering node to the video layer in this case.

[sandersd (OOO until July 31), 2016-06-14 20:14:57]

Description was changed from

==========
Exclude more CSS properties for fullscreen video.

This CL overrides additional CSS properties that prevent accelerated
overlays from being used:
  - -webkit-filter
  - opacity
  - border-radius
  - mix-blend-mode

This is especially important on Android where there is no non-overlay
fallback.

BUG=618753
==========

to

==========
Exclude more CSS properties for fullscreen video on Android.

This CL overrides additional CSS properties that prevent accelerated overlays
from being used:
  - -webkit-filter
  - opacity
  - border-radius
  - mix-blend-mode

And two that are rendered incorrectly:
  - background-image
  - border

This is especially important on Android where there is no non-overlay fallback.

This mechanism is expected to be replaced by compositor changes in M53.

BUG=618753
==========

[sandersd (OOO until July 31), 2016-06-14 20:15:20]

Description was changed from

==========
Exclude more CSS properties for fullscreen video on Android.

This CL overrides additional CSS properties that prevent accelerated overlays
from being used:
  - -webkit-filter
  - opacity
  - border-radius
  - mix-blend-mode

And two that are rendered incorrectly:
  - background-image
  - border

This is especially important on Android where there is no non-overlay fallback.

This mechanism is expected to be replaced by compositor changes in M53.

BUG=618753
==========

to

==========
Exclude more CSS properties for fullscreen video on Android.

This CL overrides additional CSS properties that prevent accelerated overlays
from being used:
  - -webkit-filter
  - opacity
  - border-radius
  - mix-blend-mode

And two that are rendered incorrectly:
  - background-image
  - border

This is especially important because there is no non-overlay fallback.

This mechanism is expected to be replaced by compositor changes in M53.

BUG=618753
==========

[sandersd (OOO until July 31), 2016-06-14 20:24:06]

On 2016/06/13 17:55:36, sandersd wrote:
> On 2016/06/13 14:00:26, Philip Jägenstedt wrote:
> > On 2016/06/10 21:23:05, sandersd wrote:
> > > On 2016/06/10 05:38:48, Philip J. (OOO June 10) wrote:
> > > > I'm OOO today, but to begin with can you file an issue at
> > > > https://github.com/whatwg/fullscreen/issues/new to have these added to
the
> > > spec?
> > > 
> > > Is there a way to get the same effect as this CL, but only for Android?
> > > 
> > > I'm dubious about standardizing a some of these, but changing the
compositor
> > > code has not been fruitful.
> > 
> > Oh, is this actually a compositor bug? Would other browsers behave
differently
> > given the same computed style for all elements?
> 
> I suppose that is a technically correct categorization.
> 
> > If limiting this to Android it would still be a web-observable change, so
that
> > wouldn't really help.
> 
> Unfortunately, it's wrong either way, and the way it is wrong without this
> change is worse. The problem on Android is that once the decoder has chosen to
> output zero-copy frames, they can in fact only be displayed as hardware
> underlays. Therefore any path that disables hardware overlays (any kind of
> post-processed effect) results in the video disappearing. (More precisely, the
> compositor can only punch holes in the root render pass.)
> 
> Since we are forcing this mode for fullscreen H.264 for power reasons, we need
> to be sure that these kinds of effect won't be applied for fullscreen video on
> Android.
> 
> > Is there a specific code path where accelerated overlays kick in where it's
a
> > bit more obvious why this change works? Might it be fixed by
> > https://bugs.chromium.org/p/chromium/issues/detail?id=240576 too?
> 
> Yes, when the new unified media pipeline is enabled (default), one of these
> properties is applied, and the video element is fullscreened, the video
> disappears.
> 
> I do not believe that the top layer change will make a difference, because we
> are already changing the root rendering node to the video layer in this case.

Updated to only affect Android. PTAL.

[sandersd (OOO until July 31), 2016-06-16 18:16:28]

On 2016/06/14 20:24:06, sandersd wrote:
> On 2016/06/13 17:55:36, sandersd wrote:
> > On 2016/06/13 14:00:26, Philip Jägenstedt wrote:
> > > On 2016/06/10 21:23:05, sandersd wrote:
> > > > On 2016/06/10 05:38:48, Philip J. (OOO June 10) wrote:
> > > > > I'm OOO today, but to begin with can you file an issue at
> > > > > https://github.com/whatwg/fullscreen/issues/new to have these added to
> the
> > > > spec?
> > > > 
> > > > Is there a way to get the same effect as this CL, but only for Android?
> > > > 
> > > > I'm dubious about standardizing a some of these, but changing the
> compositor
> > > > code has not been fruitful.
> > > 
> > > Oh, is this actually a compositor bug? Would other browsers behave
> differently
> > > given the same computed style for all elements?
> > 
> > I suppose that is a technically correct categorization.
> > 
> > > If limiting this to Android it would still be a web-observable change, so
> that
> > > wouldn't really help.
> > 
> > Unfortunately, it's wrong either way, and the way it is wrong without this
> > change is worse. The problem on Android is that once the decoder has chosen
to
> > output zero-copy frames, they can in fact only be displayed as hardware
> > underlays. Therefore any path that disables hardware overlays (any kind of
> > post-processed effect) results in the video disappearing. (More precisely,
the
> > compositor can only punch holes in the root render pass.)
> > 
> > Since we are forcing this mode for fullscreen H.264 for power reasons, we
need
> > to be sure that these kinds of effect won't be applied for fullscreen video
on
> > Android.
> > 
> > > Is there a specific code path where accelerated overlays kick in where
it's
> a
> > > bit more obvious why this change works? Might it be fixed by
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=240576 too?
> > 
> > Yes, when the new unified media pipeline is enabled (default), one of these
> > properties is applied, and the video element is fullscreened, the video
> > disappears.
> > 
> > I do not believe that the top layer change will make a difference, because
we
> > are already changing the root rendering node to the video layer in this
case.
> 
> Updated to only affect Android. PTAL.

Ping.

[foolip, 2016-06-21 16:59:37]

third_party/WebKit/ LGTM, but it would be good to also pass this by one of the
css/ and layout/ owners.

https://codereview.chromium.org/2055023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/fullscreenAndroid.css (right):

https://codereview.chromium.org/2055023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/fullscreenAndroid.css:1:
video:-webkit-full-screen, audio:-webkit-full-screen {
Should it really apply to audio elements? They can never paint video frames :)

https://codereview.chromium.org/2055023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/fullscreenAndroid.css:2: border: none
!important;
This one ought to go away with the top layer rewrite at least.

Can you add a link to the bug and a short description of why this is needed?
Also, isn't it needed when playing inline as well, or does it just never work
for that case?

https://codereview.chromium.org/2055023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/fullscreenAndroid.css:5: -webkit-filter: none
!important;
-webkit-filter is an alias for filter, are both needed?

https://codereview.chromium.org/2055023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutThemeMobile.h (right):

https://codereview.chromium.org/2055023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutThemeMobile.h:41: String
extraFullScreenStyleSheet() override;
Suggest Fullscreen with a small s

[sandersd (OOO until July 31), 2016-06-21 18:30:08]

> also pass this by one of the css/ and layout/ owners.

Can you recommend people? (I'm not actually sure what a css/ owner is.)

https://codereview.chromium.org/2055023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/fullscreenAndroid.css (right):

https://codereview.chromium.org/2055023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/fullscreenAndroid.css:1:
video:-webkit-full-screen, audio:-webkit-full-screen {
On 2016/06/21 16:59:36, Philip Jägenstedt wrote:
> Should it really apply to audio elements? They can never paint video frames :)

Done. (I did not change fullscreen.css to match though.)

https://codereview.chromium.org/2055023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/fullscreenAndroid.css:2: border: none
!important;
On 2016/06/21 16:59:36, Philip Jägenstedt wrote:
> This one ought to go away with the top layer rewrite at least.
> 
> Can you add a link to the bug and a short description of why this is needed?
> Also, isn't it needed when playing inline as well, or does it just never work
> for that case?

This is specific to fullscreen on Android, for inline video the compositing path
is correct.

https://codereview.chromium.org/2055023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/fullscreenAndroid.css:5: -webkit-filter: none
!important;
On 2016/06/21 16:59:36, Philip Jägenstedt wrote:
> -webkit-filter is an alias for filter, are both needed?

Then it'll have to be the -webkit-filter variant, since this needs to be
merge-able with M52.

https://codereview.chromium.org/2055023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutThemeMobile.h (right):

https://codereview.chromium.org/2055023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutThemeMobile.h:41: String
extraFullScreenStyleSheet() override;
On 2016/06/21 16:59:36, Philip Jägenstedt wrote:
> Suggest Fullscreen with a small s

To confirm: you would like me to change the name of this method in LayoutTheme
as part of this CL?

[sandersd (OOO until July 31), 2016-06-21 18:34:08]

sandersd@chromium.org changed reviewers:
+ jochen@chromium.org

[sandersd (OOO until July 31), 2016-06-21 18:34:08]

jochen: Please review changes in content/child/blink_platform_impl.cc

[foolip, 2016-06-21 19:12:56]

https://codereview.chromium.org/2055023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutThemeMobile.h (right):

https://codereview.chromium.org/2055023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutThemeMobile.h:41: String
extraFullScreenStyleSheet() override;
On 2016/06/21 18:30:08, sandersd wrote:
> On 2016/06/21 16:59:36, Philip Jägenstedt wrote:
> > Suggest Fullscreen with a small s
> 
> To confirm: you would like me to change the name of this method in LayoutTheme
> as part of this CL?

Yes, for newly added code it would be good to use fullscreen as a single word.
I'll try to rename existing code when I get a chance.

[foolip, 2016-06-21 19:14:12]

On 2016/06/21 18:30:08, sandersd wrote:
> > also pass this by one of the css/ and layout/ owners.
> 
> Can you recommend people? (I'm not actually sure what a css/ owner is.)

Turns out there is no css/OWNERS, oh well :)

I'll ask dsinclair@

[foolip, 2016-06-21 19:16:08]

foolip@chromium.org changed reviewers:
+ dsinclair@chromium.org

[foolip, 2016-06-21 19:16:08]

dsinclair@, you're in layout/OWNERS, can you sanity check this and say if there
might be a way to do the same thing that's not web observable? I'm guessing not,
just want to make sure.

[sandersd (OOO until July 31), 2016-06-21 19:34:20]

https://codereview.chromium.org/2055023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutThemeMobile.h (right):

https://codereview.chromium.org/2055023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutThemeMobile.h:41: String
extraFullScreenStyleSheet() override;
On 2016/06/21 19:12:55, Philip Jägenstedt wrote:
> On 2016/06/21 18:30:08, sandersd wrote:
> > On 2016/06/21 16:59:36, Philip Jägenstedt wrote:
> > > Suggest Fullscreen with a small s
> > 
> > To confirm: you would like me to change the name of this method in
LayoutTheme
> > as part of this CL?
> 
> Yes, for newly added code it would be good to use fullscreen as a single word.
> I'll try to rename existing code when I get a chance.

Done.

[dsinclair, 2016-06-21 19:46:38]

dsinclair@chromium.org changed reviewers:
+ eae@chromium.org

[dsinclair, 2016-06-21 19:46:39]

Passing over to eae@ to take a look at the layout/ stuff.

[dsinclair, 2016-06-21 19:46:53]

dsinclair@chromium.org changed reviewers:
- dsinclair@chromium.org

[jochen (gone - plz use gerrit), 2016-06-22 13:22:19]

content/ lgtm

[eae, 2016-06-22 13:28:23]

LGTM for layout/

[foolip, 2016-06-22 14:55:24]

Hmm, any chance of a test so I don't break this? :)

[sandersd (OOO until July 31), 2016-06-23 21:37:37]

On 2016/06/22 14:55:24, Philip Jägenstedt wrote:
> Hmm, any chance of a test so I don't break this? :)

I tried to create a LayoutThemeTest for this, but I couldn't get it to apply the
fullscreen styles. I don't really want to write a (virtual) LayoutTest for this,
so I'll say no for now.

That said, we *should* have a test for fullscreen.css in general, and this could
be a part of it. If you have a suggestion for correctly simulating fullscreen,
I'll go ahead and do that in a followup.


For reference, I tried combinations of these things (in LayoutThemeTest.cpp):

   {
       UserGestureIndicator gesture(DefinitelyProcessingUserGesture);
       video->enterFullscreen();
       document().view()->updateAllLifecyclePhases();
   }
   EXPECT_FALSE(video->computedStyle()->hasOpacity());

[sandersd (OOO until July 31), 2016-06-23 21:38:29]

The CQ bit was checked by sandersd@chromium.org

[sandersd (OOO until July 31), 2016-06-23 21:38:30]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2055023002/#ps80001
(title: "FullScreen -> Fullscreen.")

[commit-bot: I haz the power, 2016-06-23 21:38:53]

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

[commit-bot: I haz the power, 2016-06-24 00:16:40]

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

[commit-bot: I haz the power, 2016-06-24 00:16:42]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sandersd (OOO until July 31), 2016-06-24 00:19:36]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2016-06-24 00:20:38]

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

[commit-bot: I haz the power, 2016-06-24 01:51:37]

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

[commit-bot: I haz the power, 2016-06-24 01:51:39]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sandersd (OOO until July 31), 2016-06-24 17:50:15]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2016-06-24 17:50:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-24 19:17:26]

Description was changed from

==========
Exclude more CSS properties for fullscreen video on Android.

This CL overrides additional CSS properties that prevent accelerated overlays
from being used:
  - -webkit-filter
  - opacity
  - border-radius
  - mix-blend-mode

And two that are rendered incorrectly:
  - background-image
  - border

This is especially important because there is no non-overlay fallback.

This mechanism is expected to be replaced by compositor changes in M53.

BUG=618753
==========

to

==========
Exclude more CSS properties for fullscreen video on Android.

This CL overrides additional CSS properties that prevent accelerated overlays
from being used:
  - -webkit-filter
  - opacity
  - border-radius
  - mix-blend-mode

And two that are rendered incorrectly:
  - background-image
  - border

This is especially important because there is no non-overlay fallback.

This mechanism is expected to be replaced by compositor changes in M53.

BUG=618753
==========

[commit-bot: I haz the power, 2016-06-24 19:17:27]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-24 19:21:24]

Description was changed from

==========
Exclude more CSS properties for fullscreen video on Android.

This CL overrides additional CSS properties that prevent accelerated overlays
from being used:
  - -webkit-filter
  - opacity
  - border-radius
  - mix-blend-mode

And two that are rendered incorrectly:
  - background-image
  - border

This is especially important because there is no non-overlay fallback.

This mechanism is expected to be replaced by compositor changes in M53.

BUG=618753
==========

to

==========
Exclude more CSS properties for fullscreen video on Android.

This CL overrides additional CSS properties that prevent accelerated overlays
from being used:
  - -webkit-filter
  - opacity
  - border-radius
  - mix-blend-mode

And two that are rendered incorrectly:
  - background-image
  - border

This is especially important because there is no non-overlay fallback.

This mechanism is expected to be replaced by compositor changes in M53.

BUG=618753

Committed: https://crrev.com/f33109c4d3bdafe0a0c07c59bd1bf1639595dc8a
Cr-Commit-Position: refs/heads/master@{#401923}
==========

[commit-bot: I haz the power, 2016-06-24 19:21:25]

Patchset 5 (id:??) landed as
https://crrev.com/f33109c4d3bdafe0a0c07c59bd1bf1639595dc8a
Cr-Commit-Position: refs/heads/master@{#401923}