Clipping textured quads explicitly without scissoring

cc/texture_layer_impl.cc

Index: cc/texture_layer_impl.cc
diff --git a/cc/texture_layer_impl.cc b/cc/texture_layer_impl.cc
index e8a68741855891ef8d8e1317942c96484d7994bc..149f9ac17f02ec90b9d3a0b168e0ad49858893cd 100644
--- a/cc/texture_layer_impl.cc
+++ b/cc/texture_layer_impl.cc
@@ -45,6 +45,61 @@ void TextureLayerImpl::appendQuads(QuadSink& quadSink, AppendQuadsData& appendQu
     gfx::Rect opaqueRect(contentsOpaque() ? quadRect : gfx::Rect());
     scoped_ptr<TextureDrawQuad> quad = TextureDrawQuad::Create();
     quad->SetNew(sharedQuadState, quadRect, opaqueRect, m_externalTextureResource, m_premultipliedAlpha, m_uvRect, m_flipped);
+
+    if (quad->quadTransform().IsScaleOrTranslation())

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

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

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

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:

+    {
+        // Grab the corners of our boxes.
+        float rect_min_x = quad->rect.x();
+        float rect_min_y = quad->rect.y();
+        float rect_max_x = rect_min_x + quad->rect.width();
+        float rect_max_y = rect_min_y + quad->rect.height();
+
+        // Grab the relevant values from our matrix.
+        float x_scale = quad->quadTransform().matrix().getDouble(0,0);
+        float y_scale = quad->quadTransform().matrix().getDouble(1,1);
+        float x_offset = quad->quadTransform().matrix().getDouble(0,3);
+        float y_offset = quad->quadTransform().matrix().getDouble(1,3);
+
+        // Transform the rect to the same space as the clip.
+        rect_min_x = x_scale * rect_min_x + x_offset;
+        rect_min_y = y_scale * rect_min_y + y_offset;
+        rect_max_x = x_scale * rect_max_x + x_offset;
+        rect_max_y = y_scale * rect_max_y + y_offset;
+
+        // Grab the clipped region.
+        float clip_min_x = quad->clipRect().x();
+        float clip_min_y = quad->clipRect().y();
+        float clip_max_x = clip_min_x + quad->clipRect().width();
+        float clip_max_y = clip_min_y + quad->clipRect().height();
+
+        // Perform clipping on the rect.

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

Unless there's some performance caveats, I think we should use the existing
gfx::rect Intersect() method as appropriate.

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

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:

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

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.

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

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:

+        float min_x = std::max(rect_min_x, clip_min_x);
+        float min_y = std::max(rect_min_y, clip_min_y);
+        float max_x = std::min(rect_max_x, clip_max_x);
+        float max_y = std::min(rect_max_y, clip_max_y);
+
+        // Empty bounds mean that this is clipped completely away.
+        if (min_x > max_x || min_y > max_y)
+          return;
+
+        // Create a new uv-rect by clipping the old one to the new bounds.
+        quad->uv_rect = gfx::RectF(
+            quad->uv_rect.x() + quad->uv_rect.width () * (1.0f / (rect_max_x - rect_min_x)) * (min_x - rect_min_x),

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

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.

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

Ditto as previous comment.

On 2012/12/03 23:34:08, shawnsingh wrote:

+            quad->uv_rect.y() + quad->uv_rect.height() * (1.0f / (rect_max_y - rect_min_y)) * (min_y - rect_min_y),
+            quad->uv_rect.width () * (1.0f / (rect_max_x - rect_min_x)) * (max_x - min_x),
+            quad->uv_rect.height() * (1.0f / (rect_max_y - rect_min_y)) * (max_y - min_y));
+
+        // Move the clipped rectangle back into its space.
+        quad->rect = gfx::Rect(

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

Its likely that scaling and translation are operations provided by gfx::Rect,
too. Unless there's some performance caveats that come up.

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

Ditto again.

On 2012/12/03 23:34:08, shawnsingh wrote:

+            (int)((min_x - x_offset) * (1.0f / x_scale) + 0.5f),

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

we prefer c++ style casts to c-style cases in chromium, so this would be

static_cast<int>(expression)

same for following lines

+            (int)((min_y - y_offset) * (1.0f / y_scale) + 0.5f),
+            (int)((max_x - min_x) * (1.0f / x_scale) + 0.5f),
+            (int)((max_y - min_y) * (1.0f / y_scale) + 0.5f));
+
+        // Disable hardware clipping
+        sharedQuadState->is_clipped = false;
+    }
+
     quadSink.append(quad.PassAs<DrawQuad>(), appendQuadsData);
 }