Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

Commit

Permalink
Revert of Fix bad scaling in TiledLayer (patchset #8 id:140001 of htt…
Browse files Browse the repository at this point in the history
…ps://codereview.chromium.org/567743003/)

Reason for revert:
This break Blink layout tests. See crbug.com/416775.

Original issue's description:
> Fix bad scaling in TiledLayer
>
> On the windows side (and likely chromeos for some devices) we can end
> up in a situation where the pixel bounds is not an even multiple of the
> scale factor. This means when TiledLayer calculates the scale it ends
> up off.
>
> BUG=416655
> TEST=added test coverage
> [email protected]
>
> Committed: https://crrev.com/c01746b4f7d811a5fdc75ce6570a9667b9877913
> Cr-Commit-Position: refs/heads/master@{#296108}

[email protected],[email protected]
NOTREECHECKS=true
NOTRY=true
BUG=416655

Review URL: https://codereview.chromium.org/590313004

Cr-Commit-Position: refs/heads/master@{#296158}
  • Loading branch information
madsager authored and Commit bot committed Sep 23, 2014
1 parent 235fd06 commit fcd7452
Show file tree
Hide file tree
Showing 14 changed files with 63 additions and 163 deletions.
1 change: 0 additions & 1 deletion cc/layers/content_layer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@ void ContentLayer::CreateUpdaterIfNeeded() {
updater_->SetOpaque(contents_opaque());
if (client_)
updater_->SetFillsBoundsCompletely(client_->FillsBoundsCompletely());
updater_->SetBackgroundColor(background_color());

SetTextureFormat(
layer_tree_host()->GetRendererCapabilities().best_texture_format);
Expand Down
15 changes: 8 additions & 7 deletions cc/layers/tiled_layer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -462,8 +462,12 @@ void TiledLayer::UpdateTileTextures(const gfx::Rect& update_rect,
const OcclusionTracker<Layer>* occlusion) {
// The update_rect should be in layer space. So we have to convert the
// paint_rect from content space to layer space.
float width_scale = 1 / draw_properties().contents_scale_x;
float height_scale = 1 / draw_properties().contents_scale_y;
float width_scale =
paint_properties().bounds.width() /
static_cast<float>(content_bounds().width());
float height_scale =
paint_properties().bounds.height() /
static_cast<float>(content_bounds().height());
update_rect_ = gfx::ScaleRect(update_rect, width_scale, height_scale);

// Calling PrepareToUpdate() calls into WebKit to paint, which may have the
Expand All @@ -472,11 +476,8 @@ void TiledLayer::UpdateTileTextures(const gfx::Rect& update_rect,
// the SkCanvas until the paint finishes, so we grab a local reference here to
// hold the updater alive until the paint completes.
scoped_refptr<LayerUpdater> protector(Updater());
Updater()->PrepareToUpdate(content_bounds(),
paint_rect,
tiler_->tile_size(),
1.f / width_scale,
1.f / height_scale);
Updater()->PrepareToUpdate(
paint_rect, tiler_->tile_size(), 1.f / width_scale, 1.f / height_scale);

for (int j = top; j <= bottom; ++j) {
for (int i = left; i <= right; ++i) {
Expand Down
43 changes: 5 additions & 38 deletions cc/layers/tiled_layer_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -289,27 +289,6 @@ TEST_F(TiledLayerTest, PushDirtyTiles) {
EXPECT_FALSE(layer_impl->HasResourceIdForTileAt(0, 1));
}

TEST_F(TiledLayerTest, Scale) {
layer_tree_host_->SetDeviceScaleFactor(1.5);

scoped_refptr<FakeTiledLayer> layer =
make_scoped_refptr(new FakeTiledLayer(resource_manager_.get()));
scoped_ptr<FakeTiledLayerImpl> layer_impl =
make_scoped_ptr(new FakeTiledLayerImpl(host_impl_->active_tree(), 1));
RenderSurfaceLayerList render_surface_layer_list;

layer_tree_host_->root_layer()->AddChild(layer);

layer->SetBounds(gfx::Size(100, 200));
CalcDrawProps(&render_surface_layer_list);

// Change the width so that it doesn't divide cleanly by the scale.
layer->SetBounds(gfx::Size(101, 200));
UpdateAndPush(layer, layer_impl);

EXPECT_EQ(1.5, layer->fake_layer_updater()->last_contents_width_scale());
}

TEST_F(TiledLayerTest, PushOccludedDirtyTiles) {
scoped_refptr<FakeTiledLayer> layer =
make_scoped_refptr(new FakeTiledLayer(resource_manager_.get()));
Expand Down Expand Up @@ -893,13 +872,11 @@ TEST_F(TiledLayerTest, VerifyUpdateRectWhenContentBoundsAreScaled) {
layer_tree_host_->root_layer()->AddChild(layer);

gfx::Rect layer_bounds(0, 0, 300, 200);
gfx::Rect content_bounds(0, 0, 150, 250);
gfx::Rect content_bounds(0, 0, 200, 250);

layer->SetBounds(layer_bounds.size());
layer->SetContentBounds(content_bounds.size());
layer->draw_properties().visible_content_rect = content_bounds;
layer->draw_properties().contents_scale_x = .5f;
layer->draw_properties().contents_scale_y = 1.25f;

// On first update, the update_rect includes all tiles, even beyond the
// boundaries of the layer.
Expand All @@ -910,9 +887,7 @@ TEST_F(TiledLayerTest, VerifyUpdateRectWhenContentBoundsAreScaled) {
resource_manager_->PrioritizeTextures();
layer->SavePaintProperties();
layer->Update(queue_.get(), NULL);

// Update rect is 200x300 (tile size of 100x100). Scaled this gives 400x240.
EXPECT_FLOAT_RECT_EQ(gfx::RectF(0, 0, 400, 240), layer->update_rect());
EXPECT_FLOAT_RECT_EQ(gfx::RectF(0, 0, 300, 300 * 0.8), layer->update_rect());
UpdateTextures();

// After the tiles are updated once, another invalidate only needs to update
Expand All @@ -933,7 +908,7 @@ TEST_F(TiledLayerTest, VerifyUpdateRectWhenContentBoundsAreScaled) {
resource_manager_->PrioritizeTextures();
layer->SavePaintProperties();
layer->Update(queue_.get(), NULL);
EXPECT_FLOAT_RECT_EQ(gfx::RectF(60, 80, 20, 8), layer->update_rect());
EXPECT_FLOAT_RECT_EQ(gfx::RectF(45, 80, 15, 8), layer->update_rect());
}

TEST_F(TiledLayerTest, VerifyInvalidationWhenContentsScaleChanges) {
Expand Down Expand Up @@ -1717,11 +1692,7 @@ TEST_F(TiledLayerTest, NonIntegerContentsScaleIsNotDistortedDuringPaint) {
layer->InvalidateContentRect(content_rect);
layer->Update(queue_.get(), NULL);

// Rounding leads to an extra pixel.
gfx::Rect expanded_layer_rect(layer_rect);
expanded_layer_rect.set_height(32);
EXPECT_RECT_EQ(expanded_layer_rect,
layer->tracking_layer_painter()->PaintedRect());
EXPECT_RECT_EQ(layer_rect, layer->tracking_layer_painter()->PaintedRect());
}

TEST_F(TiledLayerTest,
Expand Down Expand Up @@ -1756,11 +1727,7 @@ TEST_F(TiledLayerTest,
layer->SetNeedsDisplayRect(layer_rect);
layer->Update(queue_.get(), NULL);

// Rounding leads to an extra pixel.
gfx::Rect expanded_layer_rect(layer_rect);
expanded_layer_rect.set_height(32);
EXPECT_RECT_EQ(expanded_layer_rect,
layer->tracking_layer_painter()->PaintedRect());
EXPECT_RECT_EQ(layer_rect, layer->tracking_layer_painter()->PaintedRect());
}

} // namespace
Expand Down
28 changes: 14 additions & 14 deletions cc/resources/bitmap_content_layer_updater.cc
Original file line number Diff line number Diff line change
Expand Up @@ -54,34 +54,31 @@ scoped_ptr<LayerUpdater::Resource> BitmapContentLayerUpdater::CreateResource(
new Resource(this, PrioritizedResource::Create(manager)));
}

void BitmapContentLayerUpdater::PrepareToUpdate(const gfx::Size& content_size,
const gfx::Rect& paint_rect,
void BitmapContentLayerUpdater::PrepareToUpdate(const gfx::Rect& content_rect,
const gfx::Size& tile_size,
float contents_width_scale,
float contents_height_scale) {
if (canvas_size_ != paint_rect.size()) {
if (canvas_size_ != content_rect.size()) {
devtools_instrumentation::ScopedLayerTask paint_setup(
devtools_instrumentation::kPaintSetup, layer_id_);
canvas_size_ = paint_rect.size();
canvas_size_ = content_rect.size();
bitmap_backing_.allocN32Pixels(
canvas_size_.width(), canvas_size_.height(), layer_is_opaque_);
// TODO(danak): Remove when skia does the check for us: crbug.com/360384
canvas_ = skia::AdoptRef(new SkCanvas(bitmap_backing_));
DCHECK_EQ(paint_rect.width(), canvas_->getBaseLayerSize().width());
DCHECK_EQ(paint_rect.height(), canvas_->getBaseLayerSize().height());
DCHECK_EQ(content_rect.width(), canvas_->getBaseLayerSize().width());
DCHECK_EQ(content_rect.height(), canvas_->getBaseLayerSize().height());
}

base::TimeTicks start_time =
rendering_stats_instrumentation_->StartRecording();
PaintContents(canvas_.get(),
content_size,
paint_rect,
contents_width_scale,
contents_height_scale);
PaintContents(
canvas_.get(), content_rect, contents_width_scale, contents_height_scale);
base::TimeDelta duration =
rendering_stats_instrumentation_->EndRecording(start_time);
rendering_stats_instrumentation_->AddPaint(
duration, paint_rect.width() * paint_rect.height());
duration,
content_rect.width() * content_rect.height());
}

void BitmapContentLayerUpdater::UpdateTexture(ResourceUpdateQueue* queue,
Expand All @@ -90,8 +87,11 @@ void BitmapContentLayerUpdater::UpdateTexture(ResourceUpdateQueue* queue,
const gfx::Vector2d& dest_offset,
bool partial_update) {
CHECK(canvas_);
ResourceUpdate upload = ResourceUpdate::Create(
texture, &bitmap_backing_, paint_rect(), source_rect, dest_offset);
ResourceUpdate upload = ResourceUpdate::Create(texture,
&bitmap_backing_,
content_rect(),
source_rect,
dest_offset);
if (partial_update)
queue->AppendPartialUpload(upload);
else
Expand Down
3 changes: 1 addition & 2 deletions cc/resources/bitmap_content_layer_updater.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,7 @@ class CC_EXPORT BitmapContentLayerUpdater : public ContentLayerUpdater {

virtual scoped_ptr<LayerUpdater::Resource> CreateResource(
PrioritizedResourceManager* manager) OVERRIDE;
virtual void PrepareToUpdate(const gfx::Size& content_size,
const gfx::Rect& paint_rect,
virtual void PrepareToUpdate(const gfx::Rect& content_rect,
const gfx::Size& tile_size,
float contents_width_scale,
float contents_height_scale) OVERRIDE;
Expand Down
4 changes: 2 additions & 2 deletions cc/resources/bitmap_skpicture_content_layer_updater.cc
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ void BitmapSkPictureContentLayerUpdater::PaintContentsRect(
if (!canvas)
return;
// Translate the origin of content_rect to that of source_rect.
canvas->translate(paint_rect().x() - source_rect.x(),
paint_rect().y() - source_rect.y());
canvas->translate(content_rect().x() - source_rect.x(),
content_rect().y() - source_rect.y());
base::TimeTicks start_time =
rendering_stats_instrumentation_->StartRecording();
DrawPicture(canvas);
Expand Down
66 changes: 12 additions & 54 deletions cc/resources/content_layer_updater.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
#include "third_party/skia/include/core/SkScalar.h"
#include "ui/gfx/rect_conversions.h"
#include "ui/gfx/rect_f.h"
#include "ui/gfx/skia_util.h"

namespace cc {

Expand All @@ -26,9 +25,7 @@ ContentLayerUpdater::ContentLayerUpdater(
layer_id_(layer_id),
layer_is_opaque_(false),
layer_fills_bounds_completely_(false),
painter_(painter.Pass()),
background_color_(SK_ColorTRANSPARENT) {
}
painter_(painter.Pass()) {}

ContentLayerUpdater::~ContentLayerUpdater() {}

Expand All @@ -38,62 +35,27 @@ void ContentLayerUpdater::set_rendering_stats_instrumentation(
}

void ContentLayerUpdater::PaintContents(SkCanvas* canvas,
const gfx::Size& layer_content_size,
const gfx::Rect& paint_rect,
const gfx::Rect& content_rect,
float contents_width_scale,
float contents_height_scale) {
TRACE_EVENT0("cc", "ContentLayerUpdater::PaintContents");
if (!canvas)
return;
canvas->save();
canvas->translate(SkIntToScalar(-paint_rect.x()),
SkIntToScalar(-paint_rect.y()));

// The |canvas| backing should be sized to hold the |paint_rect|.
DCHECK_EQ(paint_rect.width(), canvas->getBaseLayerSize().width());
DCHECK_EQ(paint_rect.height(), canvas->getBaseLayerSize().height());

const bool is_scaled =
contents_width_scale != 1.f || contents_height_scale != 1.f;

if (is_scaled && (layer_is_opaque_ || layer_fills_bounds_completely_)) {
// Even if completely covered, for rasterizations that touch the edge of the
// layer, we also need to raster the background color underneath the last
// texel (since the paint won't cover it).
//
// The final texel of content may only be partially covered by a
// rasterization; this rect represents the content rect that is fully
// covered by content.
const gfx::Rect layer_content_rect = gfx::Rect(layer_content_size);
gfx::Rect deflated_layer_content_rect = layer_content_rect;
deflated_layer_content_rect.Inset(0, 0, 1, 1);

if (!layer_content_rect.Contains(deflated_layer_content_rect)) {
// Drawing at most 1 x 1 x (canvas width + canvas height) texels is 2-3X
// faster than clearing, so special case this.
DCHECK_LE(paint_rect.right(), layer_content_rect.right());
DCHECK_LE(paint_rect.bottom(), layer_content_rect.bottom());
canvas->save();
canvas->clipRect(gfx::RectToSkRect(layer_content_rect),
SkRegion::kReplace_Op);
canvas->clipRect(gfx::RectToSkRect(deflated_layer_content_rect),
SkRegion::kDifference_Op);
canvas->drawColor(background_color_, SkXfermode::kSrc_Mode);
canvas->restore();
}
}
canvas->translate(SkFloatToScalar(-content_rect.x()),
SkFloatToScalar(-content_rect.y()));

// The |canvas| backing should be sized to hold the |content_rect|.
DCHECK_EQ(content_rect.width(), canvas->getBaseLayerSize().width());
DCHECK_EQ(content_rect.height(), canvas->getBaseLayerSize().height());

gfx::Rect layer_rect;
if (is_scaled) {
gfx::Rect layer_rect = content_rect;
if (contents_width_scale != 1.f || contents_height_scale != 1.f) {
canvas->scale(SkFloatToScalar(contents_width_scale),
SkFloatToScalar(contents_height_scale));

// NOTE: this may go beyond the bounds of the layer, but that shouldn't
// cause problems (anything beyond the layer is clipped out).
layer_rect = gfx::ScaleToEnclosingRect(
paint_rect, 1.f / contents_width_scale, 1.f / contents_height_scale);
} else {
layer_rect = paint_rect;
content_rect, 1.f / contents_width_scale, 1.f / contents_height_scale);
}

SkRect layer_sk_rect = SkRect::MakeXYWH(
Expand All @@ -111,7 +73,7 @@ void ContentLayerUpdater::PaintContents(SkCanvas* canvas,
painter_->Paint(canvas, layer_rect);
canvas->restore();

paint_rect_ = paint_rect;
content_rect_ = content_rect;
}

void ContentLayerUpdater::SetOpaque(bool opaque) {
Expand All @@ -122,8 +84,4 @@ void ContentLayerUpdater::SetFillsBoundsCompletely(bool fills_bounds) {
layer_fills_bounds_completely_ = fills_bounds;
}

void ContentLayerUpdater::SetBackgroundColor(SkColor background_color) {
background_color_ = background_color;
}

} // namespace cc
14 changes: 3 additions & 11 deletions cc/resources/content_layer_updater.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,31 +24,24 @@ class CC_EXPORT ContentLayerUpdater : public LayerUpdater {
void set_rendering_stats_instrumentation(RenderingStatsInstrumentation* rsi);
virtual void SetOpaque(bool opaque) OVERRIDE;
virtual void SetFillsBoundsCompletely(bool fills_bounds) OVERRIDE;
virtual void SetBackgroundColor(SkColor background_color) OVERRIDE;

protected:
ContentLayerUpdater(scoped_ptr<LayerPainter> painter,
RenderingStatsInstrumentation* stats_instrumentation,
int layer_id);
virtual ~ContentLayerUpdater();

// Paints the contents. |content_size| size of the underlying layer in
// layer's content space. |paint_rect| bounds to paint in content space of the
// layer. Both |content_size| and |paint_rect| are in pixels.
void PaintContents(SkCanvas* canvas,
const gfx::Size& layer_content_size,
const gfx::Rect& paint_rect,
const gfx::Rect& content_rect,
float contents_width_scale,
float contents_height_scale);
gfx::Rect paint_rect() const { return paint_rect_; }
gfx::Rect content_rect() const { return content_rect_; }

bool layer_is_opaque() const { return layer_is_opaque_; }
bool layer_fills_bounds_completely() const {
return layer_fills_bounds_completely_;
}

SkColor background_color() const { return background_color_; }

RenderingStatsInstrumentation* rendering_stats_instrumentation_;
int layer_id_;

Expand All @@ -58,9 +51,8 @@ class CC_EXPORT ContentLayerUpdater : public LayerUpdater {
bool layer_fills_bounds_completely_;

private:
gfx::Rect paint_rect_;
gfx::Rect content_rect_;
scoped_ptr<LayerPainter> painter_;
SkColor background_color_;

DISALLOW_COPY_AND_ASSIGN(ContentLayerUpdater);
};
Expand Down
5 changes: 1 addition & 4 deletions cc/resources/layer_updater.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
#include "base/memory/ref_counted.h"
#include "base/memory/scoped_ptr.h"
#include "cc/base/cc_export.h"
#include "third_party/skia/include/core/SkColor.h"
#include "ui/gfx/rect.h"
#include "ui/gfx/vector2d.h"

Expand Down Expand Up @@ -47,8 +46,7 @@ class CC_EXPORT LayerUpdater : public base::RefCounted<LayerUpdater> {

virtual scoped_ptr<Resource> CreateResource(
PrioritizedResourceManager* manager) = 0;
virtual void PrepareToUpdate(const gfx::Size& content_size,
const gfx::Rect& paint_rect,
virtual void PrepareToUpdate(const gfx::Rect& content_rect,
const gfx::Size& tile_size,
float contents_width_scale,
float contents_height_scale) {}
Expand All @@ -60,7 +58,6 @@ class CC_EXPORT LayerUpdater : public base::RefCounted<LayerUpdater> {
// Set true by the layer when it is known that the entire output bounds will
// be rasterized.
virtual void SetFillsBoundsCompletely(bool fills_bounds) {}
virtual void SetBackgroundColor(SkColor background_color) {}

protected:
virtual ~LayerUpdater() {}
Expand Down
Loading

0 comments on commit fcd7452

Please sign in to comment.