Fix UpdateTilePriorities viewport in Android WebView.

Fix UpdateTilePriorities viewport in Android WebView.

This patch fixes the tile management viewport to be the on-screen-visible
rect for the last hardware draw.  There are two problems I needed to solve:

1) Suppress UpdateTilePriorities from happening when the last draw was a
software draw, which is specified by the new
"viewport_valid_for_tile_management" bool.

2) In some cases, the clip rect is smaller than the WebView's visible
viewport -- for example when the Android Browser progress bar is forcing
redraws, the clip is only a small area at the top of the screen.  I
switched to using the visible viewport for UpdateDrawProperties +
glViewport, and introduce a separate "DeviceClip" to override glScissor.
(It's necessary to make the change at this level, rather than directly
plumbing the visible viewport to UpdateTilePriorities, because
UpdateTilePriorities also makes use of outputs from the draw properties
calculation.)

I also cleaned up all uses of device_viewport_size() to call DrawViewportSize()
instead, with the sole exception of UnscaledScrollableViewportSize() which
should continue to use the main-thread device_viewport_size_ in all cases
(because that's the viewport for scrolling, not drawing).

New tests: PictureLayerImplTest.SuppressUpdateTilePriorities,
           ExternalStencilPixelTest.DeviceClip

NOTRY=true
BUG=232844
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221321

[aelias_OOO_until_Jul13, 2013-09-03 08:57:23]

PTAL.  This should be the last big externally-managed-viewport related change, I
think.  This fixes a swath of tile-flickering bugs WebView had due to the tile
manager evicting tiles based on the wrong viewport.

[tomhudson, 2013-09-03 10:43:27]

The comment on RendererClient::DeviceViewport() seems to be completely wrong?
(Or it may just be the use of the term 'Draw viewport', where it isn't clear
whether draw is a verb or an adjective, in which case 'Returns the draw
viewport...' would suffice.)

Meanwhile, if DeviceViewport() and device_viewport_size() have radically
different semantics, can we think about renaming? Or being explicit about how
they interact with the threading model in header comments?

[enne (OOO), 2013-09-03 16:58:47]

https://codereview.chromium.org/23171014/diff/13001/cc/layers/picture_layer_i...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/23171014/diff/13001/cc/layers/picture_layer_i...
cc/layers/picture_layer_impl.cc:296: if
(!layer_tree_impl()->device_viewport_valid_for_tile_management())
If you're going to early out here, can you also DCHECK that each tiling
has_ever_been_updated?

I think you also need to handle this case in CalculateContentsScales, possibly
by skipping the ManageTilings call.  If the scale from the external transform is
different than the normal one, then layers will create tilings needlessly.

https://codereview.chromium.org/23171014/diff/13001/cc/layers/picture_layer_i...
cc/layers/picture_layer_impl.cc:324: gfx::Size device_viewport_size =
layer_tree_impl()->DeviceViewport().size();
Can you either use DeviceViewport() directly without dropping its location or
DCHECK that the location is the origin?

In general, it seems awkward to pass a full viewport rect, but then only use its
size in some places.  Having a full transform and a viewport rect seems like
there's redundant information.  Could we keep the assumption that the viewport
is always at the origin and just bake any viewport offsets into the external
transform?  Isn't that what AdjustTransform is already doing?

https://codereview.chromium.org/23171014/diff/13001/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/23171014/diff/13001/cc/output/direct_renderer...
cc/output/direct_renderer.cc:270: gfx::Rect DirectRenderer::DeviceClipRect(const
DrawingFrame* frame) const {
Could you help me understand a little bit better why you need this additional
device clip?  Why should this only apply to drawing but not
UpdateTilePrioritieS?

[aelias_OOO_until_Jul13, 2013-09-03 19:52:09]

https://codereview.chromium.org/23171014/diff/13001/cc/layers/picture_layer_i...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/23171014/diff/13001/cc/layers/picture_layer_i...
cc/layers/picture_layer_impl.cc:296: if
(!layer_tree_impl()->device_viewport_valid_for_tile_management())
On 2013/09/03 16:58:47, enne wrote:
> If you're going to early out here, can you also DCHECK that each tiling
> has_ever_been_updated?
> 
> I think you also need to handle this case in CalculateContentsScales, possibly
> by skipping the ManageTilings call.  If the scale from the external transform
is
> different than the normal one, then layers will create tilings needlessly.

Done.

https://codereview.chromium.org/23171014/diff/13001/cc/layers/picture_layer_i...
cc/layers/picture_layer_impl.cc:324: gfx::Size device_viewport_size =
layer_tree_impl()->DeviceViewport().size();
On 2013/09/03 16:58:47, enne wrote:
> Can you either use DeviceViewport() directly without dropping its location or
> DCHECK that the location is the origin?
> 
> In general, it seems awkward to pass a full viewport rect, but then only use
its
> size in some places.  Having a full transform and a viewport rect seems like
> there's redundant information.  Could we keep the assumption that the viewport
> is always at the origin and just bake any viewport offsets into the external
> transform?  Isn't that what AdjustTransform is already doing?

This is a pretty subtle question but I've convinced myself the current
implementation is appropriate and we cannot drop the offset from the viewport
rect.

A key point here is that there are two confusingly similar spaces which I've
been calling in the code "draw space" and "window space".  In both spaces a
single 1x1 unit corresponds to a pixel, but draw space is always rooted at the
origin, whereas window space is offsetted and flipped.  Layers and quads are
targeted to draw space, then transformed into the unit square by the projection
matrix, then transformed into window space by the glViewport (in GLRenderer) or
equivalently the window matrix (in SoftwareRenderer).  When I use
"DeviceViewport().size()" it's because that's a draw-space thing, when I use the
full DeviceViewport() rect it's for a window-space thing.

Furthermore, the transform is in draw space.  The transform + viewport size
tells us which content pixels are visible.  If we baked the window-space offset
of the viewport into the transform, we would end up with a strange idea of what
pixels are onscreen (very likely extending outside the document bounds).  So we
split off the origin of the visibility rect on the physical screen into a
separate value applied later.

(One possible path to address some of your concerns would be to redefine "draw
space" to not necessarily be rooted at the origin.  If we did that, then we
wouldn't need to extract DeviceViewport().size() and we wouldn't need to adjust
the transform.  I tried to do this a month ago  but the assumption of
rooted-at-origin is deeply embedded in CC.  It didn't seem a good idea to change
a lot of code to use a rect instead of a size, introducing a lot of
opportunities for bugs if somebody forgets to consider the rect offset since
it's usually zero, when I can instead apply the offset in a superficial and
reliable way in DirectRenderer.)

https://codereview.chromium.org/23171014/diff/13001/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/23171014/diff/13001/cc/output/direct_renderer...
cc/output/direct_renderer.cc:270: gfx::Rect DirectRenderer::DeviceClipRect(const
DrawingFrame* frame) const {
On 2013/09/03 16:58:47, enne wrote:
> Could you help me understand a little bit better why you need this additional
> device clip?  Why should this only apply to drawing but not
> UpdateTilePrioritieS?

Sometimes a part of another Android View overlaps slightly with the WebView and
causes a small invalidation.  When this happens, the Android View system
requests a draw with a small clip rect surrounding that invalidation.  So, if we
use the clip rect as the visible viewport to be used for tile management, it
will work most of the time but when this happens CC will think the viewport
suddenly is tiny and evict tiles.

As of this patch, we use instead the "global visible rect" of our WebView as the
viewport (which is guaranteed to be a superset of the clip).  Conversely we
cannot use only *that* rect for both viewport and clip or we'll illegally draw
over other views sometimes.  So making CC aware of two different rectangles
becomes necessary.

https://codereview.chromium.org/23171014/diff/13001/cc/output/renderer.h
File cc/output/renderer.h (right):

https://codereview.chromium.org/23171014/diff/13001/cc/output/renderer.h#newc...
cc/output/renderer.h:21: // Draw viewport and clip in non-y-flipped window
space. Note that while a
Switched this to say "These return the draw viewport..."

https://codereview.chromium.org/23171014/diff/13001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/23171014/diff/13001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.h:545: gfx::Size device_viewport_size_;
Added a comment explaining the role of this better.  I'm thinking of renaming it
in a future patch to just "viewport_size_".

[enne (OOO), 2013-09-03 21:47:31]

https://codereview.chromium.org/23171014/diff/13001/cc/layers/picture_layer_i...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/23171014/diff/13001/cc/layers/picture_layer_i...
cc/layers/picture_layer_impl.cc:324: gfx::Size device_viewport_size =
layer_tree_impl()->DeviceViewport().size();
On 2013/09/03 19:52:09, aelias wrote:

> This is a pretty subtle question but I've convinced myself the current
> implementation is appropriate and we cannot drop the offset from the viewport
> rect.

Aha!  Thanks, I really appreciate the explanation of the distinction here.

I do agree with you that redefining draw space to not be at the origin might
confuse somebody, but at the same time having these two different spaces come
from the same variable (more or less) seems also prone to error.

Is there some way to name or differentiate these better? I mean, maybe it's as
simple as having DeviceViewport only used by DirectRenderer and
DeviceViewportSize used elsewhere.

I think I could get behind a policy where DeviceViewport was only used by
DirectRenderer and device_viewport_size as before is used everywhere else, with
a comment in LTHI to document these different spaces.

Maybe they could also be called DeviceDrawViewport(Size?) and
DeviceWindowViewport? I realize that's getting kind of wordy, and maybe that's
overkill.

https://codereview.chromium.org/23171014/diff/13001/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/23171014/diff/13001/cc/output/direct_renderer...
cc/output/direct_renderer.cc:270: gfx::Rect DirectRenderer::DeviceClipRect(const
DrawingFrame* frame) const {
On 2013/09/03 19:52:09, aelias wrote:

> So making CC aware of two different rectangles
> becomes necessary.

Thanks for all the explanation.  This clip rect then is really behaving as
damage caused by the embedder that needs a redraw.  

Although, if there is a composited animation going on (or scrolling), only
redrawing part of the viewport sounds like it would show content from different
frames.  I guess this is the Android view system asking for this draw though,
and it's probably too late to merge the compositor's damage rect with that
"clip" rect?

[boliu, 2013-09-03 21:54:04]

https://codereview.chromium.org/23171014/diff/13001/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/23171014/diff/13001/cc/output/direct_renderer...
cc/output/direct_renderer.cc:270: gfx::Rect DirectRenderer::DeviceClipRect(const
DrawingFrame* frame) const {
On 2013/09/03 21:47:31, enne wrote:
> On 2013/09/03 19:52:09, aelias wrote:
> 
> > So making CC aware of two different rectangles
> > becomes necessary.
> 
> Thanks for all the explanation.  This clip rect then is really behaving as
> damage caused by the embedder that needs a redraw.  
> 
> Although, if there is a composited animation going on (or scrolling), only
> redrawing part of the viewport sounds like it would show content from
different
> frames.

Absolutely. Eventually webview would like to avoid updating scroll offset inside
draw: crbug.com/249972

For now we have an hack of detecting this situation and invalidating the whole
view again, so the next draw will be the whole visible rect.

> I guess this is the Android view system asking for this draw though,
> and it's probably too late to merge the compositor's damage rect with that
> "clip" rect?

Right. Contract with android view system is never draw outside of the clip, lest
we clobber some other view on top of us that's not damaged.

[aelias_OOO_until_Jul13, 2013-09-04 01:22:22]

https://codereview.chromium.org/23171014/diff/13001/cc/layers/picture_layer_i...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/23171014/diff/13001/cc/layers/picture_layer_i...
cc/layers/picture_layer_impl.cc:324: gfx::Size device_viewport_size =
layer_tree_impl()->DeviceViewport().size();
OK, I went with "DrawViewportSize" and "DeviceViewport", and added more comments
in the LTHI header.  Now only the Renderer subclasses use DeviceViewport() and
you can only obtain the size via the LayerTreeImpl.  I also renamed
"DeviceTransform" to "DrawTransform" to connote it's a draw space concept.

[enne (OOO), 2013-09-04 17:20:51]

lgtm.  Thanks for the naming and the comments.  It's a lot more clear now.

[aelias_OOO_until_Jul13, 2013-09-04 20:53:29]

OK, need joth@ OWNERS for android_webview/ changes.

[joth, 2013-09-04 21:55:58]

lgtm!

[commit-bot: I haz the power, 2013-09-05 00:41:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/23171014/38001

[commit-bot: I haz the power, 2013-09-05 00:43:33]

Change committed as 221321