From 7dade42e6f2e35dd27b4090fd7894322d755d4db Mon Sep 17 00:00:00 2001 From: "djsollen@google.com" Date: Fri, 27 Jul 2012 15:58:23 +0000 Subject: [PATCH] Always store pixels of mutable bitmaps when recording a SkPicture. Prior to this CL mutable bitmaps only saved a copy of their pixels if a flag was set at recording time. That flag has been removed and the default behavior when recording a mutable bitmap is to make a copy of it's pixels. This is the only way to ensure that the pixels are not manipulated before we playback their contents. However, enabling this behavior breaks the recording of extracted bitmaps in SkPicture. This is because we currently cache bitmaps within a picture based only on their pixelRef. This results in false positive cache hit when drawing an extracted bitmap as it shares a pixelRef with its orginating bitmap. Therefore we must update the index of the bitmap cache to be both the pixelRef AND the size and offset of the bitmap using those pixels. BUG= TEST=extractbitmap.cpp Review URL: https://codereview.appspot.com/6439043 git-svn-id: http://skia.googlecode.com/svn/trunk@4809 2bbb7eff-a529-9590-31e7-b0007b416f81 --- include/core/SkPicture.h | 16 +--------------- src/core/SkPicture.cpp | 4 ---- src/core/SkPictureRecord.cpp | 26 +++++++++++--------------- src/core/SkPictureRecord.h | 30 +++++++++++++++++++++--------- src/utils/SkDeferredCanvas.cpp | 3 +-- tests/CanvasTest.cpp | 3 --- 6 files changed, 34 insertions(+), 48 deletions(-) diff --git a/include/core/SkPicture.h b/include/core/SkPicture.h index 976c594..dae7ed7 100644 --- a/include/core/SkPicture.h +++ b/include/core/SkPicture.h @@ -58,16 +58,7 @@ public: clip-query calls will reflect the path's bounds, not the actual path. */ - kUsePathBoundsForClip_RecordingFlag = 0x01, - - /* When a draw operation is recorded that has a bitmap parameter, it - may be unsafe to defer rendering if source bitmap may be written to - between the time of recording and the time of executing the draw - operation. This flag specifies that SkPicture should serialize a - snapshot of any source bitmaps that reside in RAM and are not - marked as immutable, making the draw operation safe for deferral. - */ - kFlattenMutableNonTexturePixelRefs_RecordingFlag = 0x02 + kUsePathBoundsForClip_RecordingFlag = 0x01 }; /** Returns the canvas that records the drawing commands. @@ -96,11 +87,6 @@ public: */ bool hasRecorded() const; - /** Returns true if a snapshot of the specified bitmap will be flattened - whaen a draw operation using the bitmap is recorded. - */ - bool willFlattenPixelsOnRecord(const SkBitmap&) const; - /** Replays the drawing commands on the specified canvas. This internally calls endRecording() if that has not already been called. @param surface the canvas receiving the drawing commands. diff --git a/src/core/SkPicture.cpp b/src/core/SkPicture.cpp index 58098e7..9f606f7 100644 --- a/src/core/SkPicture.cpp +++ b/src/core/SkPicture.cpp @@ -168,10 +168,6 @@ bool SkPicture::hasRecorded() const { return NULL != fRecord && fRecord->writeStream().size() > 0; } -bool SkPicture::willFlattenPixelsOnRecord(const SkBitmap& bitmap) const { - return NULL != fRecord && fRecord->shouldFlattenPixels(bitmap); -} - SkCanvas* SkPicture::getRecordingCanvas() const { // will be null if we are not recording return fRecord; diff --git a/src/core/SkPictureRecord.cpp b/src/core/SkPictureRecord.cpp index 9f5a1c2..8b3e9fd 100644 --- a/src/core/SkPictureRecord.cpp +++ b/src/core/SkPictureRecord.cpp @@ -523,7 +523,7 @@ void SkPictureRecord::reset() { fPathHeap = NULL; fBitmaps.reset(); - fPixelRefDictionary.reset(); + fBitmapIndexCache.reset(); fMatrices.reset(); fPaints.reset(); fPictureRefs.unrefAll(); @@ -640,27 +640,23 @@ void SkPictureRecord::addText(const void* text, size_t byteLength) { /////////////////////////////////////////////////////////////////////////////// -bool SkPictureRecord::shouldFlattenPixels(const SkBitmap& bitmap) const { - return (fRecordFlags & - SkPicture::kFlattenMutableNonTexturePixelRefs_RecordingFlag) - && !bitmap.isImmutable() && bitmap.pixelRef() - && NULL == bitmap.getTexture(); -} - int SkPictureRecord::find(const SkBitmap& bitmap) { int dictionaryIndex = 0; - PixelRefDictionaryEntry entry; - bool flattenPixels = shouldFlattenPixels(bitmap); + BitmapIndexCacheEntry entry; + const bool flattenPixels = !bitmap.isImmutable(); if (flattenPixels) { // Flattened bitmap may be very large. First attempt a fast lookup // based on generation ID to avoid unnecessary flattening in // fBitmaps.find() - entry.fKey = bitmap.pixelRef()->getGenerationID(); + entry.fGenerationId = bitmap.getGenerationID(); + entry.fPixelOffset = bitmap.pixelRefOffset(); + entry.fWidth = bitmap.width(); + entry.fHeight = bitmap.height(); dictionaryIndex = - SkTSearch(fPixelRefDictionary.begin(), - fPixelRefDictionary.count(), entry, sizeof(entry)); + SkTSearch(fBitmapIndexCache.begin(), + fBitmapIndexCache.count(), entry, sizeof(entry)); if (dictionaryIndex >= 0) { - return fPixelRefDictionary[dictionaryIndex].fIndex; + return fBitmapIndexCache[dictionaryIndex].fIndex; } } @@ -671,7 +667,7 @@ int SkPictureRecord::find(const SkBitmap& bitmap) { if (flattenPixels) { entry.fIndex = index; dictionaryIndex = ~dictionaryIndex; - *fPixelRefDictionary.insert(dictionaryIndex) = entry; + *fBitmapIndexCache.insert(dictionaryIndex) = entry; } return index; } diff --git a/src/core/SkPictureRecord.h b/src/core/SkPictureRecord.h index 23d0828..3dda39b 100644 --- a/src/core/SkPictureRecord.h +++ b/src/core/SkPictureRecord.h @@ -96,18 +96,30 @@ public: return fWriter; } - bool shouldFlattenPixels(const SkBitmap&) const; - void endRecording(); private: - struct PixelRefDictionaryEntry { - uint32_t fKey; // SkPixelRef GenerationID. + struct BitmapIndexCacheEntry { + uint32_t fGenerationId; // SkPixelRef GenerationID. + size_t fPixelOffset; + uint32_t fWidth; + uint32_t fHeight; uint32_t fIndex; // Index of corresponding flattened bitmap in fBitmaps. - bool operator < (const PixelRefDictionaryEntry& other) const { - return this->fKey < other.fKey; + bool operator < (const BitmapIndexCacheEntry& other) const { + if (this->fGenerationId != other.fGenerationId) { + return this->fGenerationId < other.fGenerationId; + } else if(this->fPixelOffset != other.fPixelOffset) { + return this->fPixelOffset < other.fPixelOffset; + } else if(this->fWidth != other.fWidth) { + return this->fWidth < other.fWidth; + } else { + return this->fHeight < other.fHeight; + } } - bool operator != (const PixelRefDictionaryEntry& other) const { - return this->fKey != other.fKey; + bool operator != (const BitmapIndexCacheEntry& other) const { + return this->fGenerationId != other.fGenerationId + || this->fPixelOffset != other.fPixelOffset + || this->fWidth != other.fWidth + || this->fHeight != other.fHeight; } }; @@ -192,7 +204,7 @@ private: SkRefCntSet fRCSet; SkRefCntSet fTFSet; - SkTDArray fPixelRefDictionary; + SkTDArray fBitmapIndexCache; SkBitmapDictionary fBitmaps; SkMatrixDictionary fMatrices; SkPaintDictionary fPaints; diff --git a/src/utils/SkDeferredCanvas.cpp b/src/utils/SkDeferredCanvas.cpp index c5a2183..47e551e 100644 --- a/src/utils/SkDeferredCanvas.cpp +++ b/src/utils/SkDeferredCanvas.cpp @@ -576,8 +576,7 @@ void SkDeferredCanvas::DeferredDevice::beginRecording() { fRecordingCanvas = fPipeWriter.startRecording(&fPipeController, 0); #else fRecordingCanvas = fPicture.beginRecording(fImmediateDevice->width(), - fImmediateDevice->height(), - SkPicture::kFlattenMutableNonTexturePixelRefs_RecordingFlag); + fImmediateDevice->height()); #endif } diff --git a/tests/CanvasTest.cpp b/tests/CanvasTest.cpp index cf70f43..5401026 100644 --- a/tests/CanvasTest.cpp +++ b/tests/CanvasTest.cpp @@ -884,9 +884,6 @@ static void TestCanvas(skiatest::Reporter* reporter) { TestOverrideStateConsistency(reporter, testStepArray()[testStep]); SkPictureTester::TestPictureFlattenedObjectReuse(reporter, testStepArray()[testStep], 0); - SkPictureTester::TestPictureFlattenedObjectReuse(reporter, - testStepArray()[testStep], - SkPicture::kFlattenMutableNonTexturePixelRefs_RecordingFlag); } } -- 2.7.4