Fixed rounding issue on scrollbar rasterization.

cc/layers/painted_scrollbar_layer.cc

 // Copyright 2013 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 "cc/layers/painted_scrollbar_layer.h"
 
 #include "base/auto_reset.h"
 #include "base/basictypes.h"
 #include "base/debug/trace_event.h"
 #include "cc/layers/painted_scrollbar_layer_impl.h"
@@ skipping 152 matching lines @@
   }
 
   ContentsScalingLayer::SetLayerTreeHost(host);
 }
 
 gfx::Rect PaintedScrollbarLayer::ScrollbarLayerRectToContentRect(
     const gfx::Rect& layer_rect) const {
   // Don't intersect with the bounds as in LayerRectToContentRect() because
   // layer_rect here might be in coordinates of the containing layer.
   gfx::Rect expanded_rect = gfx::ScaleToEnclosingRect(
-      layer_rect, contents_scale_y(), contents_scale_y());
+      layer_rect, contents_scale_x(), contents_scale_y());
   // We should never return a rect bigger than the content_bounds().
   gfx::Size clamped_size = expanded_rect.size();
   clamped_size.SetToMin(content_bounds());
   expanded_rect.set_size(clamped_size);
   return expanded_rect;
 }
 
 gfx::Rect PaintedScrollbarLayer::OriginThumbRect() const {
   gfx::Size thumb_size;
   if (orientation() == HORIZONTAL) {
     thumb_size =
         gfx::Size(scrollbar_->ThumbLength(), scrollbar_->ThumbThickness());
   } else {
     thumb_size =
         gfx::Size(scrollbar_->ThumbThickness(), scrollbar_->ThumbLength());
   }
-  return ScrollbarLayerRectToContentRect(gfx::Rect(thumb_size));
+  return gfx::Rect(thumb_size);
 }
 
 void PaintedScrollbarLayer::UpdateThumbAndTrackGeometry() {
   UpdateProperty(scrollbar_->TrackRect(), &track_rect_);
   UpdateProperty(scrollbar_->Location(), &location_);
   UpdateProperty(scrollbar_->IsOverlay(), &is_overlay_);
   UpdateProperty(scrollbar_->HasThumb(), &has_thumb_);
   if (has_thumb_) {
     UpdateProperty(scrollbar_->ThumbThickness(), &thumb_thickness_);
     UpdateProperty(scrollbar_->ThumbLength(), &thumb_length_);
   }
 }
 
 bool PaintedScrollbarLayer::Update(ResourceUpdateQueue* queue,
                                    const OcclusionTracker* occlusion) {
   UpdateThumbAndTrackGeometry();
 
+  gfx::Rect track_layer_rect = gfx::Rect(location_, bounds());
   gfx::Rect scaled_track_rect = ScrollbarLayerRectToContentRect(
-      gfx::Rect(location_, bounds()));
+      track_layer_rect);
 
   if (track_rect_.IsEmpty() || scaled_track_rect.IsEmpty())
     return false;
 
   {
     base::AutoReset<bool> ignore_set_needs_commit(&ignore_set_needs_commit_,
                                                   true);
     ContentsScalingLayer::Update(queue, occlusion);
   }
 
   if (update_rect_.IsEmpty() && track_resource_)
     return false;
 
   track_resource_ = ScopedUIResource::Create(
-      layer_tree_host(), RasterizeScrollbarPart(scaled_track_rect, TRACK));
+      layer_tree_host(),
+      RasterizeScrollbarPart(track_layer_rect, scaled_track_rect, TRACK));
 
-  gfx::Rect thumb_rect = OriginThumbRect();
-  if (has_thumb_ && !thumb_rect.IsEmpty()) {
+  gfx::Rect thumb_layer_rect = OriginThumbRect();
+  gfx::Rect scaled_thumb_rect =
+      ScrollbarLayerRectToContentRect(thumb_layer_rect);
+  if (has_thumb_ && !scaled_thumb_rect.IsEmpty()) {
     thumb_resource_ = ScopedUIResource::Create(
-        layer_tree_host(), RasterizeScrollbarPart(thumb_rect, THUMB));
+        layer_tree_host(),
+        RasterizeScrollbarPart(thumb_layer_rect, scaled_thumb_rect, THUMB));
   }
 
   // UI resources changed so push properties is needed.
   SetNeedsPushProperties();
   return true;
 }
 
+float PaintedScrollbarLayer::calculateIdealScale(

[danakj, 2014/02/14 20:38:31]

CalculateScaleToBitmap?

make this a file-static method? It doesn't appear to benefit from being a member
of the class.

+    float content_scale,
+    int layer_edge,
+    int layer_size,
+    int target_size) const {

[danakj, 2014/02/14 20:38:31]

bitmap_size?

+  const float EPSILLON = 0.1f;

[danakj, 2014/02/14 20:38:31]

kEpsilon

+
+  DCHECK(layer_size);
+  DCHECK(target_size);
+
+  // Because skia rounds to the nearest pixel when scaling, scaling by the
+  // content scale can lead to painting thinner than the bitmap rect (e.g. if
+  // the left edge gets rounded up and the right edge is rounded down). We
+  // calculate an "ideal scale" that will give us scale the layer rect to the
+  // same size as the bitmap.
+
+  // Of course, due to the precission loss of floating point arithmetic, we'll

[danakj, 2014/02/14 20:38:31]

precision

+  // likely be off by some epsillon which leads to edge cases when we're

[danakj, 2014/02/14 20:38:31]

epsilon

+  // right around 0.5. To be safe, we adjust the scale by an epsillon depending

[danakj, 2014/02/14 20:38:31]

epsilon

+  // on which direction the leading edge was rounded in. This ensures we're
+  // always rounding both edges such that the width after rounding is exactly
+  // equal to the target width.
+
+  float scaled_layer_edge = layer_edge * content_scale;

[danakj, 2014/02/14 20:38:31]

Can you explain this bit here? I'm a little confused by it. You're using the
left/top edge to adjust the scale factor by 0.1/layer_size.. I'm not clear what
situation you're trying to deal with here?

[bokan, 2014/02/17 17:11:50]

Sorry, I've been massively over-thinking this. The problem I was trying to solve
was:

Suppose left_edge*scale lands exactly on 10.5, it rounds up to 11. Say
bitmap_size is 5, but scale*layer_size=4.9999 due to floating point rounding.
The right edge will land on 15.49999 which will get rounded down to 15, 15-11=4
so we lost a pixel. 

In trying to explain my solution I realized it's wrong. All this was being
complicated by the fact that we were scaling something that's not at the origin.
The *obvious* solution is to translate to origin first, then scale. I've done
that in a new patch, all this went away.

+  bool rounding_up = round(scaled_layer_edge) > scaled_layer_edge;
+  float adjusted_target_size = rounding_up ? target_size + EPSILLON
+                                           : target_size - EPSILLON;
+
+  return adjusted_target_size / layer_size;
+}
+
 UIResourceBitmap PaintedScrollbarLayer::RasterizeScrollbarPart(
-    const gfx::Rect& rect,
+    const gfx::Rect& layer_rect,
+    const gfx::Rect& content_rect,
     ScrollbarPart part) {
-  DCHECK(!rect.size().IsEmpty());
+  DCHECK(!content_rect.size().IsEmpty());
+  DCHECK(!layer_rect.size().IsEmpty());
 
   SkBitmap skbitmap;
-  skbitmap.setConfig(SkBitmap::kARGB_8888_Config, rect.width(), rect.height());
+  skbitmap.setConfig(SkBitmap::kARGB_8888_Config,
+                     content_rect.width(),
+                     content_rect.height());
   skbitmap.allocPixels();
 
   SkCanvas skcanvas(skbitmap);
-  skcanvas.translate(SkFloatToScalar(-rect.x()), SkFloatToScalar(-rect.y()));
-  skcanvas.scale(SkFloatToScalar(contents_scale_x()),
-                 SkFloatToScalar(contents_scale_y()));
 
-  gfx::Rect layer_rect = gfx::ScaleToEnclosingRect(
-      rect, 1.f / contents_scale_x(), 1.f / contents_scale_y());
+  float ideal_scale_x = calculateIdealScale(contents_scale_x(),
+                                            layer_rect.x(),
+                                            layer_rect.width(),
+                                            content_rect.width());
+  float ideal_scale_y = calculateIdealScale(contents_scale_y(),
+                                            layer_rect.y(),
+                                            layer_rect.height(),
+                                            content_rect.height());
+
+  // Skia rounds scaled coordinates.
+  float rounded_x = round(layer_rect.x() * ideal_scale_x);
+  float rounded_y = round(layer_rect.y() * ideal_scale_y);
+  skcanvas.translate(SkFloatToScalar(-rounded_x), SkFloatToScalar(-rounded_y));

[danakj, 2014/02/14 20:38:31]

Ok so, I'm not sure why you need this translate. This is basically trying to
pixel-snap the left/top edge of the thing you're drawing?

But things that start at 0 will be scaled to 0 still, and then shifted left
slightly?

[bokan, 2014/02/17 17:11:50]

The PaintPart method for the track draws directly to the x/y offset of the
scrollbar. We want the bitmap origin to be the scrollbar's top right corner so
we need this translation to transform from unscaled-page coordinates into scaled
scrollbar coordinates.

In the new patch, I've flipped the transforms here so we translate to the origin
before scaling. All this rounding nonsense is no longer necessary.

+  skcanvas.scale(SkFloatToScalar(ideal_scale_x),
+                 SkFloatToScalar(ideal_scale_y));
+
   SkRect layer_skrect = RectToSkRect(layer_rect);
   SkPaint paint;
   paint.setAntiAlias(false);
   paint.setXfermodeMode(SkXfermode::kClear_Mode);
   skcanvas.drawRect(layer_skrect, paint);
   skcanvas.clipRect(layer_skrect);
 
   scrollbar_->PaintPart(&skcanvas, part, layer_rect);
   // Make sure that the pixels are no longer mutable to unavoid unnecessary
   // allocation and copying.
   skbitmap.setImmutable();
 
   return UIResourceBitmap(skbitmap);
 }
 
 }  // namespace cc