Skip to content

Commit

Permalink
override new SkCanvas virtuals for transforms
Browse files Browse the repository at this point in the history
- override new 4x4 virtual
- override scale/translate virtuals

In the new *brave* world, SkCanvas will call didTranslate() and didScale() in
response to the corresponding public calls. On the impl side, SkCanvas
no longer has a default impl for either, so they must be overridden.

Having to update multiple unittests might point to a slight problem with layering
and testing. It appears (to me) that most of the sites I had to update were
incidental, not core, to the test. The caller is baking in assumptions about how
Skia handles some high-level api calls (and what virtuals it may call). If we
could find a different way to still test the chrome/blink code in question, it
will be easier going forward.

bug: skia:9768

Change-Id: I8aa62feb354392105229ea45686b8208ba0e1450
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1998247
Commit-Queue: Mike Reed <reed@google.com>
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Reviewed-by: vmpstr <vmpstr@chromium.org>
Reviewed-by: Florin Malita <fmalita@chromium.org>
Cr-Commit-Position: refs/heads/master@{#731734}
  • Loading branch information
reed-at-google authored and Commit Bot committed Jan 14, 2020
1 parent 4112fcf commit 32a1af6
Show file tree
Hide file tree
Showing 10 changed files with 89 additions and 25 deletions.
24 changes: 11 additions & 13 deletions cc/paint/paint_op_buffer_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2833,13 +2833,6 @@ MATCHER(NonLazyImage, "") {
return !arg->isLazyGenerated();
}

MATCHER_P(MatchesInvScale, expected, "") {
SkSize scale;
arg.decomposeScale(&scale, nullptr);
SkSize inv = SkSize::Make(1.0f / scale.width(), 1.0f / scale.height());
return inv == expected;
}

MATCHER_P2(MatchesRect, rect, scale, "") {
EXPECT_EQ(arg->x(), rect.x() * scale.width());
EXPECT_EQ(arg->y(), rect.y() * scale.height());
Expand Down Expand Up @@ -2899,7 +2892,7 @@ TEST(PaintOpBufferTest, RasterPaintWorkletImageRectBasicCase) {
EXPECT_CALL(canvas, willSave()).InSequence(s);
EXPECT_CALL(canvas, OnSaveLayer()).InSequence(s);
EXPECT_CALL(canvas, willSave()).InSequence(s);
EXPECT_CALL(canvas, didConcat(SkMatrix::MakeTrans(8.0f, 8.0f)));
EXPECT_CALL(canvas, didTranslate(8.0f, 8.0f));
EXPECT_CALL(canvas, OnSaveLayer()).InSequence(s);
EXPECT_CALL(canvas, OnDrawRectWithColor(0u));
EXPECT_CALL(canvas, willRestore()).InSequence(s);
Expand Down Expand Up @@ -2943,7 +2936,8 @@ TEST(PaintOpBufferTest, RasterPaintWorkletImageRectTranslated) {
EXPECT_CALL(canvas, OnSaveLayer()).InSequence(s);
EXPECT_CALL(canvas, didConcat(SkMatrix::MakeTrans(5.0f, 7.0f)));
EXPECT_CALL(canvas, willSave()).InSequence(s);
EXPECT_CALL(canvas, didConcat(MatchesInvScale(scale_adjustment[0])));
EXPECT_CALL(canvas, didScale(1.0f / scale_adjustment[0].width(),
1.0f / scale_adjustment[0].height()));
EXPECT_CALL(canvas, onDrawImage(NonLazyImage(), 0.0f, 0.0f,
MatchesQuality(quality[0])));
EXPECT_CALL(canvas, willRestore()).InSequence(s);
Expand Down Expand Up @@ -2987,7 +2981,8 @@ TEST(PaintOpBufferTest, RasterPaintWorkletImageRectScaled) {
EXPECT_CALL(canvas, OnSaveLayer()).InSequence(s);
EXPECT_CALL(canvas, didConcat(SkMatrix::MakeScale(2.f, 1.5f)));
EXPECT_CALL(canvas, willSave()).InSequence(s);
EXPECT_CALL(canvas, didConcat(MatchesInvScale(scale_adjustment[0])));
EXPECT_CALL(canvas, didScale(1.0f / scale_adjustment[0].width(),
1.0f / scale_adjustment[0].height()));
EXPECT_CALL(canvas, onDrawImage(NonLazyImage(), 0.0f, 0.0f,
MatchesQuality(quality[0])));
EXPECT_CALL(canvas, willRestore()).InSequence(s);
Expand Down Expand Up @@ -3033,7 +3028,8 @@ TEST(PaintOpBufferTest, RasterPaintWorkletImageRectClipped) {
EXPECT_CALL(canvas, OnSaveLayer()).InSequence(s);
EXPECT_CALL(canvas, OnSaveLayer()).InSequence(s);
EXPECT_CALL(canvas, willSave()).InSequence(s);
EXPECT_CALL(canvas, didConcat(MatchesInvScale(scale_adjustment[0])));
EXPECT_CALL(canvas, didScale(1.0f / scale_adjustment[0].width(),
1.0f / scale_adjustment[0].height()));
EXPECT_CALL(canvas, onDrawImage(NonLazyImage(), 0.0f, 0.0f,
MatchesQuality(quality[0])));
EXPECT_CALL(canvas, willRestore()).InSequence(s);
Expand Down Expand Up @@ -3073,7 +3069,8 @@ TEST(PaintOpBufferTest, ReplacesImagesFromProvider) {

// Save/scale/image/restore from DrawImageop.
EXPECT_CALL(canvas, willSave()).InSequence(s);
EXPECT_CALL(canvas, didConcat(MatchesInvScale(scale_adjustment[0])));
EXPECT_CALL(canvas, didScale(1.0f / scale_adjustment[0].width(),
1.0f / scale_adjustment[0].height()));
EXPECT_CALL(canvas, onDrawImage(NonLazyImage(), 0.0f, 0.0f,
MatchesQuality(quality[0])));
EXPECT_CALL(canvas, willRestore()).InSequence(s);
Expand Down Expand Up @@ -3140,7 +3137,8 @@ TEST(PaintOpBufferTest, ReplacesImagesFromProviderOOP) {
if (op->GetType() == PaintOpType::DrawImage) {
// Save/scale/image/restore from DrawImageop.
EXPECT_CALL(canvas, willSave()).InSequence(s);
EXPECT_CALL(canvas, didConcat(MatchesInvScale(expected_scale)));
EXPECT_CALL(canvas, didScale(1.0f / expected_scale.width(),
1.0f / expected_scale.height()));
EXPECT_CALL(canvas, onDrawImage(NonLazyImage(), 0.0f, 0.0f, _));
EXPECT_CALL(canvas, willRestore()).InSequence(s);
op->Raster(&canvas, params);
Expand Down
5 changes: 2 additions & 3 deletions cc/raster/raster_source_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -559,12 +559,11 @@ TEST(RasterSourceTest, RasterTransformWithoutRecordingScale) {
StrictMock<MockCanvas> mock_canvas;
Sequence s;

SkMatrix m;
m.setScale(1.f / recording_scale, 1.f / recording_scale);
SkScalar scale = 1.f / recording_scale;

// The recording source has no ops, so will only do the setup.
EXPECT_CALL(mock_canvas, willSave()).InSequence(s);
EXPECT_CALL(mock_canvas, didConcat(m)).InSequence(s);
EXPECT_CALL(mock_canvas, didScale(scale, scale)).InSequence(s);
EXPECT_CALL(mock_canvas, willRestore()).InSequence(s);

gfx::Size small_size(50, 50);
Expand Down
3 changes: 3 additions & 0 deletions cc/test/test_skcanvas.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,10 @@ class MockCanvas : public SkNoDrawCanvas {
SrcRectConstraint));
MOCK_METHOD5(onDrawArc,
void(const SkRect&, SkScalar, SkScalar, bool, const SkPaint&));
MOCK_METHOD1(didConcat44, void(const SkScalar[16]));
MOCK_METHOD1(didConcat, void(const SkMatrix&));
MOCK_METHOD2(didScale, void(SkScalar, SkScalar));
MOCK_METHOD2(didTranslate, void(SkScalar, SkScalar));
MOCK_METHOD2(onDrawOval, void(const SkRect&, const SkPaint&));
MOCK_METHOD2(onCustomCallback, void(SkCanvas*, uint32_t));
MOCK_METHOD0(onFlush, void());
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/vr/elements/vector_icon_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ TEST(VectorIcon, SmokeTest) {
EXPECT_CALL(canvas, willSave());

// This matrix is concatenated to apply to the vector icon.
EXPECT_CALL(canvas, didConcat(_));
EXPECT_CALL(canvas, didScale(_, _));

// This is the call to draw the path comprising the vector icon.
EXPECT_CALL(canvas, onDrawPath(_, _));
Expand Down
4 changes: 0 additions & 4 deletions skia/config/SkUserConfig.h
Original file line number Diff line number Diff line change
Expand Up @@ -215,10 +215,6 @@ SK_API void SkDebugf_FileLine(const char* file,
#define SK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS
#endif

#ifndef SK_SUPPORT_LEGACY_CANVAS_MATRIX_VIRTUALS
#define SK_SUPPORT_LEGACY_CANVAS_MATRIX_VIRTUALS
#endif

// Max. verb count for paths rendered by the edge-AA tessellating path renderer.
#define GR_AA_TESSELLATOR_MAX_VERB_COUNT 100

Expand Down
23 changes: 23 additions & 0 deletions skia/ext/benchmarking_canvas.cc
Original file line number Diff line number Diff line change
Expand Up @@ -435,13 +435,36 @@ void BenchmarkingCanvas::willRestore() {
INHERITED::willRestore();
}

void BenchmarkingCanvas::didConcat44(const SkScalar m[16]) {
AutoOp op(this, "Concat44");
op.addParam("column-major", AsListValue(m, 16));

INHERITED::didConcat44(m);
}

void BenchmarkingCanvas::didConcat(const SkMatrix& m) {
AutoOp op(this, "Concat");
op.addParam("matrix", AsValue(m));

INHERITED::didConcat(m);
}

void BenchmarkingCanvas::didScale(SkScalar x, SkScalar y) {
AutoOp op(this, "Scale");
op.addParam("scale-x", AsValue(x));
op.addParam("scale-y", AsValue(y));

INHERITED::didScale(x, y);
}

void BenchmarkingCanvas::didTranslate(SkScalar x, SkScalar y) {
AutoOp op(this, "Translate");
op.addParam("translate-x", AsValue(x));
op.addParam("translate-y", AsValue(y));

INHERITED::didTranslate(x, y);
}

void BenchmarkingCanvas::didSetMatrix(const SkMatrix& m) {
AutoOp op(this, "SetMatrix");
op.addParam("matrix", AsValue(m));
Expand Down
3 changes: 3 additions & 0 deletions skia/ext/benchmarking_canvas.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,10 @@ class SK_API BenchmarkingCanvas : public SkNWayCanvas {
SaveLayerStrategy getSaveLayerStrategy(const SaveLayerRec&) override;
void willRestore() override;

void didConcat44(const SkScalar[16]) override;
void didConcat(const SkMatrix&) override;
void didScale(SkScalar, SkScalar) override;
void didTranslate(SkScalar, SkScalar) override;
void didSetMatrix(const SkMatrix&) override;

void onClipRect(const SkRect&, SkClipOp, ClipEdgeStyle) override;
Expand Down
17 changes: 15 additions & 2 deletions third_party/blink/renderer/platform/graphics/intercepting_canvas.h
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,10 @@ class InterceptingCanvasBase : public SkCanvas {
const SkMatrix*,
const SkPaint*) override = 0;
void didSetMatrix(const SkMatrix&) override = 0;
void didConcat44(const SkScalar[16]) override = 0;
void didConcat(const SkMatrix&) override = 0;
void didScale(SkScalar, SkScalar) override = 0;
void didTranslate(SkScalar, SkScalar) override = 0;
void willSave() override = 0;
SaveLayerStrategy getSaveLayerStrategy(const SaveLayerRec&) override = 0;
void willRestore() override = 0;
Expand Down Expand Up @@ -285,12 +288,22 @@ class InterceptingCanvas : public InterceptingCanvasBase {

void didSetMatrix(const SkMatrix& matrix) override {
Interceptor interceptor(this);
this->SkCanvas::didSetMatrix(matrix);
}

void didConcat44(const SkScalar m[16]) override {
Interceptor interceptor(this);
}

void didConcat(const SkMatrix& matrix) override {
Interceptor interceptor(this);
this->SkCanvas::didConcat(matrix);
}

void didScale(SkScalar x, SkScalar y) override {
Interceptor interceptor(this);
}

void didTranslate(SkScalar x, SkScalar y) override {
Interceptor interceptor(this);
}

void willSave() override {
Expand Down
30 changes: 28 additions & 2 deletions third_party/blink/renderer/platform/graphics/logging_canvas.cc
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,14 @@ std::unique_ptr<JSONArray> ArrayForSkMatrix(const SkMatrix& matrix) {
return matrix_array;
}

std::unique_ptr<JSONArray> ArrayForSkScalars(size_t count,
const SkScalar array[]) {
auto points_array_item = std::make_unique<JSONArray>();
for (size_t i = 0; i < count; ++i)
points_array_item->PushDouble(array[i]);
return points_array_item;
}

std::unique_ptr<JSONObject> ObjectForSkShader(const SkShader& shader) {
return std::make_unique<JSONObject>();
}
Expand Down Expand Up @@ -677,7 +685,12 @@ void LoggingCanvas::didSetMatrix(const SkMatrix& matrix) {
AutoLogger logger(this);
JSONObject* params = logger.LogItemWithParams("setMatrix");
params->SetArray("matrix", ArrayForSkMatrix(matrix));
this->SkCanvas::didSetMatrix(matrix);
}

void LoggingCanvas::didConcat44(const SkScalar m[16]) {
AutoLogger logger(this);
JSONObject* params = logger.LogItemWithParams("concat44");
params->SetArray("matrix44", ArrayForSkScalars(16, m));
}

void LoggingCanvas::didConcat(const SkMatrix& matrix) {
Expand All @@ -701,7 +714,20 @@ void LoggingCanvas::didConcat(const SkMatrix& matrix) {
params = logger.LogItemWithParams("concat");
params->SetArray("matrix", ArrayForSkMatrix(matrix));
}
this->SkCanvas::didConcat(matrix);
}

void LoggingCanvas::didScale(SkScalar x, SkScalar y) {
AutoLogger logger(this);
JSONObject* params = logger.LogItemWithParams("scale");
params->SetDouble("scaleX", x);
params->SetDouble("scaleY", y);
}

void LoggingCanvas::didTranslate(SkScalar x, SkScalar y) {
AutoLogger logger(this);
JSONObject* params = logger.LogItemWithParams("translate");
params->SetDouble("dx", x);
params->SetDouble("dy", y);
}

void LoggingCanvas::willSave() {
Expand Down
3 changes: 3 additions & 0 deletions third_party/blink/renderer/platform/graphics/logging_canvas.h
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,10 @@ class LoggingCanvas : public InterceptingCanvasBase {
const SkMatrix*,
const SkPaint*) override;
void didSetMatrix(const SkMatrix&) override;
void didConcat44(const SkScalar[16]) override;
void didConcat(const SkMatrix&) override;
void didScale(SkScalar, SkScalar) override;
void didTranslate(SkScalar, SkScalar) override;
void willSave() override;
SaveLayerStrategy getSaveLayerStrategy(const SaveLayerRec&) override;
void willRestore() override;
Expand Down

0 comments on commit 32a1af6

Please sign in to comment.