Implements core logic for Pixel Canvas

ui/compositor/paint_info.cc

+// Copyright (c) 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ui/compositor/paint_info.h"
+
+#include "ui/compositor/compositor_switches.h"
+
+namespace ui {
+
+PaintInfo::PaintInfo(const PaintContext& context, const gfx::Size& size)
+    : paint_recording_scale_x_(IsPixelCanvas() ? context.device_scale_factor()
+                                               : 1.f),
+      paint_recording_scale_y_(paint_recording_scale_x_),
+      paint_recording_bounds_(
+          gfx::ScaleToRoundedRect(gfx::Rect(size), paint_recording_scale_x_)),

[danakj, 2017/07/21 17:29:39]

Rounding is almost always not what you want to do when you scale. Scaling means
you can have partial pixels in your rect. Rounding means you don't know if those
partial pixels will be included in the resulting rect or not.

Can you explain why rounding is the right thing here? What are all the edge
cases and how it does the right thing?

[malaykeshav, 2017/07/21 23:30:17]

That's true. But if we round at every location in the code the result is always
consistent and aligned. The idea is to use rounding logic to scale at all
locations and to use enclosing/enclosed only at places where rounding logic does
not satisfy or is a special case. Doing this for ui/views/ so far gave accurate
results and solved a lot(MOST) of the pixel misalignment issues.

I also did a dry run by replacing _EVERY_ {scale by enclosing and scale by
enclosed for gfx::Rect} in chromium to always use {scale by rounding logic}, and
everything looked good at first look. There was just 1 instance where I could
find a bug on Chrome OS which later turned out was an existing bug that was only
made more visible due to this dry run.

Caveat: The use of rounding logic everywhere only works with coordinates. That
is, it works for bounds (gfx::Rect) that has 4 corner points or a point with
{x,y} values. 
This cannot be applied in case of gfx::Size as it only has width and height, but
no coordinate points.
So far I have not been able to find many edge cases specifically related to
this, but all other minor bugs that this change causes is tracked in
crbug/731293. Most of those bugs already have a fix and would be pushed once
this core change is committed.

[danakj, 2017/07/25 17:58:56]

Not finding bugs easily isn't surprising, they would be off-by-ones which would
hide until they came up. I can give you a specific example:

OcclusionTracker takes the enclosed rect when making a Rect for occlusion data.
And takes an enclosing rect when making a Rect to compare against it.

If it were to round instead, a partial pixel could be considered as occluding
pixels below it, but would actually blend with them. Then we'd not draw things
below and we'd get black (or undefined) ghosting in the pixels instead of the
correct output.

Partial pixel bugs may be hard for you to just find randomly using chrome. But
would show up in cases like partial swap, occlusion, or blending.

If you round here, then might you see that at different sizes, an object that it
goes from being 1px being partially filled on the edges to 1px being cropped off
the edge of the recording, or something like this?

[malaykeshav, 2017/07/25 22:57:57]

For pixel canvas we are only modifying the scaling logic for ui/views/view
coordinates. The rounding logic does not effect the top level layer.
Can doing just this cause the occlusion bug as well?
This is the bug that I observed when I changed _EVERYTHING_ to rounding logic.
Something similar happens even without this change that I have been unable to
track. But reading this it seems this might be related.

+      context_(context, gfx::Vector2d()),
+      root_context_(&context) {}
+
+PaintInfo::PaintInfo(const PaintInfo& other,
+                     const gfx::Rect& bounds,
+                     const gfx::Rect& parent_bounds,
+                     ScaleType scale_type)
+    : paint_recording_bounds_(
+          other.GetSnappedRecordingBounds(parent_bounds, bounds)),
+      offset_from_parent_(paint_recording_bounds_.OffsetFromOrigin() -
+                          other.paint_recording_bounds_.OffsetFromOrigin()),
+      context_(other.context(), offset_from_parent_),
+      root_context_(nullptr) {
+  ComputePaintRecordingScales(bounds.size(), scale_type);
+}
+
+PaintInfo::PaintInfo(const PaintInfo& other)
+    : paint_recording_scale_x_(other.paint_recording_scale_x_),
+      paint_recording_scale_y_(other.paint_recording_scale_y_),
+      paint_recording_bounds_(other.paint_recording_bounds_),
+      offset_from_parent_(other.offset_from_parent_),
+      context_(other.context(), ui::PaintContext::CLONE_WITHOUT_INVALIDATION),
+      root_context_(nullptr) {}
+
+PaintInfo::~PaintInfo() {}
+
+// static
+bool PaintInfo::IsPixelCanvas() {
+  return IsPixelCanvasRecordingEnabled();

[danakj, 2017/07/21 17:29:39]

pass this in the constructor, instead of reading the command line. Unit tests
should just be able to make objects and pass in how they want them to run,
rather than setting global states that get read later.

Two choices I think:
1) Leave the invalidation in DIP on the PaintContext. Consumer of PaintContext
(View::PaintFromPaintRoot) scales the invalidation if the
compositor->RecordPixelCanvas() or whatever is true, and passes that bool to the
PaintInfo if needed as well.

2) Scale the invalidation on the PaintContext. Then the user of PaintContext
needs to know what state the PaintContext uses, so pass the
compositor->RecordPixelCanvas() or whatever to the PaintContext constructor and
expose it like the DSF as a property.

[malaykeshav, 2017/07/21 23:30:17]

Done. Passing it to PaintContext should propagate it to rest of the code.

+}
+
+void PaintInfo::ComputePaintRecordingScales(const gfx::Size& size,
+                                            ScaleType scale_type) {
+  if (!IsPixelCanvas()) {
+    // If pixel canvas is disabled, all recordings are done in DIPs. These are
+    // later played back on to the raster canvas at |device_scale_factor_|.
+    paint_recording_scale_x_ = paint_recording_scale_y_ = 1.f;
+    return;
+  }
+
+  paint_recording_scale_x_ = paint_recording_scale_y_ =
+      context().device_scale_factor();
+  if (scale_type == ScaleType::kScaleToScaleFactor)
+    return;
+
+  if (size.width() > 0) {
+    paint_recording_scale_x_ =
+        static_cast<float>(paint_recording_bounds_.width()) /
+        static_cast<float>(size.width());
+  }
+
+  if (size.height() > 0) {
+    paint_recording_scale_y_ =
+        static_cast<float>(paint_recording_bounds_.height()) /
+        static_cast<float>(size.height());
+  }
+}
+
+gfx::Rect PaintInfo::GetSnappedRecordingBounds(
+    const gfx::Rect& parent_bounds,
+    const gfx::Rect& child_bounds) const {
+  if (!IsPixelCanvas())
+    return (child_bounds + paint_recording_bounds_.OffsetFromOrigin());
+
+  const gfx::Vector2d& child_origin = child_bounds.OffsetFromOrigin();
+
+  int right = child_origin.x() + child_bounds.width();
+  int bottom = child_origin.y() + child_bounds.height();
+
+  int new_x = std::round(child_origin.x() * context().device_scale_factor());
+  int new_y = std::round(child_origin.y() * context().device_scale_factor());
+
+  int new_right;
+  int new_bottom;
+
+  if (right == parent_bounds.width()) {
+    new_right = paint_recording_bounds_.width();
+  } else {
+    new_right = std::round(right * context().device_scale_factor());
+  }
+
+  if (bottom == parent_bounds.height()) {
+    new_bottom = paint_recording_bounds_.height();
+  } else {
+    new_bottom = std::round(bottom * context().device_scale_factor());
+  }
+
+  return gfx::Rect(new_x + paint_recording_bounds_.x(),
+                   new_y + paint_recording_bounds_.y(), new_right - new_x,
+                   new_bottom - new_y);
+}
+
+}  // namespace ui