Skip to content

Commit

Permalink
Revert "cc: Decode discardable images in PaintRecord backed shaders."
Browse files Browse the repository at this point in the history 
This reverts commit 576bda0.

Reason for revert:
Seems to cause LayoutTest failure on CI.

Failing tests are:
* css3/masking/mask-repeat-space-padding.html
* fast/backgrounds/background-repeat-with-background-color.html
* http/tests/misc/slow-loading-image-in-pattern.html
* images/color-profile-background-image-space.html
* images/color-profile-svg-fill-text.html
* virtual/exotic-color-space/images/color-profile-svg-fill-text.html
* virtual/exotic-color-space/images/cross-fade-background-size.html
* virtual/mojo-loading/http/tests/misc/slow-loading-image-in-pattern.html

The error log is available here:
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Mac10.11/builds/23965

Original change's description:
> cc: Decode discardable images in PaintRecord backed shaders.
> 
> For PaintRecord backed shaders, the rasterization is done internally in
> skia, which means images present in these shaders are not pre-decoded
> by compositor's decode cache. This change ensures that we capture these
> images during discardable image analysis, and replace them with decoded
> images from the compositor's cache.
> 
> Replacing of these images requires transforming the PaintRecords in
> these shaders to SkPictures with decoded images before executing the op
> with the shader.
> 
> Bug: 735741, 728359
> Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
> Change-Id: Ie9314e566eaeb1a393c7490e6309d1315ec3733e
> Reviewed-on: https://chromium-review.googlesource.com/673826
> Commit-Queue: Khushal <khushalsagar@chromium.org>
> Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
> Reviewed-by: enne <enne@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#504860}

TBR=vmpstr@chromium.org,chrishtr@chromium.org,enne@chromium.org,khushalsagar@chromium.org

Change-Id: I308e40a4f88c45b25d68ed2a6663d4f83e8b71ee
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 735741, 728359
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/688520
Reviewed-by: Taiju Tsuiki <tzik@chromium.org>
Commit-Queue: Taiju Tsuiki <tzik@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504891}
  • Loading branch information
@tzik
tzik authored and Commit Bot committed Sep 28, 2017
1 parent 3f5043c commit 6c499c0
Show file tree
Hide file tree
Showing 61 changed files with 178 additions and 730 deletions.
2 changes: 0 additions & 2 deletions cc/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -606,8 +606,6 @@ cc_test("cc_unittests") {
"paint/display_item_list_unittest.cc",
"paint/paint_image_unittest.cc",
"paint/paint_op_buffer_unittest.cc",
"paint/paint_shader_unittest.cc",
"paint/scoped_image_flags_unittest.cc",
"paint/solid_color_analyzer_unittest.cc",
"raster/playback_image_provider_unittest.cc",
"raster/raster_buffer_provider_unittest.cc",
Expand Down
2 changes: 0 additions & 2 deletions cc/paint/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,6 @@ cc_component("paint") {
"paint_shader.h",
"record_paint_canvas.cc",
"record_paint_canvas.h",
"scoped_image_flags.cc",
"scoped_image_flags.h",
"skia_paint_canvas.cc",
"skia_paint_canvas.h",
"skia_paint_image_generator.cc",
Expand Down
238 changes: 86 additions & 152 deletions cc/paint/discardable_image_map.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@ SkRect MapRect(const SkMatrix& matrix, const SkRect& src) {
return dst;
}

// This canvas is used only for tracking transform/clip/filter state from the
// non-drawing ops.
class PaintTrackingCanvas final : public SkNoDrawCanvas {
public:
PaintTrackingCanvas(int width, int height) : SkNoDrawCanvas(width, height) {}
Expand Down Expand Up @@ -77,14 +75,52 @@ class PaintTrackingCanvas final : public SkNoDrawCanvas {

class DiscardableImageGenerator {
public:
DiscardableImageGenerator(int width,
int height,
const PaintOpBuffer* buffer) {
PaintTrackingCanvas canvas(width, height);
GatherDiscardableImages(buffer, nullptr, &canvas);
}
DiscardableImageGenerator(int width, int height) : canvas_(width, height) {}
~DiscardableImageGenerator() = default;

void GatherDiscardableImages(const PaintOpBuffer* buffer) {
if (!buffer->HasDiscardableImages())
return;

PlaybackParams params(nullptr, canvas_.getTotalMatrix());
canvas_.save();
// TODO(khushalsagar): Optimize out save/restore blocks if there are no
// images in the draw ops between them.
for (auto* op : PaintOpBuffer::Iterator(buffer)) {
if (op->IsDrawOp()) {
SkRect op_rect;
if (op->IsPaintOpWithFlags() && PaintOp::GetBounds(op, &op_rect)) {
AddImageFromFlags(op_rect,
static_cast<const PaintOpWithFlags*>(op)->flags);
}

PaintOpType op_type = static_cast<PaintOpType>(op->type);
if (op_type == PaintOpType::DrawImage) {
auto* image_op = static_cast<DrawImageOp*>(op);
auto* sk_image = image_op->image.GetSkImage().get();
AddImage(image_op->image,
SkRect::MakeIWH(sk_image->width(), sk_image->height()),
SkRect::MakeXYWH(image_op->left, image_op->top,
sk_image->width(), sk_image->height()),
nullptr, image_op->flags);
} else if (op_type == PaintOpType::DrawImageRect) {
auto* image_rect_op = static_cast<DrawImageRectOp*>(op);
SkMatrix matrix;
matrix.setRectToRect(image_rect_op->src, image_rect_op->dst,
SkMatrix::kFill_ScaleToFit);
AddImage(image_rect_op->image, image_rect_op->src, image_rect_op->dst,
&matrix, image_rect_op->flags);
} else if (op_type == PaintOpType::DrawRecord) {
GatherDiscardableImages(
static_cast<const DrawRecordOp*>(op)->record.get());
}
} else {
op->Raster(&canvas_, params);
}
}
canvas_.restore();
}

std::vector<std::pair<DrawImage, gfx::Rect>> TakeImages() {
return std::move(image_set_);
}
Expand Down Expand Up @@ -117,111 +153,47 @@ class DiscardableImageGenerator {
}

private:
// Adds discardable images from |buffer| to the set of images tracked by
// this generator. If |buffer| is being used in a DrawOp that requires
// rasterization of the buffer as a pre-processing step for execution of the
// op (for instance, with PaintRecord backed PaintShaders),
// |top_level_op_rect| is set to the rect for that op. If provided, the
// |top_level_op_rect| will be used as the rect for tracking the position of
// this image in the top-level buffer.
void GatherDiscardableImages(const PaintOpBuffer* buffer,
const gfx::Rect* top_level_op_rect,
PaintTrackingCanvas* canvas) {
if (!buffer->HasDiscardableImages())
void AddImageFromFlags(const SkRect& rect, const PaintFlags& flags) {
if (!flags.HasShader() ||
flags.getShader()->shader_type() != PaintShader::Type::kImage)
return;

// Prevent PaintOpBuffers from having side effects back into the canvas.
SkAutoCanvasRestore save_restore(canvas, true);

PlaybackParams params(nullptr, canvas->getTotalMatrix());
// TODO(khushalsagar): Optimize out save/restore blocks if there are no
// images in the draw ops between them.
for (auto* op : PaintOpBuffer::Iterator(buffer)) {
if (!op->IsDrawOp()) {
op->Raster(canvas, params);
continue;
} else if (!PaintOp::OpHasDiscardableImages(op)) {
continue;
}

gfx::Rect op_rect;
base::Optional<gfx::Rect> local_op_rect;

if (top_level_op_rect) {
op_rect = *top_level_op_rect;
} else {
local_op_rect = ComputePaintRect(op, canvas);
if (local_op_rect.value().IsEmpty())
continue;
const PaintImage& paint_image = flags.getShader()->paint_image();
SkMatrix local_matrix = flags.getShader()->GetLocalMatrix();
AddImage(paint_image,
SkRect::MakeWH(paint_image.width(), paint_image.height()), rect,
&local_matrix, flags);
}

op_rect = local_op_rect.value();
}
void AddImage(PaintImage paint_image,
const SkRect& src_rect,
const SkRect& rect,
const SkMatrix* local_matrix,
const PaintFlags& flags) {
if (!paint_image.IsLazyGenerated())
return;

const SkMatrix& ctm = canvas->getTotalMatrix();
if (op->IsPaintOpWithFlags()) {
AddImageFromFlags(op_rect,
static_cast<const PaintOpWithFlags*>(op)->flags, ctm);
}
const SkRect& clip_rect = SkRect::Make(canvas_.getDeviceClipBounds());
const SkMatrix& ctm = canvas_.getTotalMatrix();

PaintOpType op_type = static_cast<PaintOpType>(op->type);
if (op_type == PaintOpType::DrawImage) {
auto* image_op = static_cast<DrawImageOp*>(op);
auto* sk_image = image_op->image.GetSkImage().get();
AddImage(image_op->image,
SkRect::MakeIWH(sk_image->width(), sk_image->height()),
op_rect, ctm, image_op->flags.getFilterQuality());
} else if (op_type == PaintOpType::DrawImageRect) {
auto* image_rect_op = static_cast<DrawImageRectOp*>(op);
SkMatrix matrix = ctm;
matrix.postConcat(SkMatrix::MakeRectToRect(image_rect_op->src,
image_rect_op->dst,
SkMatrix::kFill_ScaleToFit));
AddImage(image_rect_op->image, image_rect_op->src, op_rect, matrix,
image_rect_op->flags.getFilterQuality());
} else if (op_type == PaintOpType::DrawRecord) {
GatherDiscardableImages(
static_cast<const DrawRecordOp*>(op)->record.get(),
top_level_op_rect, canvas);
}
SkRect paint_rect = MapRect(ctm, rect);
SkPaint paint = flags.ToSkPaint();
bool computed_paint_bounds =
canvas_.ComputePaintBounds(paint_rect, &paint, &paint_rect);
if (!computed_paint_bounds) {
// TODO(vmpstr): UMA this case.
paint_rect = clip_rect;
}
}

// Given the |op_rect|, which is the rect for the draw op, returns the
// transformed rect accounting for the current transform, clip and paint
// state on |canvas_|.
gfx::Rect ComputePaintRect(const PaintOp* op, PaintTrackingCanvas* canvas) {
const SkRect& clip_rect = SkRect::Make(canvas->getDeviceClipBounds());
const SkMatrix& ctm = canvas->getTotalMatrix();

gfx::Rect transformed_rect;
SkRect op_rect;
if (!PaintOp::GetBounds(op, &op_rect)) {
// If we can't provide a conservative bounding rect for the op, assume it
// covers the complete current clip.
transformed_rect = gfx::ToEnclosingRect(gfx::SkRectToRectF(clip_rect));
} else {
const PaintFlags* flags =
op->IsPaintOpWithFlags()
? &static_cast<const PaintOpWithFlags*>(op)->flags
: nullptr;
SkPaint paint;
if (flags)
paint = flags->ToSkPaint();

SkRect paint_rect = MapRect(ctm, op_rect);
bool computed_paint_bounds =
canvas->ComputePaintBounds(paint_rect, &paint, &paint_rect);
if (!computed_paint_bounds) {
// TODO(vmpstr): UMA this case.
paint_rect = clip_rect;
}
// Clamp the image rect by the current clip rect.
if (!paint_rect.intersect(clip_rect))
return;

// Clamp the image rect by the current clip rect.
if (!paint_rect.intersect(clip_rect))
return gfx::Rect();
SkFilterQuality filter_quality = flags.getFilterQuality();

transformed_rect = gfx::ToEnclosingRect(gfx::SkRectToRectF(paint_rect));
}
SkIRect src_irect;
src_rect.roundOut(&src_irect);
gfx::Rect image_rect = gfx::ToEnclosingRect(gfx::SkRectToRectF(paint_rect));

// During raster, we use the device clip bounds on the canvas, which outsets
// the actual clip by 1 due to the possibility of antialiasing. Account for
Expand All @@ -233,52 +205,7 @@ class DiscardableImageGenerator {
// raster time, since we might be sending a larger-than-one-item display
// item to skia, which means that skia will internally determine whether to
// raster the picture (using device clip bounds that are outset).
transformed_rect.Inset(-1, -1);
return transformed_rect;
}

void AddImageFromFlags(const gfx::Rect& op_rect,
const PaintFlags& flags,
const SkMatrix& ctm) {
if (!flags.getShader())
return;

if (flags.getShader()->shader_type() == PaintShader::Type::kImage) {
const PaintImage& paint_image = flags.getShader()->paint_image();
SkMatrix matrix = ctm;
matrix.postConcat(flags.getShader()->GetLocalMatrix());
AddImage(paint_image,
SkRect::MakeWH(paint_image.width(), paint_image.height()),
op_rect, matrix, flags.getFilterQuality());
} else if (flags.getShader()->shader_type() ==
PaintShader::Type::kPaintRecord &&
flags.getShader()->paint_record()->HasDiscardableImages()) {
SkRect scaled_tile_rect;
if (!flags.getShader()->GetRasterizationTileRect(ctm,
&scaled_tile_rect)) {
return;
}

PaintTrackingCanvas canvas(scaled_tile_rect.width(),
scaled_tile_rect.height());
canvas.setMatrix(SkMatrix::MakeRectToRect(flags.getShader()->tile(),
scaled_tile_rect,
SkMatrix::kFill_ScaleToFit));
GatherDiscardableImages(flags.getShader()->paint_record().get(), &op_rect,
&canvas);
}
}

void AddImage(PaintImage paint_image,
const SkRect& src_rect,
const gfx::Rect& image_rect,
const SkMatrix& matrix,
SkFilterQuality filter_quality) {
if (!paint_image.IsLazyGenerated())
return;

SkIRect src_irect;
src_rect.roundOut(&src_irect);
image_rect.Inset(-1, -1);

// Make a note if any image was originally specified in a non-sRGB color
// space.
Expand All @@ -290,6 +217,10 @@ class DiscardableImageGenerator {
color_stats_srgb_image_count_++;
}

SkMatrix matrix = ctm;
if (local_matrix)
matrix.postConcat(*local_matrix);

image_id_to_rect_[paint_image.stable_id()].Union(image_rect);

if (paint_image.ShouldAnimate()) {
Expand All @@ -304,6 +235,9 @@ class DiscardableImageGenerator {
image_rect);
}

// This canvas is used only for tracking transform/clip/filter state from the
// non-drawing ops.
PaintTrackingCanvas canvas_;
std::vector<std::pair<DrawImage, gfx::Rect>> image_set_;
base::flat_map<PaintImage::Id, gfx::Rect> image_id_to_rect_;
std::vector<DiscardableImageMap::AnimatedImageMetadata>
Expand All @@ -329,8 +263,8 @@ void DiscardableImageMap::Generate(const PaintOpBuffer* paint_op_buffer,
if (!paint_op_buffer->HasDiscardableImages())
return;

DiscardableImageGenerator generator(bounds.right(), bounds.bottom(),
paint_op_buffer);
DiscardableImageGenerator generator(bounds.right(), bounds.bottom());
generator.GatherDiscardableImages(paint_op_buffer);
generator.RecordColorHistograms();
image_id_to_rect_ = generator.TakeImageIdToRectMap();
animated_images_metadata_ = generator.TakeAnimatedImagesMetadata();
Expand Down
37 changes: 0 additions & 37 deletions cc/paint/discardable_image_map_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -747,43 +747,6 @@ TEST_F(DiscardableImageMapTest, GathersAnimatedImages) {
EXPECT_DCHECK_DEATH(images[2]->frame_index());
}

TEST_F(DiscardableImageMapTest, CapturesImagesInPaintRecordShaders) {
// Create the record to use in the shader.
auto shader_record = sk_make_sp<PaintOpBuffer>();
shader_record->push<ScaleOp>(2.0f, 2.0f);
PaintImage paint_image = CreateDiscardablePaintImage(gfx::Size(100, 100));
shader_record->push<DrawImageOp>(paint_image, 0.f, 0.f, nullptr);

gfx::Rect visible_rect(500, 500);
scoped_refptr<DisplayItemList> display_list = new DisplayItemList();
display_list->StartPaint();
display_list->push<ScaleOp>(2.0f, 2.0f);
PaintFlags flags;
SkRect tile = SkRect::MakeWH(100, 100);
flags.setShader(PaintShader::MakePaintRecord(
shader_record, tile, SkShader::TileMode::kClamp_TileMode,
SkShader::TileMode::kClamp_TileMode, nullptr));
display_list->push<DrawRectOp>(SkRect::MakeWH(200, 200), flags);
display_list->EndPaintOfUnpaired(visible_rect);
display_list->Finalize();

display_list->GenerateDiscardableImagesMetadata();
const auto& image_map = display_list->discardable_image_map();

// The image rect is set to the rect for the DrawRectOp.
std::vector<PositionScaleDrawImage> draw_images =
GetDiscardableImagesInRect(image_map, visible_rect);
std::vector<gfx::Rect> inset_rects = InsetImageRects(draw_images);
ASSERT_EQ(draw_images.size(), 1u);
EXPECT_EQ(draw_images[0].image, paint_image);
// The position of the image is the position of the DrawRectOp that uses the
// shader.
EXPECT_EQ(gfx::Rect(400, 400), inset_rects[0]);
// The scale of the image includes the scale at which the shader record is
// rasterized.
EXPECT_EQ(SkSize::Make(4.f, 4.f), draw_images[0].scale);
}

class DiscardableImageMapColorSpaceTest
: public DiscardableImageMapTest,
public testing::WithParamInterface<gfx::ColorSpace> {};
Expand Down
Loading

0 comments on commit 6c499c0

Please sign in to comment.