From 653d51867c20fc643537e4a0d73178697766ae4a Mon Sep 17 00:00:00 2001 From: "commit-bot@chromium.org" Date: Tue, 15 Apr 2014 14:27:14 +0000 Subject: SkRecord bug fixes Optional arguments to SkCanvas calls leaked refs (but not memory) because we didn't destruct the optional objects (really, just SkPaint: other optional args are all POD). This adds Optional and PODArray, where Optional makes sure to call the destructor. I overlooked that SkPictureRecord really does call paint.computeFastBounds(). Do the same in SkRecordDraw. BUG=skia:2378 R=reed@google.com, mtklein@google.com, tomhudson@google.com Author: mtklein@chromium.org Review URL: https://codereview.chromium.org/235983015 git-svn-id: http://skia.googlecode.com/svn/trunk@14197 2bbb7eff-a529-9590-31e7-b0007b416f81 --- src/record/SkRecordDraw.cpp | 10 +++++-- src/record/SkRecords.h | 73 ++++++++++++++++++++++++++++++++++----------- tests/RecorderTest.cpp | 22 ++++++++++++++ 3 files changed, 85 insertions(+), 20 deletions(-) diff --git a/src/record/SkRecordDraw.cpp b/src/record/SkRecordDraw.cpp index f0d3772e9f..c6e85502a7 100644 --- a/src/record/SkRecordDraw.cpp +++ b/src/record/SkRecordDraw.cpp @@ -76,7 +76,7 @@ CAN_SKIP(ClipRegion); static bool can_skip_text(const SkCanvas& c, const SkPaint& p, SkScalar minY, SkScalar maxY) { // If we're drawing vertical text, none of the checks we're about to do make any sense. - // We use canComputeFastBounds as a proxy for "is this text going to be rectangular?". + // We'll need to call SkPaint::computeFastBounds() later, so bail out if that's not possible. if (p.isVerticalText() || !p.canComputeFastBounds()) { return false; } @@ -88,7 +88,13 @@ static bool can_skip_text(const SkCanvas& c, const SkPaint& p, SkScalar minY, Sk SkDEBUGCODE(p.getFontMetrics(&metrics);) SkASSERT(-buffer <= metrics.fTop); SkASSERT(+buffer >= metrics.fBottom); - return c.quickRejectY(minY - buffer, maxY + buffer); + + // Let the paint adjust the text bounds. We don't care about left and right here, so we use + // 0 and 1 respectively just so the bounds rectangle isn't empty. + SkRect bounds; + bounds.set(0, -buffer, SK_Scalar1, buffer); + SkRect adjusted = p.computeFastBounds(bounds, &bounds); + return c.quickRejectY(minY + adjusted.fTop, maxY + adjusted.fBottom); } template <> bool Draw::canSkip(const SkRecords::DrawPosTextH& r) const { diff --git a/src/record/SkRecords.h b/src/record/SkRecords.h index 7958280e48..10806565d9 100644 --- a/src/record/SkRecords.h +++ b/src/record/SkRecords.h @@ -107,6 +107,31 @@ struct T { \ A a; B b; C c; D d; E e; \ }; +// An Optional doesn't own the pointer's memory, but may need to destroy non-POD data. +template +class Optional : SkNoncopyable { +public: + Optional(T* ptr) : fPtr(ptr) {} + ~Optional() { if (fPtr) fPtr->~T(); } + + operator T*() { return fPtr; } + operator const T*() const { return fPtr; } +private: + T* fPtr; +}; + +// PODArray doesn't own the pointer's memory, and we assume the data is POD. +template +class PODArray : SkNoncopyable { +public: + PODArray(T* ptr) : fPtr(ptr) {} + + operator T*() { return fPtr; } + operator const T*() const { return fPtr; } +private: + T* fPtr; +}; + // Like SkBitmap, but deep copies pixels if they're not immutable. // Using this, we guarantee the immutability of all bitmaps we record. class ImmutableBitmap { @@ -126,13 +151,12 @@ private: SkBitmap fBitmap; }; -// Pointers here represent either an optional value or an array if accompanied by a count. // None of these records manages the lifetimes of pointers, except for DrawVertices handling its // Xfermode specially. RECORD0(Restore); RECORD1(Save, SkCanvas::SaveFlags, flags); -RECORD3(SaveLayer, SkRect*, bounds, SkPaint*, paint, SkCanvas::SaveFlags, flags); +RECORD3(SaveLayer, Optional, bounds, Optional, paint, SkCanvas::SaveFlags, flags); static const unsigned kUnsetPopOffset = 0; RECORD2(PushCull, SkRect, rect, unsigned, popOffset); @@ -147,33 +171,46 @@ RECORD3(ClipRect, SkRect, rect, SkRegion::Op, op, bool, doAA); RECORD2(ClipRegion, SkRegion, region, SkRegion::Op, op); RECORD1(Clear, SkColor, color); -RECORD4(DrawBitmap, ImmutableBitmap, bitmap, SkScalar, left, SkScalar, top, SkPaint*, paint); -RECORD3(DrawBitmapMatrix, ImmutableBitmap, bitmap, SkMatrix, matrix, SkPaint*, paint); -RECORD4(DrawBitmapNine, ImmutableBitmap, bitmap, SkIRect, center, SkRect, dst, SkPaint*, paint); +RECORD4(DrawBitmap, ImmutableBitmap, bitmap, + SkScalar, left, + SkScalar, top, + Optional, paint); +RECORD3(DrawBitmapMatrix, ImmutableBitmap, bitmap, SkMatrix, matrix, Optional, paint); +RECORD4(DrawBitmapNine, ImmutableBitmap, bitmap, + SkIRect, center, + SkRect, dst, + Optional, paint); RECORD5(DrawBitmapRectToRect, ImmutableBitmap, bitmap, - SkRect*, src, + Optional, src, SkRect, dst, - SkPaint*, paint, + Optional, paint, SkCanvas::DrawBitmapRectFlags, flags); RECORD3(DrawDRRect, SkRRect, outer, SkRRect, inner, SkPaint, paint); RECORD2(DrawOval, SkRect, oval, SkPaint, paint); RECORD1(DrawPaint, SkPaint, paint); RECORD2(DrawPath, SkPath, path, SkPaint, paint); RECORD4(DrawPoints, SkCanvas::PointMode, mode, size_t, count, SkPoint*, pts, SkPaint, paint); -RECORD4(DrawPosText, char*, text, size_t, byteLength, SkPoint*, pos, SkPaint, paint); -RECORD5(DrawPosTextH, char*, text, +RECORD4(DrawPosText, PODArray, text, + size_t, byteLength, + PODArray, pos, + SkPaint, paint); +RECORD5(DrawPosTextH, PODArray, text, size_t, byteLength, - SkScalar*, xpos, + PODArray, xpos, SkScalar, y, SkPaint, paint); RECORD2(DrawRRect, SkRRect, rrect, SkPaint, paint); RECORD2(DrawRect, SkRect, rect, SkPaint, paint); -RECORD4(DrawSprite, ImmutableBitmap, bitmap, int, left, int, top, SkPaint*, paint); -RECORD5(DrawText, char*, text, size_t, byteLength, SkScalar, x, SkScalar, y, SkPaint, paint); -RECORD5(DrawTextOnPath, char*, text, +RECORD4(DrawSprite, ImmutableBitmap, bitmap, int, left, int, top, Optional, paint); +RECORD5(DrawText, PODArray, text, + size_t, byteLength, + SkScalar, x, + SkScalar, y, + SkPaint, paint); +RECORD5(DrawTextOnPath, PODArray, text, size_t, byteLength, SkPath, path, - SkMatrix*, matrix, + Optional, matrix, SkPaint, paint); // This guy is so ugly we just write it manually. @@ -201,11 +238,11 @@ struct DrawVertices { SkCanvas::VertexMode vmode; int vertexCount; - SkPoint* vertices; - SkPoint* texs; - SkColor* colors; + PODArray vertices; + PODArray texs; + PODArray colors; SkAutoTUnref xmode; - uint16_t* indices; + PODArray indices; int indexCount; SkPaint paint; }; diff --git a/tests/RecorderTest.cpp b/tests/RecorderTest.cpp index 34b3f2c0de..570bce8a3c 100644 --- a/tests/RecorderTest.cpp +++ b/tests/RecorderTest.cpp @@ -11,6 +11,8 @@ #include "SkRecorder.h" #include "SkRecords.h" +#include "SkEmptyShader.h" + #define COUNT(T) + 1 static const int kRecordTypes = SK_RECORD_TYPES(COUNT); #undef COUNT @@ -41,3 +43,23 @@ DEF_TEST(Recorder, r) { REPORTER_ASSERT(r, 1 == tally.count()); } + +// Regression test for leaking refs held by optional arguments. +DEF_TEST(Recorder_RefLeaking, r) { + // We use SaveLayer to test: + // - its SkRect argument is optional and SkRect is POD. Just testing that that works. + // - its SkPaint argument is optional and SkPaint is not POD. The bug was here. + + SkRect bounds; + SkPaint paint; + paint.setShader(SkNEW(SkEmptyShader))->unref(); + + REPORTER_ASSERT(r, paint.getShader()->unique()); + { + SkRecord record; + SkRecorder recorder(SkRecorder::kWriteOnly_Mode, &record, 1920, 1080); + recorder.saveLayer(&bounds, &paint); + REPORTER_ASSERT(r, !paint.getShader()->unique()); + } + REPORTER_ASSERT(r, paint.getShader()->unique()); +} -- cgit v1.2.3