Revert of Picture Recording: fix the performance bottleneck in SkDeferredCanvas::isFullFrame (patchset #7 id:140001 of https://codereview.chromium.org/545813002/)
Reason for revert:
This is leaking memory:
http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-ASAN/builds/2516/steps/RunDM/logs/stdio
Original issue's description:
> Picture Recording: fix the performance bottleneck in SkDeferredCanvas::isFullFrame
>
> blink skips all pending commands during picture recording if it is drawing an opaque full-frame
> geometry or image. This may improve performance for some edge cases. To recognize an opaque
> full-frame drawing should be cheap enough. Otherwise, the overhead will offset the improvement.
> Unfortunately, data from perf for content_shell on Nexus7 shows that SkDeferredCanvas::isFullFrame
> is far from cheap. Table below shows that how much isFullFrame() costs in the whole render process.
>
> benchmark percentage
> my local benchmark(draw 1000 sprites) 4.1%
> speedReading 2.8%
> FishIETank(1000 fishes) 1.5%
> GUIMark3 Bitmap 2.0%
>
> By contrast, real recording (SkGPipeCanvas::drawBitmapRectToRect) and real rasterization
> (GrDrawTarget::drawRect) cost ~4% and ~6% in the whole render process respectively. Apparently,
> SkDeferredCanvas::isFullFrame() is nontrivial.
>
> getDeviceSize() is the main contributor to this hotspot. The change simply save the canvasSize and
> reuse it among drawings if it is not a fresh frame. This change cut off ~65% (or improved ~2 times)
> of isFullFrame().
>
> telemetry smoothness canvas_tough_test didn't show obvious improvement or regression.
>
> BUG=411166
>
> Committed: https://skia.googlesource.com/skia/+/8e45c3777d886ba3fe239bb549d06b0693692152
R=junov@chromium.org, tomhudson@google.com, reed@google.com, yunchao.he@intel.com
TBR=junov@chromium.org, reed@google.com, tomhudson@google.com, yunchao.he@intel.com
NOTREECHECKS=true
NOTRY=true
BUG=411166
Author: mtklein@google.com
Review URL: https://codereview.chromium.org/571053002
diff --git a/include/utils/SkDeferredCanvas.h b/include/utils/SkDeferredCanvas.h
index 5f781f8..907cb91 100644
--- a/include/utils/SkDeferredCanvas.h
+++ b/include/utils/SkDeferredCanvas.h
@@ -89,11 +89,6 @@
bool isFreshFrame() const;
/**
- * Returns canvas's size.
- */
- SkISize getCanvasSize() const;
-
- /**
* Returns true if the canvas has recorded draw commands that have
* not yet been played back.
*/
@@ -255,9 +250,6 @@
size_t fBitmapSizeThreshold;
bool fDeferredDrawing;
- mutable SkISize fCachedCanvasSize;
- mutable bool fCachedCanvasSizeDirty;
-
friend class SkDeferredCanvasTester; // for unit testing
typedef SkCanvas INHERITED;
};
diff --git a/src/utils/SkDeferredCanvas.cpp b/src/utils/SkDeferredCanvas.cpp
index cce5dde..b46e92a 100644
--- a/src/utils/SkDeferredCanvas.cpp
+++ b/src/utils/SkDeferredCanvas.cpp
@@ -526,8 +526,6 @@
void SkDeferredCanvas::init() {
fBitmapSizeThreshold = kDeferredCanvasBitmapSizeThreshold;
fDeferredDrawing = true; // On by default
- fCachedCanvasSize.setEmpty();
- fCachedCanvasSizeDirty = true;
}
void SkDeferredCanvas::setMaxRecordingStorage(size_t maxStorage) {
@@ -591,14 +589,6 @@
return this->getDeferredDevice()->isFreshFrame();
}
-SkISize SkDeferredCanvas::getCanvasSize() const {
- if (fCachedCanvasSizeDirty) {
- fCachedCanvasSize = this->getBaseLayerSize();
- fCachedCanvasSizeDirty = false;
- }
- return fCachedCanvasSize;
-}
-
bool SkDeferredCanvas::hasPendingCommands() const {
return this->getDeferredDevice()->hasPendingCommands();
}
@@ -619,7 +609,6 @@
// all pending commands, which can help to seamlessly recover from
// a lost accelerated graphics context.
deferredDevice->setSurface(surface);
- fCachedCanvasSizeDirty = true;
return surface;
}
@@ -643,7 +632,7 @@
bool SkDeferredCanvas::isFullFrame(const SkRect* rect,
const SkPaint* paint) const {
SkCanvas* canvas = this->drawingCanvas();
- SkISize canvasSize = this->getCanvasSize();
+ SkISize canvasSize = this->getDeviceSize();
if (rect) {
if (!canvas->getTotalMatrix().rectStaysRect()) {
return false; // conservative
diff --git a/tests/DeferredCanvasTest.cpp b/tests/DeferredCanvasTest.cpp
index 8945425..61af550 100644
--- a/tests/DeferredCanvasTest.cpp
+++ b/tests/DeferredCanvasTest.cpp
@@ -839,58 +839,6 @@
REPORTER_ASSERT(reporter, notificationCounter.fStorageAllocatedChangedCount == 1);
}
-static void TestDeferredCanvasGetCanvasSize(skiatest::Reporter* reporter) {
- SkRect rect;
- rect.setXYWH(SkIntToScalar(0), SkIntToScalar(0), SkIntToScalar(gWidth), SkIntToScalar(gHeight));
- SkRect clip;
- clip.setXYWH(SkIntToScalar(0), SkIntToScalar(0), SkIntToScalar(1), SkIntToScalar(1));
-
- SkPaint paint;
- SkISize size = SkISize::Make(gWidth, gHeight);
-
- SkAutoTUnref<SkSurface> surface(createSurface(0xFFFFFFFF));
- SkAutoTUnref<SkDeferredCanvas> canvas(SkDeferredCanvas::Create(surface.get()));
-
- for (int i = 0; i < 2; ++i) {
- if (i == 1) {
- SkSurface* newSurface = SkSurface::NewRasterPMColor(4, 4);
- canvas->setSurface(newSurface);
- size = SkISize::Make(4, 4);
- }
-
- // verify that canvas size is correctly initialized or set
- REPORTER_ASSERT(reporter, size == canvas->getCanvasSize());
-
- // Verify that clear, clip and draw the canvas will not change its size
- canvas->clear(0x00000000);
- canvas->clipRect(clip, SkRegion::kIntersect_Op, false);
- canvas->drawRect(rect, paint);
- REPORTER_ASSERT(reporter, size == canvas->getCanvasSize());
-
- // Verify that flush the canvas will not change its size
- canvas->flush();
- REPORTER_ASSERT(reporter, size == canvas->getCanvasSize());
-
- // Verify that clear canvas with saved state will not change its size
- canvas->save();
- canvas->clear(0xFFFFFFFF);
- REPORTER_ASSERT(reporter, size == canvas->getCanvasSize());
-
- // Verify that restore canvas state will not change its size
- canvas->restore();
- REPORTER_ASSERT(reporter, size == canvas->getCanvasSize());
-
- // Verify that clear within a layer will not change canvas size
- canvas->saveLayer(&clip, &paint);
- canvas->clear(0x00000000);
- REPORTER_ASSERT(reporter, size == canvas->getCanvasSize());
-
- // Verify that restore from a layer will not change canvas size
- canvas->restore();
- REPORTER_ASSERT(reporter, size == canvas->getCanvasSize());
- }
-}
-
DEF_TEST(DeferredCanvas_CPU, reporter) {
TestDeferredCanvasFlush(reporter);
TestDeferredCanvasSilentFlush(reporter);
@@ -902,7 +850,6 @@
TestDeferredCanvasBitmapSizeThreshold(reporter);
TestDeferredCanvasCreateCompatibleDevice(reporter);
TestDeferredCanvasWritePixelsToSurface(reporter);
- TestDeferredCanvasGetCanvasSize(reporter);
TestDeferredCanvasSurface(reporter, NULL);
TestDeferredCanvasSetSurface(reporter, NULL);
}
