aw: Skip hardware onDraw when visible rect is empty

aw: Skip hardware onDraw when visible rect is empty

Framework does not clamp onDraw to only the visible area. However must
still request functor in this case to support rt-side animation, and to
obtain the correct draw matrix. This means HardwareRenderer must be able
to handle not having a parent frame available.

BUG=412623
Committed: https://crrev.com/693c6a04c69d69a1240e4b83e2ec7fea459acda4
Cr-Commit-Position: refs/heads/master@{#294093}

[boliu, 2014-09-08 15:38:39]

boliu@chromium.org changed reviewers:
+ hush@chromium.org

[boliu, 2014-09-08 15:38:39]

does this work?

[hush (inactive), 2014-09-08 20:55:42]

On 2014/09/08 15:38:39, boliu wrote:
> does this work?

yeah. It works for Vice app to show the article content. Well, it basically
ignores the hardware onDraws that has an empty global visible rect. What are the
implications? Suppose we're doing a swiping animation and the webview with an
empty global visible rect is about to appear. During this draw, no tiles are
allocated for this webview. So we will be more likely to see missing tiles
during the animation. If you convert the empty visible rect into a 1x1 rect like
in https://codereview.chromium.org/543313002/, you will kick off tile allocation
for this webview right in this frame, at least some low res tiles I guess?

[boliu, 2014-09-08 21:16:18]

On 2014/09/08 20:55:42, hush wrote:
> On 2014/09/08 15:38:39, boliu wrote:
> > does this work?
> 
> yeah. It works for Vice app to show the article content. Well, it basically
> ignores the hardware onDraws that has an empty global visible rect. What are
the
> implications? Suppose we're doing a swiping animation and the webview with an
> empty global visible rect is about to appear. During this draw, no tiles are
> allocated for this webview. So we will be more likely to see missing tiles
> during the animation.

Yes it's a trade off. And this CL is definitely taking smoothness over
correctness. See my big rant about this in the internal bug.

I think smoothness always trumps tile correctness.

> If you convert the empty visible rect into a 1x1 rect like
> in https://codereview.chromium.org/543313002/, you will kick off tile
allocation
> for this webview right in this frame, at least some low res tiles I guess?

That change is clearly wrong. It provides no guidance to the compositor on what
part of the layer tree will be shown as 0,0 1x1 is just plain wrong, which can
cause tiles to be thrown away because of the tile rect jumping around etc.

The correct thing there would be transform the screen rect to surface space, and
use that. (That's what the compositor does too, except it's using the wrong
rect)

[hush (inactive), 2014-09-08 21:58:16]

> The correct thing there would be transform the screen rect to surface space,
and
> use that. (That's what the compositor does too, except it's using the wrong
> rect)

Why don't you use one of these methods to get the screen size when global
visible rect is empty? 
https://code.google.com/p/chromium/codesearch#chromium/src/ui/android/java/sr...

[boliu, 2014-09-08 22:01:37]

On 2014/09/08 21:58:16, hush wrote:
> > The correct thing there would be transform the screen rect to surface space,
> and
> > use that. (That's what the compositor does too, except it's using the wrong
> > rect)
> 
> Why don't you use one of these methods to get the screen size when global
> visible rect is empty? 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/android/java/sr...

You can, and then take the matrix from functor and I think apply the inverse I
think.

My point was I perfer skipping onDraw (ie prefer smoothness) in this case.

[hush (inactive), 2014-09-08 22:45:40]

Okay... If it is a general consistent policy that we prefer smoothness to
correctness, (I believe so)
then let's just be consistent with it and stick with what you did here.

Anyways, this CL will stop the offscreen and large webviews from having an
onDraw loop. And it mysteriously
fixes the problem for Vice app's on-screen webview not showing article. I am
investigating that.


On 2014/09/08 21:16:18, boliu wrote:
> On 2014/09/08 20:55:42, hush wrote:
> > On 2014/09/08 15:38:39, boliu wrote:
> > > does this work?
> > 
> > yeah. It works for Vice app to show the article content. Well, it basically
> > ignores the hardware onDraws that has an empty global visible rect. What are
> the
> > implications? Suppose we're doing a swiping animation and the webview with
an
> > empty global visible rect is about to appear. During this draw, no tiles are
> > allocated for this webview. So we will be more likely to see missing tiles
> > during the animation.
> 
> Yes it's a trade off. And this CL is definitely taking smoothness over
> correctness. See my big rant about this in the internal bug.
> 
> I think smoothness always trumps tile correctness.
> 
> > If you convert the empty visible rect into a 1x1 rect like
> > in https://codereview.chromium.org/543313002/, you will kick off tile
> allocation
> > for this webview right in this frame, at least some low res tiles I guess?
> 
> That change is clearly wrong. It provides no guidance to the compositor on
what
> part of the layer tree will be shown as 0,0 1x1 is just plain wrong, which can
> cause tiles to be thrown away because of the tile rect jumping around etc.
> 
> The correct thing there would be transform the screen rect to surface space,
and
> use that. (That's what the compositor does too, except it's using the wrong
> rect)

[hush (inactive), 2014-09-09 00:06:02]

https://codereview.chromium.org/551023002/diff/1/android_webview/browser/brow...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/551023002/diff/1/android_webview/browser/brow...
android_webview/browser/browser_view_renderer.cc:101: kTileMultiplier *= 2;
why do we do this? By the way, if you want to double this, kTileMultiplier is
not a constant any more, so you need to rename the variable.

[boliu, 2014-09-09 00:07:20]

https://codereview.chromium.org/551023002/diff/1/android_webview/browser/brow...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/551023002/diff/1/android_webview/browser/brow...
android_webview/browser/browser_view_renderer.cc:101: kTileMultiplier *= 2;
On 2014/09/09 00:06:02, hush wrote:
> why do we do this? By the way, if you want to double this, kTileMultiplier is
> not a constant any more, so you need to rename the variable.

Ahh crap, unrelated change that I didn't mean to upload.

[hush (inactive), 2014-09-09 00:09:45]

https://codereview.chromium.org/551023002/diff/1/android_webview/browser/brow...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/551023002/diff/1/android_webview/browser/brow...
android_webview/browser/browser_view_renderer.cc:101: kTileMultiplier *= 2;
cool. Get rid of this and other parts lgtm.
You need a bug number for this to merge into m38.
On 2014/09/09 00:07:19, boliu wrote:
> On 2014/09/09 00:06:02, hush wrote:
> > why do we do this? By the way, if you want to double this, kTileMultiplier
is
> > not a constant any more, so you need to rename the variable.
> 
> Ahh crap, unrelated change that I didn't mean to upload.

[boliu, 2014-09-09 00:10:31]

I'm still waiting for why this fixes anything first before submitting..

[boliu, 2014-09-09 00:11:08]

On 2014/09/09 00:10:31, boliu wrote:
> I'm still waiting for why this fixes anything first before submitting..

Actually can you handle filing the bug and submitting and merging and whatnot?
After you figured out why the fix works.

[hush (inactive), 2014-09-10 02:18:24]

The CQ bit was checked by hush@chromium.org

[commit-bot: I haz the power, 2014-09-10 02:21:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/boliu@chromium.org/551023002/20001

[hush (inactive), 2014-09-10 03:27:15]

updated internal bug (b/17403173) about what's happening and why this fix works.
Lets' check this in.

[commit-bot: I haz the power, 2014-09-10 03:31:41]

Committed patchset #2 (id:20001) as ab8ab71b035d4325912d30cbb34fc867961181f2

[commit-bot: I haz the power, 2014-09-10 03:58:50]

Patchset 2 (id:??) landed as
https://crrev.com/693c6a04c69d69a1240e4b83e2ec7fea459acda4
Cr-Commit-Position: refs/heads/master@{#294093}