Widen GPU trigger condition

Widen GPU trigger condition

GPU rasterization will be triggered when viewports meet either of the 
following conditions:
1) width=device-width, minimum-scale=X, where X >= 1.0
2) width=device-width, user-scalable=no

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

[junchao.han, 2014-10-17 07:04:58]

junchao.han@intel.com changed reviewers:
+ ajuma@chromium.org, ernstm@google.com

[ernstm, 2014-10-17 17:11:49]

On 2014/10/17 07:04:58, junchao.han wrote:
> mailto:junchao.han@intel.com changed reviewers:
> + mailto:ajuma@chromium.org, mailto:ernstm@google.com

We should not change the trigger just to include a particular page. The
triggering logic is complicated already, I don't want to make it even more
complicated. The right solution is to make GPU rasterization work well with all
content and get rid of the trigger eventually. The current blocker for this is
image decoding. We are working on a solution for that, but it will take some
time.

[junchao.han, 2014-10-18 09:08:05]

On 2014/10/17 17:11:49, ernstm wrote:
> On 2014/10/17 07:04:58, junchao.han wrote:
> > mailto:junchao.han@intel.com changed reviewers:
> > + mailto:ajuma@chromium.org, mailto:ernstm@google.com
> 
> We should not change the trigger just to include a particular page. The
> triggering logic is complicated already, I don't want to make it even more
> complicated. The right solution is to make GPU rasterization work well with
all
> content and get rid of the trigger eventually. The current blocker for this is
> image decoding. We are working on a solution for that, but it will take some
> time.

The change is suitable for a set of pages. Antutu SVG benchmark is one typical 
case which shows great performance boost with GPU rasterization turned on. 
This patch does not change the key triggering logic but choose a more suitable 
indicator in implementation. All page with GPU rasterization will be fantastic, 
but may take some time. We can treat this modification as intermediate step. 
Although Chrome on Android is widely used, mobile web developer may not fully 
aware to add “width=device-width,minimum-scale=1.0” for better performance. It 
is a pity if web pages miss the smooth rendering.

[Jin Yang, 2014-10-21 03:06:07]

On 2014/10/18 09:08:05, junchao.han wrote:
> On 2014/10/17 17:11:49, ernstm wrote:
> > On 2014/10/17 07:04:58, junchao.han wrote:
> > > mailto:junchao.han@intel.com changed reviewers:
> > > + mailto:ajuma@chromium.org, mailto:ernstm@google.com
> > 
> > We should not change the trigger just to include a particular page. The
> > triggering logic is complicated already, I don't want to make it even more
> > complicated. The right solution is to make GPU rasterization work well with
> all
> > content and get rid of the trigger eventually. The current blocker for this
is
> > image decoding. We are working on a solution for that, but it will take some
> > time.
> 
> The change is suitable for a set of pages. Antutu SVG benchmark is one typical

> case which shows great performance boost with GPU rasterization turned on. 
> This patch does not change the key triggering logic but choose a more suitable

> indicator in implementation. All page with GPU rasterization will be
fantastic, 
> but may take some time. We can treat this modification as intermediate step. 
> Although Chrome on Android is widely used, mobile web developer may not fully 
> aware to add “width=device-width,minimum-scale=1.0” for better performance. It

> is a pity if web pages miss the smooth rendering.

Any comments? The patch is not for particular page, the added trigger condition
is meaningful for the pages which are not user-scalable and the actual scale is
no less than 1.0 before we fully enable GPU rasterization. As you pointed, when
the image decoding is ready, we can remove all the conditions.

[ernstm, 2014-10-21 17:10:50]

> Any comments? The patch is not for particular page, the added trigger
condition
> is meaningful for the pages which are not user-scalable and the actual scale
is
> no less than 1.0 before we fully enable GPU rasterization. As you pointed,
when
> the image decoding is ready, we can remove all the conditions.

I think widening the content white-list is a good idea in general, as long as we
don't hit desktop content before the image decoding issue is fixed. But the
triggering conditions must be simple. I see a lot of people being confused by
the existing triggering and vetoing mechanisms already. The current patch is
adding too much complexity. Ali might have some ideas how to rewrite it such
that the triggering mechanism becomes simpler.

[ajuma, 2014-10-21 17:40:03]

https://codereview.chromium.org/660133005/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/660133005/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:3285: 
I understand the motivation of using the resolved values, but I think it makes
things harder to explain to web developers. I'd prefer to do something similar
using |description| directly (as before). For example, how about:
 m_matchesHeuristicsForGpuRasterization = description.maxWidth ==
Length(DeviceWidth)	
         && ((description.minZoom >= 1.0 && description.minZoomIsExplicit)
             || (description.userZoom == false && description.zoom >= 1.0 &&
description.zoomIsExplicit));

This allows viewports that meet either of the following conditions:
1) width=device-width, minimum-scale=X, where X >= 1.0
2) width=device-width, user-scalable=no, initial-scale=X, where X >= 1.0

[junchao.han, 2014-10-22 07:31:33]

On 2014/10/21 17:40:03, ajuma wrote:
> https://codereview.chromium.org/660133005/diff/1/Source/web/WebViewImpl.cpp
> File Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/660133005/diff/1/Source/web/WebViewImpl.cpp#n...
> Source/web/WebViewImpl.cpp:3285: 
> I understand the motivation of using the resolved values, but I think it makes
> things harder to explain to web developers. I'd prefer to do something similar
> using |description| directly (as before). For example, how about:
>  m_matchesHeuristicsForGpuRasterization = description.maxWidth ==
> Length(DeviceWidth)	
>          && ((description.minZoom >= 1.0 && description.minZoomIsExplicit)
>              || (description.userZoom == false && description.zoom >= 1.0 &&
> description.zoomIsExplicit));
> 
> This allows viewports that meet either of the following conditions:
> 1) width=device-width, minimum-scale=X, where X >= 1.0
> 2) width=device-width, user-scalable=no, initial-scale=X, where X >= 1.0

Your suggestion is great, which widen white-list and makes mechanism simpler. 
We will update the patch based on your feedback.

BTW, maybe we can further simplify the second condition clause to
m_matchesHeuristicsForGpuRasterization = description.maxWidth ==
Length(DeviceWidth)
         && ((description.minZoom >= 1.0 && description.minZoomIsExplicit)
             || (description.userZoom == false));
Because intuitively, to display contents clearly, fixed scale web pages won’t 
have much visual complexity. What do you think?

[ajuma, 2014-10-22 14:13:51]

On 2014/10/22 07:31:33, junchao.han wrote:
> On 2014/10/21 17:40:03, ajuma wrote:
> > https://codereview.chromium.org/660133005/diff/1/Source/web/WebViewImpl.cpp
> > File Source/web/WebViewImpl.cpp (right):
> > 
> >
>
https://codereview.chromium.org/660133005/diff/1/Source/web/WebViewImpl.cpp#n...
> > Source/web/WebViewImpl.cpp:3285: 
> > I understand the motivation of using the resolved values, but I think it
makes
> > things harder to explain to web developers. I'd prefer to do something
similar
> > using |description| directly (as before). For example, how about:
> >  m_matchesHeuristicsForGpuRasterization = description.maxWidth ==
> > Length(DeviceWidth)	
> >          && ((description.minZoom >= 1.0 && description.minZoomIsExplicit)
> >              || (description.userZoom == false && description.zoom >= 1.0 &&
> > description.zoomIsExplicit));
> > 
> > This allows viewports that meet either of the following conditions:
> > 1) width=device-width, minimum-scale=X, where X >= 1.0
> > 2) width=device-width, user-scalable=no, initial-scale=X, where X >= 1.0
> 
> Your suggestion is great, which widen white-list and makes mechanism simpler. 
> We will update the patch based on your feedback.
> 
> BTW, maybe we can further simplify the second condition clause to
> m_matchesHeuristicsForGpuRasterization = description.maxWidth ==
> Length(DeviceWidth)
>          && ((description.minZoom >= 1.0 && description.minZoomIsExplicit)
>              || (description.userZoom == false));
> Because intuitively, to display contents clearly, fixed scale web pages won’t 
> have much visual complexity. What do you think?

Good idea, that intuition seems reasonable to me.

[junchao.han, 2014-10-23 01:37:32]

On 2014/10/22 14:13:51, ajuma wrote:
> On 2014/10/22 07:31:33, junchao.han wrote:
> > On 2014/10/21 17:40:03, ajuma wrote:
> > >
https://codereview.chromium.org/660133005/diff/1/Source/web/WebViewImpl.cpp
> > > File Source/web/WebViewImpl.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/660133005/diff/1/Source/web/WebViewImpl.cpp#n...
> > > Source/web/WebViewImpl.cpp:3285: 
> > > I understand the motivation of using the resolved values, but I think it
> makes
> > > things harder to explain to web developers. I'd prefer to do something
> similar
> > > using |description| directly (as before). For example, how about:
> > >  m_matchesHeuristicsForGpuRasterization = description.maxWidth ==
> > > Length(DeviceWidth)	
> > >          && ((description.minZoom >= 1.0 && description.minZoomIsExplicit)
> > >              || (description.userZoom == false && description.zoom >= 1.0
&&
> > > description.zoomIsExplicit));
> > > 
> > > This allows viewports that meet either of the following conditions:
> > > 1) width=device-width, minimum-scale=X, where X >= 1.0
> > > 2) width=device-width, user-scalable=no, initial-scale=X, where X >= 1.0
> > 
> > Your suggestion is great, which widen white-list and makes mechanism
simpler. 
> > We will update the patch based on your feedback.
> > 
> > BTW, maybe we can further simplify the second condition clause to
> > m_matchesHeuristicsForGpuRasterization = description.maxWidth ==
> > Length(DeviceWidth)
> >          && ((description.minZoom >= 1.0 && description.minZoomIsExplicit)
> >              || (description.userZoom == false));
> > Because intuitively, to display contents clearly, fixed scale web pages
won’t 
> > have much visual complexity. What do you think?
> 
> Good idea, that intuition seems reasonable to me.

We have uploaded a new patch set based on your feedback. Please review.

[ajuma, 2014-10-23 14:29:53]

On 2014/10/23 01:37:32, junchao.han wrote:
> We have uploaded a new patch set based on your feedback. Please review.

Thanks! Please also update the viewportTriggersGpuRasterization test (in
Source/web/tests). I'd suggest at least 3 more cases:
-verify that width=device-width and minimum-scale > 1.0 triggers GPU
rasterization
-verify that width=device-width and minimum-scale < 1.0 does not trigger GPU
rasterization
-verify that width=device-width and user-scalable=no triggers GPU rasterization

The files referred to by that test are found in Source/web/tests/data/viewport.

To run the test, build target webkit_unit_tests, and then run webkit_unit_tests
with --gtest_filter=ViewportTest.viewportTriggersGpuRasterization

Also, before this can land, you'll need to be added as one of the contributors
listed in Intel's CLA (see
http://www.chromium.org/developers/contributing-code/external-contributor-che...),
and then you'll need to add yourself to the AUTHORS file.

[junchao.han, 2014-10-24 03:28:09]

On 2014/10/23 14:29:53, ajuma wrote:
> On 2014/10/23 01:37:32, junchao.han wrote:
> > We have uploaded a new patch set based on your feedback. Please review.
> 
> Thanks! Please also update the viewportTriggersGpuRasterization test (in
> Source/web/tests). I'd suggest at least 3 more cases:
> -verify that width=device-width and minimum-scale > 1.0 triggers GPU
> rasterization
> -verify that width=device-width and minimum-scale < 1.0 does not trigger GPU
> rasterization
> -verify that width=device-width and user-scalable=no triggers GPU
rasterization
> 
> The files referred to by that test are found in
Source/web/tests/data/viewport.
> 
> To run the test, build target webkit_unit_tests, and then run
webkit_unit_tests
> with --gtest_filter=ViewportTest.viewportTriggersGpuRasterization
> 
> Also, before this can land, you'll need to be added as one of the contributors
> listed in Intel's CLA (see
>
http://www.chromium.org/developers/contributing-code/external-contributor-che...),
> and then you'll need to add yourself to the AUTHORS file.

3 new test cases are added and ViewportTest.viewportTriggersGpuRasterization 
unit test is passed. Please review the change.

I upload another chromium patch to add myself to AUTHOR file. Need your help to 
land the patch. Thanks!
https://codereview.chromium.org/674053002/

[ajuma, 2014-10-24 13:32:07]

On 2014/10/24 03:28:09, junchao.han wrote:
>
> I upload another chromium patch to add myself to AUTHOR file. Need your help
to 
> land the patch. Thanks!
> https://codereview.chromium.org/674053002/

Thanks! The patch looks good to me. I've triggered some try jobs for you.

Please make sure you get added to Intel's list of authorized contributors (I
just checked and you're not listed yet). Once that's done, I can lg2m both
patches. (This patch will also need a review from a Source/web OWNER.)

[junchao.han, 2014-10-28 01:31:28]

junchao.han@intel.com changed reviewers:
+ aelias@chromium.org

[junchao.han, 2014-10-28 01:36:32]

On 2014/10/24 13:32:07, ajuma wrote:
> On 2014/10/24 03:28:09, junchao.han wrote:
> >
> > I upload another chromium patch to add myself to AUTHOR file. Need your help
> to 
> > land the patch. Thanks!
> > https://codereview.chromium.org/674053002/
> 
> Thanks! The patch looks good to me. I've triggered some try jobs for you.
> 
> Please make sure you get added to Intel's list of authorized contributors (I
> just checked and you're not listed yet). Once that's done, I can lg2m both
> patches. (This patch will also need a review from a Source/web OWNER.)

Source/Web owner aelias is invited to review the patch and I have contacted 
with account manager to add my name to CLA list. 
Please continue the process. Thanks!

[aelias_OOO_until_Jul13, 2014-10-28 04:41:21]

Source/web lgtm

[junchao.han, 2014-10-29 05:38:10]

The CQ bit was checked by junchao.han@intel.com

[junchao.han, 2014-10-29 05:38:29]

The CQ bit was unchecked by junchao.han@intel.com

[junchao.han, 2014-10-29 05:38:44]

The CQ bit was checked by junchao.han@intel.com

[commit-bot: I haz the power, 2014-10-29 05:39:12]

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

[commit-bot: I haz the power, 2014-10-29 06:16:57]

Committed patchset #3 (id:40001) as 184566

[ajuma, 2014-11-05 01:04:12]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/685003008/ by ajuma@chromium.org.

The reason for reverting is: There's content that depends on the existing
trigger in order to opt-out of GPU rasterization. Since we're close to the
branch point, let's revert this and then try to expand again in M41..