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
diff --git a/src/record/SkRecordDraw.cpp b/src/record/SkRecordDraw.cpp
index f0d3772..c6e8550 100644
--- a/src/record/SkRecordDraw.cpp
+++ b/src/record/SkRecordDraw.cpp
@@ -76,7 +76,7 @@
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 @@
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 7958280..1080656 100644
--- a/src/record/SkRecords.h
+++ b/src/record/SkRecords.h
@@ -107,6 +107,31 @@
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 <typename T>
+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 <typename T>
+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 @@
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<SkRect>, bounds, Optional<SkPaint>, paint, SkCanvas::SaveFlags, flags);
static const unsigned kUnsetPopOffset = 0;
RECORD2(PushCull, SkRect, rect, unsigned, popOffset);
@@ -147,33 +171,46 @@
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<SkPaint>, paint);
+RECORD3(DrawBitmapMatrix, ImmutableBitmap, bitmap, SkMatrix, matrix, Optional<SkPaint>, paint);
+RECORD4(DrawBitmapNine, ImmutableBitmap, bitmap,
+ SkIRect, center,
+ SkRect, dst,
+ Optional<SkPaint>, paint);
RECORD5(DrawBitmapRectToRect, ImmutableBitmap, bitmap,
- SkRect*, src,
+ Optional<SkRect>, src,
SkRect, dst,
- SkPaint*, paint,
+ Optional<SkPaint>, 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<char>, text,
+ size_t, byteLength,
+ PODArray<SkPoint>, pos,
+ SkPaint, paint);
+RECORD5(DrawPosTextH, PODArray<char>, text,
size_t, byteLength,
- SkScalar*, xpos,
+ PODArray<SkScalar>, 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<SkPaint>, paint);
+RECORD5(DrawText, PODArray<char>, text,
+ size_t, byteLength,
+ SkScalar, x,
+ SkScalar, y,
+ SkPaint, paint);
+RECORD5(DrawTextOnPath, PODArray<char>, text,
size_t, byteLength,
SkPath, path,
- SkMatrix*, matrix,
+ Optional<SkMatrix>, matrix,
SkPaint, paint);
// This guy is so ugly we just write it manually.
@@ -201,11 +238,11 @@
SkCanvas::VertexMode vmode;
int vertexCount;
- SkPoint* vertices;
- SkPoint* texs;
- SkColor* colors;
+ PODArray<SkPoint> vertices;
+ PODArray<SkPoint> texs;
+ PODArray<SkColor> colors;
SkAutoTUnref<SkXfermode> xmode;
- uint16_t* indices;
+ PODArray<uint16_t> indices;
int indexCount;
SkPaint paint;
};
diff --git a/tests/RecorderTest.cpp b/tests/RecorderTest.cpp
index 34b3f2c..570bce8 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 @@
REPORTER_ASSERT(r, 1 == tally.count<SkRecords::DrawRect>());
}
+
+// 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());
+}