Clipping textured quads explicitly without scissoring

This patch makes quad-clipping to a scissor window an explicit operation at quad-generation time.  If the transform for the quad is a scale and or a transform clipping is a simple bounding box intersection and uv-recomputation.

BUG=161871
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171850

[whunt, 2012-12-03 23:23:55]

Publishing so mail goes out!

[shawnsingh, 2012-12-03 23:34:08]

Sweet!

https://codereview.chromium.org/11418297/diff/1/cc/texture_layer_impl.cc
File cc/texture_layer_impl.cc (right):

https://codereview.chromium.org/11418297/diff/1/cc/texture_layer_impl.cc#newc...
cc/texture_layer_impl.cc:75: // Perform clipping on the rect.
Unless there's some performance caveats, I think we should use the existing
gfx::rect Intersect() method as appropriate.

https://codereview.chromium.org/11418297/diff/1/cc/texture_layer_impl.cc#newc...
cc/texture_layer_impl.cc:87: quad->uv_rect.x() + quad->uv_rect.width () * (1.0f
/ (rect_max_x - rect_min_x)) * (min_x - rect_min_x),
how about (min_x - rect_min_x) / (rect_max_x - rect_min_x) 
and similar for y value

And if we can use gfx::Rect type, the width() accessor will probably look nice
here, too.

https://codereview.chromium.org/11418297/diff/1/cc/texture_layer_impl.cc#newc...
cc/texture_layer_impl.cc:93: quad->rect = gfx::Rect(
Its likely that scaling and translation are operations provided by gfx::Rect,
too. Unless there's some performance caveats that come up.

[shawnsingh, 2012-12-03 23:37:52]

https://codereview.chromium.org/11418297/diff/1/cc/texture_layer_impl.cc
File cc/texture_layer_impl.cc (right):

https://codereview.chromium.org/11418297/diff/1/cc/texture_layer_impl.cc#newc...
cc/texture_layer_impl.cc:49: if (quad->quadTransform().IsScaleOrTranslation())
A few more thoughts:

(1) Don't we need an isClipped() check here, to avoid entering this code
unnecessarily?

(2) As per my usual OCD, I'd prefer this to be a helper function, something like
"computeUnclippedQuad()"  which does the following:

  DCHECK(isScaleOrTranslation)
  DCHECK(isClipped)
  // all the rest of your following code

[enne (OOO), 2012-12-03 23:47:35]

Are you planning to do this for tile draw quads too? If so, is there some way to
share code?

[enne (OOO), 2012-12-03 23:50:12]

I wanted to also add that this is pretty similar to what's being done for tile
draw quads in GLRenderer::drawTileQuad, mapping uv rects from quad_rect to
visible_rect.

[whunt, 2012-12-04 00:01:24]

https://codereview.chromium.org/11418297/diff/1/cc/texture_layer_impl.cc
File cc/texture_layer_impl.cc (right):

https://codereview.chromium.org/11418297/diff/1/cc/texture_layer_impl.cc#newc...
cc/texture_layer_impl.cc:49: if (quad->quadTransform().IsScaleOrTranslation())
I added the isClipped check.

I can make it a helper-function but the helper function would have several
undesirable properties.  It would have to defer control flow to implement the
early out (return a boolean that gets branched on again), would be a static
function that modifies its arguments in place (not at all functional) and would
only be called in one place.

If we want to extend this functionality to the other layer types we'd want to
put it in some layer-base class and we're stuck making it a helper function but
we're not doing that so far.

I'm of the opinion that good comments and code outlining is just as clear as a
helper function.

On 2012/12/03 23:37:52, shawnsingh wrote:
> A few more thoughts:
> 
> (1) Don't we need an isClipped() check here, to avoid entering this code
> unnecessarily?
> 
> (2) As per my usual OCD, I'd prefer this to be a helper function, something
like
> "computeUnclippedQuad()"  which does the following:
> 
>   DCHECK(isScaleOrTranslation)
>   DCHECK(isClipped)
>   // all the rest of your following code

https://codereview.chromium.org/11418297/diff/1/cc/texture_layer_impl.cc#newc...
cc/texture_layer_impl.cc:75: // Perform clipping on the rect.
There are certainly performance caveats to using gfx::RectF.  I can transform
the code but it will slow down.

On 2012/12/03 23:34:08, shawnsingh wrote:
> Unless there's some performance caveats, I think we should use the existing
> gfx::rect Intersect() method as appropriate.

https://codereview.chromium.org/11418297/diff/1/cc/texture_layer_impl.cc#newc...
cc/texture_layer_impl.cc:87: quad->uv_rect.x() + quad->uv_rect.width () * (1.0f
/ (rect_max_x - rect_min_x)) * (min_x - rect_min_x),
Ditto as previous comment.

On 2012/12/03 23:34:08, shawnsingh wrote:
> how about (min_x - rect_min_x) / (rect_max_x - rect_min_x) 
> and similar for y value
> 
> And if we can use gfx::Rect type, the width() accessor will probably look nice
> here, too.

https://codereview.chromium.org/11418297/diff/1/cc/texture_layer_impl.cc#newc...
cc/texture_layer_impl.cc:93: quad->rect = gfx::Rect(
Ditto again.

On 2012/12/03 23:34:08, shawnsingh wrote:
> Its likely that scaling and translation are operations provided by gfx::Rect,
> too. Unless there's some performance caveats that come up.

[jamesr, 2012-12-04 00:08:07]

I'd like to use the clipping + uv adjustments for render pass quads to land
https://codereview.chromium.org/11420079/, so I think we should bite the bullet
with a helper function sooner rather than later.  I'd be happy to extract it
when landing a second caller of a function and leave this patch inline.

[jamesr, 2012-12-04 00:15:42]

https://codereview.chromium.org/11418297/diff/1/cc/texture_layer_impl.cc
File cc/texture_layer_impl.cc (right):

https://codereview.chromium.org/11418297/diff/1/cc/texture_layer_impl.cc#newc...
cc/texture_layer_impl.cc:75: // Perform clipping on the rect.
On 2012/12/04 00:01:24, whunt wrote:
> There are certainly performance caveats to using gfx::RectF.  I can transform
> the code but it will slow down.

Would you mind running cc_perftests on this with this version and using the
gfx::Rect* types to see what the difference is like in practice?  If there is a
measurable difference then we definitely should use the faster code, but if the
performance ends up being very similar (or can be made similar with inlining,
etc) then we should use common functionality.

https://codereview.chromium.org/11418297/diff/1/cc/texture_layer_impl.cc#newc...
cc/texture_layer_impl.cc:94: (int)((min_x - x_offset) * (1.0f / x_scale) +
0.5f),
we prefer c++ style casts to c-style cases in chromium, so this would be

static_cast<int>(expression)

same for following lines

[whunt, 2012-12-04 19:35:04]

https://codereview.chromium.org/11418297/diff/1/cc/texture_layer_impl.cc
File cc/texture_layer_impl.cc (right):

https://codereview.chromium.org/11418297/diff/1/cc/texture_layer_impl.cc#newc...
cc/texture_layer_impl.cc:75: // Perform clipping on the rect.
Given that this code gets run ~8 times/frame in the tab-switcher I can guarantee
it's not going to show up on a performance test.


On 2012/12/04 00:15:42, jamesr wrote:
> On 2012/12/04 00:01:24, whunt wrote:
> > There are certainly performance caveats to using gfx::RectF.  I can
transform
> > the code but it will slow down.
> 
> Would you mind running cc_perftests on this with this version and using the
> gfx::Rect* types to see what the difference is like in practice?  If there is
a
> measurable difference then we definitely should use the faster code, but if
the
> performance ends up being very similar (or can be made similar with inlining,
> etc) then we should use common functionality.

[jamesr, 2012-12-05 23:19:45]

lgtm but needs a bit more formatting tweaks before landing

https://codereview.chromium.org/11418297/diff/3004/cc/texture_draw_quad.cc
File cc/texture_draw_quad.cc (right):

https://codereview.chromium.org/11418297/diff/3004/cc/texture_draw_quad.cc#ne...
cc/texture_draw_quad.cc:62: {
{ goes on the previous line

https://codereview.chromium.org/11418297/diff/3004/cc/texture_draw_quad.cc#ne...
cc/texture_draw_quad.cc:63: // This only occurs if the rect is only scaled and
translated (and thus still axis alinged).
typo "alinged" -> "aligned"

https://codereview.chromium.org/11418297/diff/3004/cc/texture_draw_quad.cc#ne...
cc/texture_draw_quad.cc:64: if (quadTransform().IsScaleOrTranslation())
you'd have slightly less indentation if this was phrased as an early return,
i.e.:

if (!quadTransform().IsScaleOrTranslation())
  return;

// rest of function body

https://codereview.chromium.org/11418297/diff/3004/cc/texture_draw_quad.cc#ne...
cc/texture_draw_quad.cc:65: {
{ goes on the previous line

https://codereview.chromium.org/11418297/diff/3004/cc/texture_draw_quad.cc#ne...
cc/texture_draw_quad.cc:70: quadTransform().matrix().getDouble(0,3),
continuations should have 4-space indents

https://codereview.chromium.org/11418297/diff/3004/cc/texture_draw_quad.cc#ne...
cc/texture_draw_quad.cc:81: {
{ on previous line

https://codereview.chromium.org/11418297/diff/3004/cc/texture_draw_quad.cc#ne...
cc/texture_draw_quad.cc:89: uv_rect.x() + uv_rect.width () / rectF.width () *
(clippedRect.x() - rectF.x()),
continuations should be indented 4 spaces from previous line, and please wrap
this to 80col since this is a chromium-style file

https://codereview.chromium.org/11418297/diff/3004/cc/texture_draw_quad.cc#ne...
cc/texture_draw_quad.cc:98: static_cast<int>(clippedRect.x() + 0.5f),
4 space indent

[commit-bot: I haz the power, 2012-12-05 23:58:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/whunt@chromium.org/11418297/14001

[jamesr, 2012-12-06 00:02:56]

https://chromiumcodereview.appspot.com/11418297/diff/3004/cc/texture_draw_quad.h
File cc/texture_draw_quad.h (right):

https://chromiumcodereview.appspot.com/11418297/diff/3004/cc/texture_draw_qua...
cc/texture_draw_quad.h:44: bool const PerformClipping();
actually I missed this first time around - what does "bool const foo()" mean in
C++ ? the return type is by value

[commit-bot: I haz the power, 2012-12-06 04:40:03]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-07 18:12:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/whunt@chromium.org/11418297/30001

[commit-bot: I haz the power, 2012-12-07 21:47:18]

Change committed as 171850