Add detection for rotation and flip overlay transforms

cc/output/overlay_candidate.cc

Index: cc/output/overlay_candidate.cc
diff --git a/cc/output/overlay_candidate.cc b/cc/output/overlay_candidate.cc
index 0cbd3deefa8de361f4b9273c8d705a8743d9445e..db3bbc5a5528008abab6f066688885d218c6a4be 100644
--- a/cc/output/overlay_candidate.cc
+++ b/cc/output/overlay_candidate.cc
@@ -6,10 +6,37 @@
 
 #include <algorithm>
 #include "base/logging.h"
+#include "ui/gfx/geometry/point3_f.h"
 #include "ui/gfx/geometry/rect_conversions.h"
 
 namespace cc {
 
+namespace {
+// Taken from SkMatrix44.

[danakj, 2015/05/11 16:58:04]

can you TODO pointing to a bug to make this constant public (or some public API)
on SkMatrix44 if we're going to depend on it, and follow up with that change in
skia?

I actually don't see this constant in SkMatrix44 anywhere, can you point me to
it?

[Ian Vollick, 2015/05/12 01:24:15]

I believe that this from transform.cc (which also cites this constant), but the
comment is now bogus -- the constant no longer exists in skia. Perhaps we could
expose it on Transform and get rid of this comment and the one in transform.cc?

[halliwell, 2015/05/12 01:57:09]

Correct, I just copied this comment from Transform without actually looking at
SkMatrix44 :)

I guess I could expose it on Transform, although when I sat down to write a
comment for that, I see that Transform uses it somewhat patchily and not
consistently with all such comparisons:
* Some functions accept a tolerance parameter (e.g.
IsApproximatelyIdentityOrTranslation)
* A number of tests fall through to SkMatrix logic, which is sometimes exact
(isTranslate), for determinants checks that 1/det is finite
* Transform::isBackFaceVisible and Transform::Preserves2dAxisAlignment are the
only places that use kEpsilon, the former uses it on a determinant and the
latter on elements (note that SkMatrix44::preserves2dAxisAlignment accepts a
tolerance parameter which defaults to a different constant, SK_ScalarNearlyZero)

So I would probably favour just deleting the comment and keeping as a local
constant here.  I also think this type of error bound, while a totally arbitrary
fudge factor, is at least somewhat possible to bound by the nature of the
calculation that leads to the data.  In other words, having a general-purpose
epsilon on Transform may not be so useful compared to the ability to improve the
error bounds and method locally in this file, given it's a specific calculation
that leads to these numbers.

Let me know what you think :)

[hendrikw, 2015/05/12 02:12:29]

Perhaps even replace it with std::numeric_limits<float>::epsilon()?

[halliwell, 2015/05/12 15:11:31]

Definitely not that :)  numeric_limits::epsilon is the next representable number
greater than 1.  The tolerance required here is significantly greater.

[hendrikw, 2015/05/12 16:12:39]

If accuracy isn't important for your use case, then cool! As is, the smaller the
scale on the matrix, the larger the rotation that will be recognized as not
OVERLAY_TRANSFORM_INVALID.

Personally, I always use stl's epsilon unless I've demonstrated a case where fp
errors to creep in (starting with an arbitrarily large epsilon is akin to early
optimizing code, imo).

[halliwell, 2015/05/12 22:22:19]

Well ... this number is actually smaller than std::epsilon, not larger ... but
yes it's completely arbitrary and I am not going to defend it :)

Actually I think both may be too large.  Already computing cos(90) in floats is
about 4x this epsilon, and getting close to std::epsilon.

I've been trying to think what would make a logical number, not just an
arbitrary fudge factor.  One thought is that if |4000*x| < 1 for each x in the
comparisons below, then a fullscreen quad on a 4k display with this transform
would be within a pixel of being axis-aligned.  If that logic is correct, then
an error bound of 1/8000 might be defensible (within half a pixel everywhere on
a 4k display), and alleviate my concern about the other values discussed being
too large.

Thoughts?

+const SkMScalar kEpsilon = 1e-8f;
+
+enum Axis {
+  AXIS_POS_X,
+  AXIS_NEG_X,
+  AXIS_POS_Y,
+  AXIS_NEG_Y,
+  NONE,

[danakj, 2015/05/11 16:58:04]

usually NONE would go first so it's 0

[halliwell, 2015/05/12 15:11:31]

Done.

+};
+
+Axis PointToAxis(const gfx::Point3F& point) {
+  if (std::abs(point.z()) > kEpsilon)
+    return NONE;
+  const bool x_zero = (std::abs(point.x()) <= kEpsilon);
+  const bool y_zero = (std::abs(point.y()) <= kEpsilon);
+  if (x_zero && !y_zero)
+    return (point.y() > 0) ? AXIS_POS_Y : AXIS_NEG_Y;
+  else if (y_zero && !x_zero)
+    return (point.x() > 0) ? AXIS_POS_X : AXIS_NEG_X;
+  else
+    return NONE;
+}
+}  // namespace
+
 OverlayCandidate::OverlayCandidate()
     : transform(gfx::OVERLAY_TRANSFORM_NONE),
       format(RGBA_8888),
@@ -24,11 +51,40 @@ OverlayCandidate::~OverlayCandidate() {}
 gfx::OverlayTransform OverlayCandidate::GetOverlayTransform(
     const gfx::Transform& quad_transform,
     bool flipped) {

[hendrikw, 2015/05/12 02:12:29]

rename to flipped_over_y_axis?

[halliwell, 2015/05/12 15:11:31]

I agree this isn't super clear, especially when "FLIP_HORIZONTAL" is one of the
possible outcomes of this function.  Otoh, it's named this way throughout the
quad data structures ... thoughts?

[hendrikw, 2015/05/12 16:12:39]

I don't know, I would probably start the rename here, it's a bit clearer.

[danakj, 2015/05/12 16:54:32]

+1 Feel free to improve the name elsewhere

[halliwell, 2015/05/12 22:22:20]

Ok, renamed here and in TextureDrawQuad.

-  if (!quad_transform.IsPositiveScaleOrTranslation())
+  if (!quad_transform.Preserves2dAxisAlignment()) {
     return gfx::OVERLAY_TRANSFORM_INVALID;
+  }
 
-  return flipped ? gfx::OVERLAY_TRANSFORM_FLIP_VERTICAL
-                 : gfx::OVERLAY_TRANSFORM_NONE;
+  gfx::Transform transform = quad_transform;
+  transform.matrix().set(0, 3, 0);
+  transform.matrix().set(1, 3, 0);
+  transform.matrix().set(2, 3, 0);
+  if (flipped) {
+    transform.Scale(0, -1);
+  }

[hendrikw, 2015/05/12 02:12:29]

Should probably have GetXAxis() and GetYAxis() functions in the matrix that just
returns [(0,0), (1,0), (2,0)] (row, col) and [(0,1), (1,1), (2,1)].

That way you don't need to make a matrix copy, set the translation, or transform
a point (which is misleading since you're treating it as a vector)

You'll need to multiply the y components of each axis by -1 when
flipped_over_y_axis is true (since you wouldn't have a copy of the matrix)

[halliwell, 2015/05/12 15:11:31]

Yep, GetXAxis/GetYAxis seem like a clear way to express this.  I put the
function here for now, not sure what the policy is for adding stuff to Transform
... existing API seems quite minimal?

[danakj, 2015/05/12 16:54:32]

You could put it in cc/base/math_util if cc is the only place trying to do this.

[halliwell, 2015/05/12 22:22:19]

Moved to cc::MathUtil.

+
+  gfx::Point3F x_axis(1, 0, 0);
+  gfx::Point3F y_axis(0, 1, 0);
+  transform.TransformPoint(&x_axis);
+  transform.TransformPoint(&y_axis);

[Ian Vollick, 2015/05/12 01:24:15]

Regarding Dana's question about whether we could do something better than
transforming points, we sorta could. We could avoid nuking the translation
components and construct a Point3F out of the first two columns of the matrix
instead, but I don't think the perf hit of doing it this way is significant and
this seems a bit easier to understand.

That said, I would add a comment explaining why you zero out the translation
bits.

[halliwell, 2015/05/12 15:11:31]

Indeed, not a huge perf hit, but seems silly to be doing a full transform.  I
like hendrikw@'s GetXAxis suggestion, went with that - think it's more
self-explanatory than zeroing the translation too.

+
+  Axis x_to = PointToAxis(x_axis);
+  Axis y_to = PointToAxis(y_axis);

[hendrikw, 2015/05/12 02:12:29]

your PointToAxis functions ignores scale, is that ok for your use case?  e.g. if
you have a 10x scale in the x direction, you'll report back as
gfx::OVERLAY_TRANSFORM_NONE.

[halliwell, 2015/05/12 15:11:31]

Correct.  TRANSFORM_NONE doesn't mean 'no transform', it just means 'no rotation
or flip'.  Translation and (positive) scale are handled separately in the
hardware overlay APIs.

+
+  if (x_to == AXIS_POS_X && y_to == AXIS_POS_Y)
+    return gfx::OVERLAY_TRANSFORM_NONE;
+  else if (x_to == AXIS_NEG_X && y_to == AXIS_POS_Y)
+    return gfx::OVERLAY_TRANSFORM_FLIP_HORIZONTAL;
+  else if (x_to == AXIS_POS_X && y_to == AXIS_NEG_Y)
+    return gfx::OVERLAY_TRANSFORM_FLIP_VERTICAL;
+  else if (x_to == AXIS_NEG_Y && y_to == AXIS_POS_X)
+    return gfx::OVERLAY_TRANSFORM_ROTATE_270;
+  else if (x_to == AXIS_NEG_X && y_to == AXIS_NEG_Y)
+    return gfx::OVERLAY_TRANSFORM_ROTATE_180;
+  else if (x_to == AXIS_POS_Y && y_to == AXIS_NEG_X)
+    return gfx::OVERLAY_TRANSFORM_ROTATE_90;
+  else
+    return gfx::OVERLAY_TRANSFORM_INVALID;
 }
 
 // static
@@ -131,7 +187,7 @@ gfx::OverlayTransform OverlayCandidate::ModifyTransform(
 gfx::RectF OverlayCandidate::GetOverlayRect(
     const gfx::Transform& quad_transform,
     const gfx::Rect& rect) {
-  DCHECK(quad_transform.IsPositiveScaleOrTranslation());
+  DCHECK(quad_transform.Preserves2dAxisAlignment());
 
   gfx::RectF float_rect(rect);
   quad_transform.TransformRect(&float_rect);