From 20ee1baa69715365e53d12ffb0e9d05d8dc0eda6 Mon Sep 17 00:00:00 2001 From: Leon Scroggins III Date: Tue, 15 Jul 2014 16:04:55 -0400 Subject: Change SkCanvasState to use inheritance. Cherry-pick from master. The base class, SkCanvasState, now holds the version, width, and height. These fields will always be a necessary part of the class. (Also add in some padding.) The other fields, which may change, have been moved into the subclass, SkCanvasState_v1. If/when the version changes, it will correspond to a new subclass. In SkCanvasStateUtils::CreateFromCanvasState, check the version on the base class, then do a static_cast to the version corresponding to SkCanvasState::version. Remove CANVAS_STATE_VERSION, which is redundant with the version specified by the subclass. Use unambiguous type for rowBytes. Build Android with SK_SUPPORT_LEGACY_CLIPTOLAYERFLAG. This allows us to run the full suite of CanvasState tests. It is also representative of what will be used on Android by WebView. Fix CanvasStateTest where it was broken inside ifdef'ed out code. Use SkCanvas::getBaseLayerSize() instead of the deprecated SkCanvas::getDeviceSize(). Update the comments in the header to be more clear. In particular, an SkCanvasState can only be used to pass an SkCanvas' state to a future version of Skia (or the same); not an older version. NOTREECHECKS=true BUG=b/15693384 R=reed@google.com, djsollen@google.com, mtklein@google.com Author: scroggo@google.com Review URL: https://codereview.chromium.org/372003002 Review URL: https://codereview.chromium.org/396763002 --- include/utils/SkCanvasStateUtils.h | 15 ++++--- src/utils/SkCanvasStateUtils.cpp | 92 +++++++++++++++++++++----------------- 2 files changed, 60 insertions(+), 47 deletions(-) diff --git a/include/utils/SkCanvasStateUtils.h b/include/utils/SkCanvasStateUtils.h index f8266c9b3..6ea7b1030 100644 --- a/include/utils/SkCanvasStateUtils.h +++ b/include/utils/SkCanvasStateUtils.h @@ -13,9 +13,9 @@ class SkCanvasState; /** - * A set of functions that are useful for copying an SkCanvas across a library - * boundary where the Skia libraries on either side of the boundary may not be - * version identical. The expected usage is outline below... + * A set of functions that are useful for copying the state of an SkCanvas + * across a library boundary where the Skia library on the other side of the + * boundary may be newer. The expected usage is outline below... * * Lib Boundary * CaptureCanvasState(...) ||| @@ -30,8 +30,8 @@ class SkCanvasState; namespace SkCanvasStateUtils { /** * Captures the current state of the canvas into an opaque ptr that is safe - * to pass between different instances of Skia (which may or may not be the - * same version). The function will return NULL in the event that one of the + * to pass to a different instance of Skia (which may be the same version, + * or may be newer). The function will return NULL in the event that one of the * following conditions are true. * 1) the canvas device type is not supported (currently only raster is supported) * 2) the canvas clip type is not supported (currently only non-AA clips are supported) @@ -56,7 +56,7 @@ namespace SkCanvasStateUtils { * 1) the captured state is in an unrecognized format * 2) the captured canvas device type is not supported * - * @param canvas The canvas you wish to capture the current state of. + * @param state Opaque object created by CaptureCanvasState. * @return NULL or an SkCanvas* whose devices and matrix/clip state are * identical to the captured canvas. The caller is responsible for * calling unref on the SkCanvas. @@ -66,7 +66,8 @@ namespace SkCanvasStateUtils { /** * Free the memory associated with the captured canvas state. The state * should not be released until all SkCanvas objects created using that - * state have been dereferenced. + * state have been dereferenced. Must be called from the same library + * instance that created the state via CaptureCanvasState. * * @param state The captured state you wish to dispose of. */ diff --git a/src/utils/SkCanvasStateUtils.cpp b/src/utils/SkCanvasStateUtils.cpp index eaee61b5e..e286c9138 100644 --- a/src/utils/SkCanvasStateUtils.cpp +++ b/src/utils/SkCanvasStateUtils.cpp @@ -13,14 +13,13 @@ #include "SkErrorInternals.h" #include "SkWriter32.h" -#define CANVAS_STATE_VERSION 1 /* * WARNING: The structs below are part of a stable ABI and as such we explicitly * use unambigious primitives (e.g. int32_t instead of an enum). * - * ANY CHANGES TO THE STRUCTS BELOW THAT IMPACT THE ABI SHOULD RESULT IN AN - * UPDATE OF THE CANVAS_STATE_VERSION. SUCH CHANGES SHOULD ONLY BE MADE IF - * ABSOLUTELY NECESSARY! + * ANY CHANGES TO THE STRUCTS BELOW THAT IMPACT THE ABI SHOULD RESULT IN A NEW + * NEW SUBCLASS OF SkCanvasState. SUCH CHANGES SHOULD ONLY BE MADE IF ABSOLUTELY + * NECESSARY! */ enum RasterConfigs { kUnknown_RasterConfig = 0, @@ -48,7 +47,8 @@ struct SkMCState { ClipRect* clipRects; }; -// NOTE: If you add more members, bump CanvasState::version. +// NOTE: If you add more members, create a new subclass of SkCanvasState with a +// new CanvasState::version. struct SkCanvasLayerState { CanvasBackend type; int32_t x, y; @@ -60,7 +60,7 @@ struct SkCanvasLayerState { union { struct { RasterConfig config; // pixel format: a value from RasterConfigs. - size_t rowBytes; // Number of bytes from start of one line to next. + uint64_t rowBytes; // Number of bytes from start of one line to next. void* pixels; // The pixels, all (height * rowBytes) of them. } raster; struct { @@ -71,20 +71,41 @@ struct SkCanvasLayerState { class SkCanvasState { public: - SkCanvasState(SkCanvas* canvas) { + SkCanvasState(int32_t version, SkCanvas* canvas) { SkASSERT(canvas); - version = CANVAS_STATE_VERSION; - width = canvas->getDeviceSize().width(); - height = canvas->getDeviceSize().height(); + this->version = version; + width = canvas->getBaseLayerSize().width(); + height = canvas->getBaseLayerSize().height(); + + } + + /** + * The version this struct was built with. This field must always appear + * first in the struct so that when the versions don't match (and the + * remaining contents and size are potentially different) we can still + * compare the version numbers. + */ + int32_t version; + int32_t width; + int32_t height; + int32_t alignmentPadding; +}; + +class SkCanvasState_v1 : public SkCanvasState { +public: + static const int32_t kVersion = 1; + + SkCanvasState_v1(SkCanvas* canvas) + : INHERITED(kVersion, canvas) + { layerCount = 0; layers = NULL; - originalCanvas = SkRef(canvas); - mcState.clipRectCount = 0; mcState.clipRects = NULL; + originalCanvas = SkRef(canvas); } - ~SkCanvasState() { + ~SkCanvasState_v1() { // loop through the layers and free the data allocated to the clipRects for (int i = 0; i < layerCount; ++i) { sk_free(layers[i].mcState.clipRects); @@ -98,24 +119,13 @@ public: originalCanvas->unref(); } - /** - * The version this struct was built with. This field must always appear - * first in the struct so that when the versions don't match (and the - * remaining contents and size are potentially different) we can still - * compare the version numbers. - */ - int32_t version; - - int32_t width; - int32_t height; - SkMCState mcState; int32_t layerCount; SkCanvasLayerState* layers; - private: SkCanvas* originalCanvas; + typedef SkCanvasState INHERITED; }; //////////////////////////////////////////////////////////////////////////////// @@ -191,7 +201,7 @@ SkCanvasState* SkCanvasStateUtils::CaptureCanvasState(SkCanvas* canvas) { return NULL; } - SkAutoTDelete canvasState(SkNEW_ARGS(SkCanvasState, (canvas))); + SkAutoTDelete canvasState(SkNEW_ARGS(SkCanvasState_v1, (canvas))); // decompose the total matrix and clip setup_MC_state(&canvasState->mcState, canvas->getTotalMatrix(), @@ -247,7 +257,7 @@ SkCanvasState* SkCanvasStateUtils::CaptureCanvasState(SkCanvas* canvas) { // for now, just ignore any client supplied DrawFilter. if (canvas->getDrawFilter()) { -// SkDEBUGF(("CaptureCanvasState will ignore the canvases draw filter.\n")); +// SkDEBUGF(("CaptureCanvasState will ignore the canvas's draw filter.\n")); } return canvasState.detach(); @@ -291,7 +301,7 @@ static SkCanvas* create_canvas_from_canvas_layer(const SkCanvasLayerState& layer bitmap.installPixels(SkImageInfo::Make(layerState.width, layerState.height, colorType, kPremul_SkAlphaType), - layerState.raster.pixels, layerState.raster.rowBytes); + layerState.raster.pixels, (size_t) layerState.raster.rowBytes); SkASSERT(!bitmap.empty()); SkASSERT(!bitmap.isNull()); @@ -306,30 +316,28 @@ static SkCanvas* create_canvas_from_canvas_layer(const SkCanvasLayerState& layer SkCanvas* SkCanvasStateUtils::CreateFromCanvasState(const SkCanvasState* state) { SkASSERT(state); + // Currently there is only one possible version. + SkASSERT(SkCanvasState_v1::kVersion == state->version); - // check that the versions match - if (CANVAS_STATE_VERSION != state->version) { - SkDebugf("CreateFromCanvasState version does not match the one use to create the input"); - return NULL; - } + const SkCanvasState_v1* state_v1 = static_cast(state); - if (state->layerCount < 1) { + if (state_v1->layerCount < 1) { return NULL; } SkAutoTUnref canvas(SkNEW_ARGS(SkCanvasStack, (state->width, state->height))); // setup the matrix and clip on the n-way canvas - setup_canvas_from_MC_state(state->mcState, canvas); + setup_canvas_from_MC_state(state_v1->mcState, canvas); // Iterate over the layers and add them to the n-way canvas - for (int i = state->layerCount - 1; i >= 0; --i) { - SkAutoTUnref canvasLayer(create_canvas_from_canvas_layer(state->layers[i])); + for (int i = state_v1->layerCount - 1; i >= 0; --i) { + SkAutoTUnref canvasLayer(create_canvas_from_canvas_layer(state_v1->layers[i])); if (!canvasLayer.get()) { return NULL; } - canvas->pushCanvas(canvasLayer.get(), SkIPoint::Make(state->layers[i].x, - state->layers[i].y)); + canvas->pushCanvas(canvasLayer.get(), SkIPoint::Make(state_v1->layers[i].x, + state_v1->layers[i].y)); } return canvas.detach(); @@ -338,5 +346,9 @@ SkCanvas* SkCanvasStateUtils::CreateFromCanvasState(const SkCanvasState* state) //////////////////////////////////////////////////////////////////////////////// void SkCanvasStateUtils::ReleaseCanvasState(SkCanvasState* state) { - SkDELETE(state); + SkASSERT(!state || SkCanvasState_v1::kVersion == state->version); + // Upcast to the correct version of SkCanvasState. This avoids having a virtual destructor on + // SkCanvasState. That would be strange since SkCanvasState has no other virtual functions, and + // instead uses the field "version" to determine how to behave. + SkDELETE(static_cast(state)); } -- cgit v1.2.3 From f2d87ba43e54e1c62cfac22742210aa126113a10 Mon Sep 17 00:00:00 2001 From: bungeman Date: Wed, 23 Jul 2014 13:31:06 -0700 Subject: Get additional DW font metrics when available. BUG=chromium:395043 R=eae@chromium.org, caryclark@google.com Author: bungeman@google.com Review URL: https://codereview.chromium.org/412993002 --- src/ports/SkScalerContext_win_dw.cpp | 32 +++++++++++++++++++++++++++++--- src/ports/SkTypeface_win_dw.h | 10 +++++++++- 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/src/ports/SkScalerContext_win_dw.cpp b/src/ports/SkScalerContext_win_dw.cpp index 4cb983f5c..28d41c1b9 100644 --- a/src/ports/SkScalerContext_win_dw.cpp +++ b/src/ports/SkScalerContext_win_dw.cpp @@ -26,6 +26,7 @@ #include "SkTypeface_win_dw.h" #include +#include static bool isLCD(const SkScalerContext::Rec& rec) { return SkMask::kLCD16_Format == rec.fMaskFormat || @@ -493,10 +494,8 @@ void SkScalerContext_DW::generateFontMetrics(SkPaint::FontMetrics* mx, } if (my) { - my->fTop = -fTextSizeRender * SkIntToScalar(dwfm.ascent) / upem; - my->fAscent = my->fTop; + my->fAscent = -fTextSizeRender * SkIntToScalar(dwfm.ascent) / upem; my->fDescent = fTextSizeRender * SkIntToScalar(dwfm.descent) / upem; - my->fBottom = my->fDescent; my->fLeading = fTextSizeRender * SkIntToScalar(dwfm.lineGap) / upem; my->fXHeight = fTextSizeRender * SkIntToScalar(dwfm.xHeight) / upem; my->fUnderlineThickness = fTextSizeRender * SkIntToScalar(dwfm.underlineThickness) / upem; @@ -504,6 +503,33 @@ void SkScalerContext_DW::generateFontMetrics(SkPaint::FontMetrics* mx, my->fFlags |= SkPaint::FontMetrics::kUnderlineThinknessIsValid_Flag; my->fFlags |= SkPaint::FontMetrics::kUnderlinePositionIsValid_Flag; + + if (NULL != fTypeface->fDWriteFontFace1.get()) { + DWRITE_FONT_METRICS1 dwfm1; + fTypeface->fDWriteFontFace1->GetMetrics(&dwfm1); + my->fTop = -fTextSizeRender * SkIntToScalar(dwfm1.glyphBoxTop) / upem; + my->fBottom = -fTextSizeRender * SkIntToScalar(dwfm1.glyphBoxBottom) / upem; + my->fXMin = fTextSizeRender * SkIntToScalar(dwfm1.glyphBoxLeft) / upem; + my->fXMax = fTextSizeRender * SkIntToScalar(dwfm1.glyphBoxRight) / upem; + + my->fMaxCharWidth = my->fXMax - my->fXMin; + } else { + AutoTDWriteTable head(fTypeface->fDWriteFontFace.get()); + if (head.fExists && + head.fSize >= sizeof(SkOTTableHead) && + head->version == SkOTTableHead::version1) + { + my->fTop = -fTextSizeRender * (int16_t)SkEndian_SwapBE16(head->yMax) / upem; + my->fBottom = -fTextSizeRender * (int16_t)SkEndian_SwapBE16(head->yMin) / upem; + my->fXMin = fTextSizeRender * (int16_t)SkEndian_SwapBE16(head->xMin) / upem; + my->fXMax = fTextSizeRender * (int16_t)SkEndian_SwapBE16(head->xMax) / upem; + + my->fMaxCharWidth = my->fXMax - my->fXMin; + } else { + my->fTop = my->fAscent; + my->fBottom = my->fDescent; + } + } } } diff --git a/src/ports/SkTypeface_win_dw.h b/src/ports/SkTypeface_win_dw.h index b064ce577..465db3446 100644 --- a/src/ports/SkTypeface_win_dw.h +++ b/src/ports/SkTypeface_win_dw.h @@ -17,6 +17,7 @@ #include "SkTypes.h" #include +#include class SkFontDescriptor; struct SkScalerContextRec; @@ -50,7 +51,13 @@ private: , fDWriteFontFamily(SkRefComPtr(fontFamily)) , fDWriteFont(SkRefComPtr(font)) , fDWriteFontFace(SkRefComPtr(fontFace)) - { } + { + if (!SUCCEEDED(fDWriteFontFace->QueryInterface(&fDWriteFontFace1))) { + // IUnknown::QueryInterface states that if it fails, punk will be set to NULL. + // http://blogs.msdn.com/b/oldnewthing/archive/2004/03/26/96777.aspx + SK_ALWAYSBREAK(NULL == fDWriteFontFace1.get()); + } + } public: SkTScopedComPtr fFactory; @@ -59,6 +66,7 @@ public: SkTScopedComPtr fDWriteFontFamily; SkTScopedComPtr fDWriteFont; SkTScopedComPtr fDWriteFontFace; + SkTScopedComPtr fDWriteFontFace1; static DWriteFontTypeface* Create(IDWriteFactory* factory, IDWriteFontFace* fontFace, -- cgit v1.2.3 From 0c1c911ff19894031cd1b8acef3b597e26a85334 Mon Sep 17 00:00:00 2001 From: sugoi Date: Thu, 3 Jul 2014 10:44:26 -0700 Subject: Adding 64 bit checks Added a few more checks to avoid overflowing 32 bit sizes while computing convolutions. I also changed a dangerously misleading INHERITED typedef. BUG=389570 R=senorblanco@google.com, senorblanco@chromium.org Author: sugoi@chromium.org Review URL: https://codereview.chromium.org/361403006 --- src/core/SkConvolver.cpp | 2 +- src/core/SkScaledImageCache.cpp | 3 ++- src/effects/SkColorMatrixFilter.cpp | 4 +++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/core/SkConvolver.cpp b/src/core/SkConvolver.cpp index 0e97fac07..0f5cf9021 100644 --- a/src/core/SkConvolver.cpp +++ b/src/core/SkConvolver.cpp @@ -434,7 +434,7 @@ void BGRAConvolve2D(const unsigned char* sourceData, } // Compute where in the output image this row of final data will go. - unsigned char* curOutputRow = &output[outY * outputByteRowStride]; + unsigned char* curOutputRow = &output[(uint64_t)outY * outputByteRowStride]; // Get the list of rows that the circular buffer has, in order. int firstRowInCircularBuffer; diff --git a/src/core/SkScaledImageCache.cpp b/src/core/SkScaledImageCache.cpp index f266f9717..a03024819 100644 --- a/src/core/SkScaledImageCache.cpp +++ b/src/core/SkScaledImageCache.cpp @@ -266,7 +266,8 @@ private: bool SkScaledImageCacheDiscardableAllocator::allocPixelRef(SkBitmap* bitmap, SkColorTable* ctable) { size_t size = bitmap->getSize(); - if (0 == size) { + uint64_t size64 = bitmap->computeSize64(); + if (0 == size || size64 > (uint64_t)size) { return false; } diff --git a/src/effects/SkColorMatrixFilter.cpp b/src/effects/SkColorMatrixFilter.cpp index b60fa84b6..bd1df79d1 100644 --- a/src/effects/SkColorMatrixFilter.cpp +++ b/src/effects/SkColorMatrixFilter.cpp @@ -450,6 +450,8 @@ public: private: GrGLUniformManager::UniformHandle fMatrixHandle; GrGLUniformManager::UniformHandle fVectorHandle; + + typedef GrGLEffect INHERITED; }; private: @@ -462,7 +464,7 @@ private: SkColorMatrix fMatrix; - typedef GrGLEffect INHERITED; + typedef GrEffect INHERITED; }; GR_DEFINE_EFFECT_TEST(ColorMatrixEffect); -- cgit v1.2.3 From 0d78ac294f814a45380eb44b0bb4c01aae2cab01 Mon Sep 17 00:00:00 2001 From: Hal Canary Date: Tue, 12 Aug 2014 15:28:22 -0400 Subject: Set maximum output size for scaled-image-cache images Accessable via: SkGraphics::{G|S}etImageCacheSingleAllocationByteLimit() Also, a unit test. BUG=389439 R=reed@google.com, humper@google.com, reveman@chromium.org, tomhudson@google.com, vangelis@chromium.org Author: halcanary@google.com Review URL: https://codereview.chromium.org/394003003 Review URL: https://codereview.chromium.org/462993003 --- bench/ImageCacheBench.cpp | 2 +- gyp/tests.gypi | 1 + include/core/SkGraphics.h | 44 ++++++++++++++++++++-- src/core/SkBitmapProcState.cpp | 19 +++++++++- src/core/SkScaledImageCache.cpp | 82 ++++++++++++++++++++++++++++------------- src/core/SkScaledImageCache.h | 35 ++++++++++++------ tests/ImageCacheTest.cpp | 2 +- tests/ScaledImageCache.cpp | 60 ++++++++++++++++++++++++++++++ 8 files changed, 200 insertions(+), 45 deletions(-) create mode 100644 tests/ScaledImageCache.cpp diff --git a/bench/ImageCacheBench.cpp b/bench/ImageCacheBench.cpp index 5f1715fc3..e65d1fc3e 100644 --- a/bench/ImageCacheBench.cpp +++ b/bench/ImageCacheBench.cpp @@ -37,7 +37,7 @@ protected: } virtual void onDraw(const int loops, SkCanvas*) SK_OVERRIDE { - if (fCache.getBytesUsed() == 0) { + if (fCache.getTotalBytesUsed() == 0) { this->populateCache(); } diff --git a/gyp/tests.gypi b/gyp/tests.gypi index 2a7dddd4f..9e418d687 100644 --- a/gyp/tests.gypi +++ b/gyp/tests.gypi @@ -155,6 +155,7 @@ '../tests/RuntimeConfigTest.cpp', '../tests/SHA1Test.cpp', '../tests/ScalarTest.cpp', + '../tests/ScaledImageCache.cpp', '../tests/SerializationTest.cpp', '../tests/ShaderImageFilterTest.cpp', '../tests/ShaderOpacityTest.cpp', diff --git a/include/core/SkGraphics.h b/include/core/SkGraphics.h index 2667a388d..e7865ca5a 100644 --- a/include/core/SkGraphics.h +++ b/include/core/SkGraphics.h @@ -79,9 +79,47 @@ public: */ static void PurgeFontCache(); - static size_t GetImageCacheBytesUsed(); - static size_t GetImageCacheByteLimit(); - static size_t SetImageCacheByteLimit(size_t newLimit); + /** + * Scaling bitmaps with the SkPaint::kHigh_FilterLevel setting is + * expensive, so the result is saved in the global Scaled Image + * Cache. + * + * This function returns the memory usage of the Scaled Image Cache. + */ + static size_t GetImageCacheTotalBytesUsed(); + /** + * These functions get/set the memory usage limit for the Scaled + * Image Cache. Bitmaps are purged from the cache when the + * memory useage exceeds this limit. + */ + static size_t GetImageCacheTotalByteLimit(); + static size_t SetImageCacheTotalByteLimit(size_t newLimit); + + // DEPRECATED + static size_t GetImageCacheBytesUsed() { + return GetImageCacheTotalBytesUsed(); + } + // DEPRECATED + static size_t GetImageCacheByteLimit() { + return GetImageCacheTotalByteLimit(); + } + // DEPRECATED + static size_t SetImageCacheByteLimit(size_t newLimit) { + return SetImageCacheTotalByteLimit(newLimit); + } + + /** + * Scaling bitmaps with the SkPaint::kHigh_FilterLevel setting is + * expensive, so the result is saved in the global Scaled Image + * Cache. When the resulting bitmap is too large, this can + * overload the cache. If the ImageCacheSingleAllocationByteLimit + * is set to a non-zero number, and the resulting bitmap would be + * larger than that value, the bitmap scaling algorithm falls + * back onto a cheaper algorithm and does not cache the result. + * Zero is the default value. + */ + static size_t GetImageCacheSingleAllocationByteLimit(); + static size_t SetImageCacheSingleAllocationByteLimit(size_t newLimit); /** * Applications with command line options may pass optional state, such diff --git a/src/core/SkBitmapProcState.cpp b/src/core/SkBitmapProcState.cpp index f6f9f3fa0..137bcda91 100644 --- a/src/core/SkBitmapProcState.cpp +++ b/src/core/SkBitmapProcState.cpp @@ -127,6 +127,21 @@ private: }; #define AutoScaledCacheUnlocker(...) SK_REQUIRE_LOCAL_VAR(AutoScaledCacheUnlocker) +// Check to see that the size of the bitmap that would be produced by +// scaling by the given inverted matrix is less than the maximum allowed. +static inline bool cache_size_okay(const SkBitmap& bm, const SkMatrix& invMat) { + size_t maximumAllocation + = SkScaledImageCache::GetSingleAllocationByteLimit(); + if (0 == maximumAllocation) { + return true; + } + // float matrixScaleFactor = 1.0 / (invMat.scaleX * invMat.scaleY); + // return ((origBitmapSize * matrixScaleFactor) < maximumAllocationSize); + // Skip the division step: + return bm.info().getSafeSize(bm.info().minRowBytes()) + < (maximumAllocation * invMat.getScaleX() * invMat.getScaleY()); +} + // TODO -- we may want to pass the clip into this function so we only scale // the portion of the image that we're going to need. This will complicate // the interface to the cache, but might be well worth it. @@ -140,14 +155,14 @@ bool SkBitmapProcState::possiblyScaleImage() { if (fFilterLevel <= SkPaint::kLow_FilterLevel) { return false; } - // Check to see if the transformation matrix is simple, and if we're // doing high quality scaling. If so, do the bitmap scale here and // remove the scaling component from the matrix. if (SkPaint::kHigh_FilterLevel == fFilterLevel && fInvMatrix.getType() <= (SkMatrix::kScale_Mask | SkMatrix::kTranslate_Mask) && - kN32_SkColorType == fOrigBitmap.colorType()) { + kN32_SkColorType == fOrigBitmap.colorType() && + cache_size_okay(fOrigBitmap, fInvMatrix)) { SkScalar invScaleX = fInvMatrix.getScaleX(); SkScalar invScaleY = fInvMatrix.getScaleY(); diff --git a/src/core/SkScaledImageCache.cpp b/src/core/SkScaledImageCache.cpp index a03024819..43ff7ef89 100644 --- a/src/core/SkScaledImageCache.cpp +++ b/src/core/SkScaledImageCache.cpp @@ -165,12 +165,13 @@ void SkScaledImageCache::init() { #else fHash = NULL; #endif - fBytesUsed = 0; + fTotalBytesUsed = 0; fCount = 0; + fSingleAllocationByteLimit = 0; fAllocator = NULL; // One of these should be explicit set by the caller after we return. - fByteLimit = 0; + fTotalByteLimit = 0; fDiscardableFactory = NULL; } @@ -297,7 +298,7 @@ SkScaledImageCache::SkScaledImageCache(DiscardableFactory factory) { SkScaledImageCache::SkScaledImageCache(size_t byteLimit) { this->init(); - fByteLimit = byteLimit; + fTotalByteLimit = byteLimit; } SkScaledImageCache::~SkScaledImageCache() { @@ -502,10 +503,10 @@ void SkScaledImageCache::purgeAsNeeded() { byteLimit = SK_MaxU32; // no limit based on bytes } else { countLimit = SK_MaxS32; // no limit based on count - byteLimit = fByteLimit; + byteLimit = fTotalByteLimit; } - size_t bytesUsed = fBytesUsed; + size_t bytesUsed = fTotalBytesUsed; int countUsed = fCount; Rec* rec = fTail; @@ -531,13 +532,13 @@ void SkScaledImageCache::purgeAsNeeded() { rec = prev; } - fBytesUsed = bytesUsed; + fTotalBytesUsed = bytesUsed; fCount = countUsed; } -size_t SkScaledImageCache::setByteLimit(size_t newLimit) { - size_t prevLimit = fByteLimit; - fByteLimit = newLimit; +size_t SkScaledImageCache::setTotalByteLimit(size_t newLimit) { + size_t prevLimit = fTotalByteLimit; + fTotalByteLimit = newLimit; if (newLimit < prevLimit) { this->purgeAsNeeded(); } @@ -597,7 +598,7 @@ void SkScaledImageCache::addToHead(Rec* rec) { if (!fTail) { fTail = rec; } - fBytesUsed += rec->bytesUsed(); + fTotalBytesUsed += rec->bytesUsed(); fCount += 1; this->validate(); @@ -609,14 +610,14 @@ void SkScaledImageCache::addToHead(Rec* rec) { void SkScaledImageCache::validate() const { if (NULL == fHead) { SkASSERT(NULL == fTail); - SkASSERT(0 == fBytesUsed); + SkASSERT(0 == fTotalBytesUsed); return; } if (fHead == fTail) { SkASSERT(NULL == fHead->fPrev); SkASSERT(NULL == fHead->fNext); - SkASSERT(fHead->bytesUsed() == fBytesUsed); + SkASSERT(fHead->bytesUsed() == fTotalBytesUsed); return; } @@ -631,7 +632,7 @@ void SkScaledImageCache::validate() const { while (rec) { count += 1; used += rec->bytesUsed(); - SkASSERT(used <= fBytesUsed); + SkASSERT(used <= fTotalBytesUsed); rec = rec->fNext; } SkASSERT(fCount == count); @@ -661,10 +662,20 @@ void SkScaledImageCache::dump() const { } SkDebugf("SkScaledImageCache: count=%d bytes=%d locked=%d %s\n", - fCount, fBytesUsed, locked, + fCount, fTotalBytesUsed, locked, fDiscardableFactory ? "discardable" : "malloc"); } +size_t SkScaledImageCache::setSingleAllocationByteLimit(size_t newLimit) { + size_t oldLimit = fSingleAllocationByteLimit; + fSingleAllocationByteLimit = newLimit; + return oldLimit; +} + +size_t SkScaledImageCache::getSingleAllocationByteLimit() const { + return fSingleAllocationByteLimit; +} + /////////////////////////////////////////////////////////////////////////////// #include "SkThread.h" @@ -751,19 +762,19 @@ void SkScaledImageCache::Unlock(SkScaledImageCache::ID* id) { // get_cache()->dump(); } -size_t SkScaledImageCache::GetBytesUsed() { +size_t SkScaledImageCache::GetTotalBytesUsed() { SkAutoMutexAcquire am(gMutex); - return get_cache()->getBytesUsed(); + return get_cache()->getTotalBytesUsed(); } -size_t SkScaledImageCache::GetByteLimit() { +size_t SkScaledImageCache::GetTotalByteLimit() { SkAutoMutexAcquire am(gMutex); - return get_cache()->getByteLimit(); + return get_cache()->getTotalByteLimit(); } -size_t SkScaledImageCache::SetByteLimit(size_t newLimit) { +size_t SkScaledImageCache::SetTotalByteLimit(size_t newLimit) { SkAutoMutexAcquire am(gMutex); - return get_cache()->setByteLimit(newLimit); + return get_cache()->setTotalByteLimit(newLimit); } SkBitmap::Allocator* SkScaledImageCache::GetAllocator() { @@ -776,18 +787,37 @@ void SkScaledImageCache::Dump() { get_cache()->dump(); } +size_t SkScaledImageCache::SetSingleAllocationByteLimit(size_t size) { + SkAutoMutexAcquire am(gMutex); + return get_cache()->setSingleAllocationByteLimit(size); +} + +size_t SkScaledImageCache::GetSingleAllocationByteLimit() { + SkAutoMutexAcquire am(gMutex); + return get_cache()->getSingleAllocationByteLimit(); +} + /////////////////////////////////////////////////////////////////////////////// #include "SkGraphics.h" -size_t SkGraphics::GetImageCacheBytesUsed() { - return SkScaledImageCache::GetBytesUsed(); +size_t SkGraphics::GetImageCacheTotalBytesUsed() { + return SkScaledImageCache::GetTotalBytesUsed(); +} + +size_t SkGraphics::GetImageCacheTotalByteLimit() { + return SkScaledImageCache::GetTotalByteLimit(); } -size_t SkGraphics::GetImageCacheByteLimit() { - return SkScaledImageCache::GetByteLimit(); +size_t SkGraphics::SetImageCacheTotalByteLimit(size_t newLimit) { + return SkScaledImageCache::SetTotalByteLimit(newLimit); } -size_t SkGraphics::SetImageCacheByteLimit(size_t newLimit) { - return SkScaledImageCache::SetByteLimit(newLimit); +size_t SkGraphics::GetImageCacheSingleAllocationByteLimit() { + return SkScaledImageCache::GetSingleAllocationByteLimit(); } + +size_t SkGraphics::SetImageCacheSingleAllocationByteLimit(size_t newLimit) { + return SkScaledImageCache::SetSingleAllocationByteLimit(newLimit); +} + diff --git a/src/core/SkScaledImageCache.h b/src/core/SkScaledImageCache.h index fe072306d..817147e3b 100644 --- a/src/core/SkScaledImageCache.h +++ b/src/core/SkScaledImageCache.h @@ -60,9 +60,12 @@ public: static void Unlock(ID*); - static size_t GetBytesUsed(); - static size_t GetByteLimit(); - static size_t SetByteLimit(size_t newLimit); + static size_t GetTotalBytesUsed(); + static size_t GetTotalByteLimit(); + static size_t SetTotalByteLimit(size_t newLimit); + + static size_t SetSingleAllocationByteLimit(size_t); + static size_t GetSingleAllocationByteLimit(); static SkBitmap::Allocator* GetAllocator(); @@ -76,9 +79,9 @@ public: /** * Construct the cache to call DiscardableFactory when it * allocates memory for the pixels. In this mode, the cache has - * not explicit budget, and so methods like getBytesUsed() and - * getByteLimit() will return 0, and setByteLimit will ignore its argument - * and return 0. + * not explicit budget, and so methods like getTotalBytesUsed() + * and getTotalByteLimit() will return 0, and setTotalByteLimit + * will ignore its argument and return 0. */ SkScaledImageCache(DiscardableFactory); @@ -86,7 +89,7 @@ public: * Construct the cache, allocating memory with malloc, and respect the * byteLimit, purging automatically when a new image is added to the cache * that pushes the total bytesUsed over the limit. Note: The limit can be - * changed at runtime with setByteLimit. + * changed at runtime with setTotalByteLimit. */ SkScaledImageCache(size_t byteLimit); @@ -144,15 +147,22 @@ public: */ void unlock(ID*); - size_t getBytesUsed() const { return fBytesUsed; } - size_t getByteLimit() const { return fByteLimit; } + size_t getTotalBytesUsed() const { return fTotalBytesUsed; } + size_t getTotalByteLimit() const { return fTotalByteLimit; } + /** + * This is respected by SkBitmapProcState::possiblyScaleImage. + * 0 is no maximum at all; this is the default. + * setSingleAllocationByteLimit() returns the previous value. + */ + size_t setSingleAllocationByteLimit(size_t maximumAllocationSize); + size_t getSingleAllocationByteLimit() const; /** * Set the maximum number of bytes available to this cache. If the current * cache exceeds this new value, it will be purged to try to fit within * this new limit. */ - size_t setByteLimit(size_t newLimit); + size_t setTotalByteLimit(size_t newLimit); SkBitmap::Allocator* allocator() const { return fAllocator; }; @@ -175,8 +185,9 @@ private: // the allocator is NULL or one that matches discardables SkBitmap::Allocator* fAllocator; - size_t fBytesUsed; - size_t fByteLimit; + size_t fTotalBytesUsed; + size_t fTotalByteLimit; + size_t fSingleAllocationByteLimit; int fCount; Rec* findAndLock(uint32_t generationID, SkScalar sx, SkScalar sy, diff --git a/tests/ImageCacheTest.cpp b/tests/ImageCacheTest.cpp index 92d0b519d..00f6c77aa 100644 --- a/tests/ImageCacheTest.cpp +++ b/tests/ImageCacheTest.cpp @@ -74,7 +74,7 @@ static void test_cache(skiatest::Reporter* reporter, SkScaledImageCache& cache, } } - cache.setByteLimit(0); + cache.setTotalByteLimit(0); } #include "SkDiscardableMemoryPool.h" diff --git a/tests/ScaledImageCache.cpp b/tests/ScaledImageCache.cpp new file mode 100644 index 000000000..2040afea2 --- /dev/null +++ b/tests/ScaledImageCache.cpp @@ -0,0 +1,60 @@ +/* + * Copyright 2014 Google Inc. + * + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. + */ +#include "Test.h" +#include "SkGraphics.h" +#include "SkCanvas.h" + +static const int kCanvasSize = 1; +static const int kBitmapSize = 16; +static const int kScale = 8; + +static size_t test_scaled_image_cache_useage() { + SkAutoTUnref canvas( + SkCanvas::NewRasterN32(kCanvasSize, kCanvasSize)); + SkBitmap bitmap; + SkAssertResult(bitmap.allocN32Pixels(kBitmapSize, kBitmapSize)); + SkScalar scaledSize = SkIntToScalar(kScale * kBitmapSize); + canvas->clipRect(SkRect::MakeLTRB(0, 0, scaledSize, scaledSize)); + SkPaint paint; + paint.setFilterLevel(SkPaint::kHigh_FilterLevel); + size_t bytesUsed = SkGraphics::GetImageCacheBytesUsed(); + canvas->drawBitmapRect(bitmap, + SkRect::MakeLTRB(0, 0, scaledSize, scaledSize), + &paint); + return SkGraphics::GetImageCacheBytesUsed() - bytesUsed; +} + +// http://crbug.com/389439 +DEF_TEST(ScaledImageCache_SingleAllocationByteLimit, reporter) { + size_t originalByteLimit = SkGraphics::GetImageCacheByteLimit(); + size_t originalAllocationLimit = + SkGraphics::GetImageCacheSingleAllocationByteLimit(); + + size_t size = kBitmapSize * kScale * kBitmapSize * kScale + * SkColorTypeBytesPerPixel(kN32_SkColorType); + + SkGraphics::SetImageCacheByteLimit(0); // clear cache + SkGraphics::SetImageCacheByteLimit(2 * size); + SkGraphics::SetImageCacheSingleAllocationByteLimit(0); + + REPORTER_ASSERT(reporter, size == test_scaled_image_cache_useage()); + + SkGraphics::SetImageCacheByteLimit(0); // clear cache + SkGraphics::SetImageCacheByteLimit(2 * size); + SkGraphics::SetImageCacheSingleAllocationByteLimit(size * 2); + + REPORTER_ASSERT(reporter, size == test_scaled_image_cache_useage()); + + SkGraphics::SetImageCacheByteLimit(0); // clear cache + SkGraphics::SetImageCacheByteLimit(2 * size); + SkGraphics::SetImageCacheSingleAllocationByteLimit(size / 2); + + REPORTER_ASSERT(reporter, 0 == test_scaled_image_cache_useage()); + + SkGraphics::SetImageCacheSingleAllocationByteLimit(originalAllocationLimit); + SkGraphics::SetImageCacheByteLimit(originalByteLimit); +} -- cgit v1.2.3 From 413546c54f7ab90c8b584a2f7056fabd4f918acf Mon Sep 17 00:00:00 2001 From: reed Date: Sun, 13 Jul 2014 04:32:32 -0700 Subject: Add SkBitmap::readPixels() and reimplement copyTo and SkCanvas::readPixels This reverts commit 651eaeadeb0b1407f5fe192aeda90db1680fa2b8. TBR= Author: reed@chromium.org Review URL: https://codereview.chromium.org/390693002 --- include/core/SkBitmap.h | 22 +++++++ include/core/SkCanvas.h | 21 +++---- src/core/SkBitmap.cpp | 127 ++++++++++++++++++++++----------------- src/core/SkBitmapDevice.cpp | 76 +----------------------- src/core/SkConfig8888.cpp | 142 ++++++++++++++++++++++++++++++++++++++++++++ src/core/SkConfig8888.h | 4 ++ tests/BitmapCopyTest.cpp | 95 ++++++++++++++++++++++++++++- 7 files changed, 346 insertions(+), 141 deletions(-) diff --git a/include/core/SkBitmap.h b/include/core/SkBitmap.h index d3c33a8e0..13e9aa372 100644 --- a/include/core/SkBitmap.h +++ b/include/core/SkBitmap.h @@ -642,6 +642,28 @@ public: return this->copyTo(dst, this->colorType(), allocator); } + /** + * Copy the bitmap's pixels into the specified buffer (pixels + rowBytes), + * converting them into the requested format (SkImageInfo). The src pixels are read + * starting at the specified (srcX,srcY) offset, relative to the top-left corner. + * + * The specified ImageInfo and (srcX,srcY) offset specifies a source rectangle + * + * srcR.setXYWH(srcX, srcY, dstInfo.width(), dstInfo.height()); + * + * srcR is intersected with the bounds of the bitmap. If this intersection is not empty, + * then we have two sets of pixels (of equal size). Replace the dst pixels with the + * corresponding src pixels, performing any colortype/alphatype transformations needed + * (in the case where the src and dst have different colortypes or alphatypes). + * + * This call can fail, returning false, for several reasons: + * - If srcR does not intersect the bitmap bounds. + * - If the requested colortype/alphatype cannot be converted from the src's types. + * - If the src pixels are not available. + */ + bool readPixels(const SkImageInfo& dstInfo, void* dstPixels, size_t dstRowBytes, + int srcX, int srcY) const; + /** * Returns true if this bitmap's pixels can be converted into the requested * colorType, such that copyTo() could succeed. diff --git a/include/core/SkCanvas.h b/include/core/SkCanvas.h index a08e82800..31429f941 100644 --- a/include/core/SkCanvas.h +++ b/include/core/SkCanvas.h @@ -230,30 +230,31 @@ public: /** * Copy the pixels from the base-layer into the specified buffer (pixels + rowBytes), * converting them into the requested format (SkImageInfo). The base-layer pixels are read - * starting at the specified (x,y) location in the coordinate system of the base-layer. + * starting at the specified (srcX,srcY) location in the coordinate system of the base-layer. * - * The specified ImageInfo and (x,y) offset specifies a source rectangle + * The specified ImageInfo and (srcX,srcY) offset specifies a source rectangle * - * srcR(x, y, info.width(), info.height()); + * srcR.setXYWH(srcX, srcY, dstInfo.width(), dstInfo.height()); * - * SrcR is intersected with the bounds of the base-layer. If this intersection is not empty, - * then we have two sets of pixels (of equal size), the "src" specified by base-layer at (x,y) - * and the "dst" by info+pixels+rowBytes. Replace the dst pixels with the corresponding src - * pixels, performing any colortype/alphatype transformations needed (in the case where the - * src and dst have different colortypes or alphatypes). + * srcR is intersected with the bounds of the base-layer. If this intersection is not empty, + * then we have two sets of pixels (of equal size). Replace the dst pixels with the + * corresponding src pixels, performing any colortype/alphatype transformations needed + * (in the case where the src and dst have different colortypes or alphatypes). * * This call can fail, returning false, for several reasons: + * - If srcR does not intersect the base-layer bounds. * - If the requested colortype/alphatype cannot be converted from the base-layer's types. * - If this canvas is not backed by pixels (e.g. picture or PDF) */ - bool readPixels(const SkImageInfo&, void* pixels, size_t rowBytes, int x, int y); + bool readPixels(const SkImageInfo& dstInfo, void* dstPixels, size_t dstRowBytes, + int srcX, int srcY); /** * Helper for calling readPixels(info, ...). This call will check if bitmap has been allocated. * If not, it will attempt to call allocPixels(). If this fails, it will return false. If not, * it calls through to readPixels(info, ...) and returns its result. */ - bool readPixels(SkBitmap* bitmap, int x, int y); + bool readPixels(SkBitmap* bitmap, int srcX, int srcY); /** * Helper for allocating pixels and then calling readPixels(info, ...). The bitmap is resized diff --git a/src/core/SkBitmap.cpp b/src/core/SkBitmap.cpp index 789bf11b4..42254b518 100644 --- a/src/core/SkBitmap.cpp +++ b/src/core/SkBitmap.cpp @@ -873,11 +873,13 @@ bool SkBitmap::extractSubset(SkBitmap* result, const SkIRect& subset) const { #include "SkPaint.h" bool SkBitmap::canCopyTo(SkColorType dstColorType) const { - if (this->colorType() == kUnknown_SkColorType) { + const SkColorType srcCT = this->colorType(); + + if (srcCT == kUnknown_SkColorType) { return false; } - bool sameConfigs = (this->colorType() == dstColorType); + bool sameConfigs = (srcCT == dstColorType); switch (dstColorType) { case kAlpha_8_SkColorType: case kRGB_565_SkColorType: @@ -890,15 +892,66 @@ bool SkBitmap::canCopyTo(SkColorType dstColorType) const { } break; case kARGB_4444_SkColorType: - return sameConfigs || kN32_SkColorType == this->colorType(); + return sameConfigs || kN32_SkColorType == srcCT || kIndex_8_SkColorType == srcCT; default: return false; } return true; } -bool SkBitmap::copyTo(SkBitmap* dst, SkColorType dstColorType, - Allocator* alloc) const { +#include "SkConfig8888.h" + +bool SkBitmap::readPixels(const SkImageInfo& requestedDstInfo, void* dstPixels, size_t dstRB, + int x, int y) const { + if (kUnknown_SkColorType == requestedDstInfo.colorType()) { + return false; + } + if (NULL == dstPixels || dstRB < requestedDstInfo.minRowBytes()) { + return false; + } + if (0 == requestedDstInfo.width() || 0 == requestedDstInfo.height()) { + return false; + } + + SkIRect srcR = SkIRect::MakeXYWH(x, y, requestedDstInfo.width(), requestedDstInfo.height()); + if (!srcR.intersect(0, 0, this->width(), this->height())) { + return false; + } + + SkImageInfo dstInfo = requestedDstInfo; + // the intersect may have shrunk info's logical size + dstInfo.fWidth = srcR.width(); + dstInfo.fHeight = srcR.height(); + + // if x or y are negative, then we have to adjust pixels + if (x > 0) { + x = 0; + } + if (y > 0) { + y = 0; + } + // here x,y are either 0 or negative + dstPixels = ((char*)dstPixels - y * dstRB - x * dstInfo.bytesPerPixel()); + + ////////////// + + SkAutoLockPixels alp(*this); + + // since we don't stop creating un-pixeled devices yet, check for no pixels here + if (NULL == this->getPixels()) { + return false; + } + + SkImageInfo srcInfo = this->info(); + srcInfo.fWidth = dstInfo.width(); + srcInfo.fHeight = dstInfo.height(); + + const void* srcPixels = this->getAddr(srcR.x(), srcR.y()); + return SkPixelInfo::CopyPixels(dstInfo, dstPixels, dstRB, srcInfo, srcPixels, this->rowBytes(), + this->getColorTable()); +} + +bool SkBitmap::copyTo(SkBitmap* dst, SkColorType dstColorType, Allocator* alloc) const { if (!this->canCopyTo(dstColorType)) { return false; } @@ -971,59 +1024,21 @@ bool SkBitmap::copyTo(SkBitmap* dst, SkColorType dstColorType, // returned false. SkASSERT(tmpDst.pixelRef() != NULL); - /* do memcpy for the same configs cases, else use drawing - */ - if (src->colorType() == dstColorType) { - if (tmpDst.getSize() == src->getSize()) { - memcpy(tmpDst.getPixels(), src->getPixels(), src->getSafeSize()); + if (!src->readPixels(tmpDst.info(), tmpDst.getPixels(), tmpDst.rowBytes(), 0, 0)) { + return false; + } - SkPixelRef* dstPixelRef = tmpDst.pixelRef(); - if (dstPixelRef->info() == fPixelRef->info()) { - dstPixelRef->cloneGenID(*fPixelRef); - } - } else { - const char* srcP = reinterpret_cast(src->getPixels()); - char* dstP = reinterpret_cast(tmpDst.getPixels()); - // to be sure we don't read too much, only copy our logical pixels - size_t bytesToCopy = tmpDst.width() * tmpDst.bytesPerPixel(); - for (int y = 0; y < tmpDst.height(); y++) { - memcpy(dstP, srcP, bytesToCopy); - srcP += src->rowBytes(); - dstP += tmpDst.rowBytes(); - } - } - } else if (kARGB_4444_SkColorType == dstColorType - && kN32_SkColorType == src->colorType()) { - if (src->alphaType() == kUnpremul_SkAlphaType) { - // Our method for converting to 4444 assumes premultiplied. - return false; + // (for BitmapHeap) Clone the pixelref genID even though we have a new pixelref. + // The old copyTo impl did this, so we continue it for now. + // + // TODO: should we ignore rowbytes (i.e. getSize)? Then it could just be + // if (src_pixelref->info == dst_pixelref->info) + // + if (src->colorType() == dstColorType && tmpDst.getSize() == src->getSize()) { + SkPixelRef* dstPixelRef = tmpDst.pixelRef(); + if (dstPixelRef->info() == fPixelRef->info()) { + dstPixelRef->cloneGenID(*fPixelRef); } - SkASSERT(src->height() == tmpDst.height()); - SkASSERT(src->width() == tmpDst.width()); - for (int y = 0; y < src->height(); ++y) { - SkPMColor16* SK_RESTRICT dstRow = (SkPMColor16*) tmpDst.getAddr16(0, y); - SkPMColor* SK_RESTRICT srcRow = (SkPMColor*) src->getAddr32(0, y); - DITHER_4444_SCAN(y); - for (int x = 0; x < src->width(); ++x) { - dstRow[x] = SkDitherARGB32To4444(srcRow[x], - DITHER_VALUE(x)); - } - } - } else { - if (tmpDst.alphaType() == kUnpremul_SkAlphaType) { - // We do not support drawing to unpremultiplied bitmaps. - return false; - } - - // Always clear the dest in case one of the blitters accesses it - // TODO: switch the allocation of tmpDst to call sk_calloc_throw - tmpDst.eraseColor(SK_ColorTRANSPARENT); - - SkCanvas canvas(tmpDst); - SkPaint paint; - - paint.setDither(true); - canvas.drawBitmap(*src, 0, 0, &paint); } dst->swap(tmpDst); diff --git a/src/core/SkBitmapDevice.cpp b/src/core/SkBitmapDevice.cpp index 0f3cc2be5..1dfc3a6c0 100644 --- a/src/core/SkBitmapDevice.cpp +++ b/src/core/SkBitmapDevice.cpp @@ -155,59 +155,8 @@ void* SkBitmapDevice::onAccessPixels(SkImageInfo* info, size_t* rowBytes) { return NULL; } -static void rect_memcpy(void* dst, size_t dstRB, const void* src, size_t srcRB, size_t bytesPerRow, - int rowCount) { - SkASSERT(bytesPerRow <= srcRB); - SkASSERT(bytesPerRow <= dstRB); - for (int i = 0; i < rowCount; ++i) { - memcpy(dst, src, bytesPerRow); - dst = (char*)dst + dstRB; - src = (const char*)src + srcRB; - } -} - #include "SkConfig8888.h" -static bool copy_pixels(const SkImageInfo& dstInfo, void* dstPixels, size_t dstRowBytes, - const SkImageInfo& srcInfo, const void* srcPixels, size_t srcRowBytes) { - if (srcInfo.dimensions() != dstInfo.dimensions()) { - return false; - } - if (4 == srcInfo.bytesPerPixel() && 4 == dstInfo.bytesPerPixel()) { - SkDstPixelInfo dstPI; - dstPI.fColorType = dstInfo.colorType(); - dstPI.fAlphaType = dstInfo.alphaType(); - dstPI.fPixels = dstPixels; - dstPI.fRowBytes = dstRowBytes; - - SkSrcPixelInfo srcPI; - srcPI.fColorType = srcInfo.colorType(); - srcPI.fAlphaType = srcInfo.alphaType(); - srcPI.fPixels = srcPixels; - srcPI.fRowBytes = srcRowBytes; - - return srcPI.convertPixelsTo(&dstPI, srcInfo.width(), srcInfo.height()); - } - if (srcInfo.colorType() == dstInfo.colorType()) { - switch (srcInfo.colorType()) { - case kRGB_565_SkColorType: - case kAlpha_8_SkColorType: - break; - case kARGB_4444_SkColorType: - if (srcInfo.alphaType() != dstInfo.alphaType()) { - return false; - } - break; - default: - return false; - } - rect_memcpy(dstPixels, dstRowBytes, srcPixels, srcRowBytes, - srcInfo.width() * srcInfo.bytesPerPixel(), srcInfo.height()); - } - // TODO: add support for more conversions as needed - return false; -} - bool SkBitmapDevice::onWritePixels(const SkImageInfo& srcInfo, const void* srcPixels, size_t srcRowBytes, int x, int y) { // since we don't stop creating un-pixeled devices yet, check for no pixels here @@ -222,7 +171,7 @@ bool SkBitmapDevice::onWritePixels(const SkImageInfo& srcInfo, const void* srcPi void* dstPixels = fBitmap.getAddr(x, y); size_t dstRowBytes = fBitmap.rowBytes(); - if (copy_pixels(dstInfo, dstPixels, dstRowBytes, srcInfo, srcPixels, srcRowBytes)) { + if (SkPixelInfo::CopyPixels(dstInfo, dstPixels, dstRowBytes, srcInfo, srcPixels, srcRowBytes)) { fBitmap.notifyPixelsChanged(); return true; } @@ -231,28 +180,7 @@ bool SkBitmapDevice::onWritePixels(const SkImageInfo& srcInfo, const void* srcPi bool SkBitmapDevice::onReadPixels(const SkImageInfo& dstInfo, void* dstPixels, size_t dstRowBytes, int x, int y) { - // since we don't stop creating un-pixeled devices yet, check for no pixels here - if (NULL == fBitmap.getPixels()) { - return false; - } - - SkImageInfo srcInfo = fBitmap.info(); - - // perhaps can relax these in the future - if (4 != dstInfo.bytesPerPixel()) { - return false; - } - if (4 != srcInfo.bytesPerPixel()) { - return false; - } - - srcInfo.fWidth = dstInfo.width(); - srcInfo.fHeight = dstInfo.height(); - - const void* srcPixels = fBitmap.getAddr(x, y); - const size_t srcRowBytes = fBitmap.rowBytes(); - - return copy_pixels(dstInfo, dstPixels, dstRowBytes, srcInfo, srcPixels, srcRowBytes); + return fBitmap.readPixels(dstInfo, dstPixels, dstRowBytes, x, y); } /////////////////////////////////////////////////////////////////////////////// diff --git a/src/core/SkConfig8888.cpp b/src/core/SkConfig8888.cpp index 189309dae..b0572c054 100644 --- a/src/core/SkConfig8888.cpp +++ b/src/core/SkConfig8888.cpp @@ -1,5 +1,15 @@ +/* + * Copyright 2014 Google Inc. + * + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. + */ + +#include "SkBitmap.h" +#include "SkCanvas.h" #include "SkConfig8888.h" #include "SkColorPriv.h" +#include "SkDither.h" #include "SkMathPriv.h" #include "SkUnPreMultiply.h" @@ -115,3 +125,135 @@ bool SkSrcPixelInfo::convertPixelsTo(SkDstPixelInfo* dst, int width, int height) } return true; } + +static void rect_memcpy(void* dst, size_t dstRB, const void* src, size_t srcRB, size_t bytesPerRow, + int rowCount) { + SkASSERT(bytesPerRow <= srcRB); + SkASSERT(bytesPerRow <= dstRB); + for (int i = 0; i < rowCount; ++i) { + memcpy(dst, src, bytesPerRow); + dst = (char*)dst + dstRB; + src = (const char*)src + srcRB; + } +} + +bool SkPixelInfo::CopyPixels(const SkImageInfo& dstInfo, void* dstPixels, size_t dstRB, + const SkImageInfo& srcInfo, const void* srcPixels, size_t srcRB, + SkColorTable* ctable) { + if (srcInfo.dimensions() != dstInfo.dimensions()) { + return false; + } + + const int width = srcInfo.width(); + const int height = srcInfo.height(); + + // Handle fancy alpha swizzling if both are ARGB32 + if (4 == srcInfo.bytesPerPixel() && 4 == dstInfo.bytesPerPixel()) { + SkDstPixelInfo dstPI; + dstPI.fColorType = dstInfo.colorType(); + dstPI.fAlphaType = dstInfo.alphaType(); + dstPI.fPixels = dstPixels; + dstPI.fRowBytes = dstRB; + + SkSrcPixelInfo srcPI; + srcPI.fColorType = srcInfo.colorType(); + srcPI.fAlphaType = srcInfo.alphaType(); + srcPI.fPixels = srcPixels; + srcPI.fRowBytes = srcRB; + + return srcPI.convertPixelsTo(&dstPI, width, height); + } + + // If they agree on colorType and the alphaTypes are compatible, then we just memcpy. + // Note: we've already taken care of 32bit colortypes above. + if (srcInfo.colorType() == dstInfo.colorType()) { + switch (srcInfo.colorType()) { + case kRGB_565_SkColorType: + case kAlpha_8_SkColorType: + break; + case kIndex_8_SkColorType: + case kARGB_4444_SkColorType: + if (srcInfo.alphaType() != dstInfo.alphaType()) { + return false; + } + break; + default: + return false; + } + rect_memcpy(dstPixels, dstRB, srcPixels, srcRB, width * srcInfo.bytesPerPixel(), height); + return true; + } + + /* + * Begin section where we try to change colorTypes along the way. Not all combinations + * are supported. + */ + + // Can no longer draw directly into 4444, but we can manually whack it for a few combinations + if (kARGB_4444_SkColorType == dstInfo.colorType() && + (kN32_SkColorType == srcInfo.colorType() || kIndex_8_SkColorType == srcInfo.colorType())) { + if (srcInfo.alphaType() == kUnpremul_SkAlphaType) { + // Our method for converting to 4444 assumes premultiplied. + return false; + } + + const SkPMColor* table = NULL; + if (kIndex_8_SkColorType == srcInfo.colorType()) { + if (NULL == ctable) { + return false; + } + table = ctable->lockColors(); + } + + for (int y = 0; y < height; ++y) { + DITHER_4444_SCAN(y); + SkPMColor16* SK_RESTRICT dstRow = (SkPMColor16*)dstPixels; + if (table) { + const uint8_t* SK_RESTRICT srcRow = (const uint8_t*)srcPixels; + for (int x = 0; x < width; ++x) { + dstRow[x] = SkDitherARGB32To4444(table[srcRow[x]], DITHER_VALUE(x)); + } + } else { + const SkPMColor* SK_RESTRICT srcRow = (const SkPMColor*)srcPixels; + for (int x = 0; x < width; ++x) { + dstRow[x] = SkDitherARGB32To4444(srcRow[x], DITHER_VALUE(x)); + } + } + dstPixels = (char*)dstPixels + dstRB; + srcPixels = (const char*)srcPixels + srcRB; + } + + if (table) { + ctable->unlockColors(); + } + return true; + } + + if (dstInfo.alphaType() == kUnpremul_SkAlphaType) { + // We do not support drawing to unpremultiplied bitmaps. + return false; + } + + // Final fall-back, draw with a canvas + // + // Always clear the dest in case one of the blitters accesses it + // TODO: switch the allocation of tmpDst to call sk_calloc_throw + { + SkBitmap bm; + if (!bm.installPixels(srcInfo, const_cast(srcPixels), srcRB, ctable, NULL, NULL)) { + return false; + } + SkAutoTUnref canvas(SkCanvas::NewRasterDirect(dstInfo, dstPixels, dstRB)); + if (NULL == canvas.get()) { + return false; + } + + SkPaint paint; + paint.setDither(true); + + canvas->clear(0); + canvas->drawBitmap(bm, 0, 0, &paint); + return true; + } +} + diff --git a/src/core/SkConfig8888.h b/src/core/SkConfig8888.h index 97a3433ad..da0cbc668 100644 --- a/src/core/SkConfig8888.h +++ b/src/core/SkConfig8888.h @@ -14,6 +14,10 @@ struct SkPixelInfo { SkColorType fColorType; SkAlphaType fAlphaType; size_t fRowBytes; + + static bool CopyPixels(const SkImageInfo& dstInfo, void* dstPixels, size_t dstRowBytes, + const SkImageInfo& srcInfo, const void* srcPixels, size_t srcRowBytes, + SkColorTable* srcCTable = NULL); }; struct SkDstPixelInfo : SkPixelInfo { diff --git a/tests/BitmapCopyTest.cpp b/tests/BitmapCopyTest.cpp index 40cfbe0d5..4a49a6b48 100644 --- a/tests/BitmapCopyTest.cpp +++ b/tests/BitmapCopyTest.cpp @@ -185,7 +185,7 @@ static void writeCoordPixels(SkBitmap& bm, const Coordinates& coords) { static const Pair gPairs[] = { { kUnknown_SkColorType, "000000" }, { kAlpha_8_SkColorType, "010101" }, - { kIndex_8_SkColorType, "011101" }, + { kIndex_8_SkColorType, "011111" }, { kRGB_565_SkColorType, "010101" }, { kARGB_4444_SkColorType, "010111" }, { kN32_SkColorType, "010111" }, @@ -542,3 +542,96 @@ DEF_TEST(BitmapCopy, reporter) { } // for (size_t copyCase ... } } + +#include "SkColorPriv.h" +#include "SkUtils.h" + +/** + * Construct 4x4 pixels where we can look at a color and determine where it should be in the grid. + * alpha = 0xFF, blue = 0x80, red = x, green = y + */ +static void fill_4x4_pixels(SkPMColor colors[16]) { + for (int y = 0; y < 4; ++y) { + for (int x = 0; x < 4; ++x) { + colors[y*4+x] = SkPackARGB32(0xFF, x, y, 0x80); + } + } +} + +static bool check_4x4_pixel(SkPMColor color, unsigned x, unsigned y) { + SkASSERT(x < 4 && y < 4); + return 0xFF == SkGetPackedA32(color) && + x == SkGetPackedR32(color) && + y == SkGetPackedG32(color) && + 0x80 == SkGetPackedB32(color); +} + +/** + * Fill with all zeros, which will never match any value from fill_4x4_pixels + */ +static void clear_4x4_pixels(SkPMColor colors[16]) { + sk_memset32(colors, 0, 16); +} + +// Much of readPixels is exercised by copyTo testing, since readPixels is the backend for that +// method. Here we explicitly test subset copies. +// +DEF_TEST(BitmapReadPixels, reporter) { + const int W = 4; + const int H = 4; + const size_t rowBytes = W * sizeof(SkPMColor); + const SkImageInfo srcInfo = SkImageInfo::MakeN32Premul(W, H); + SkPMColor srcPixels[16]; + fill_4x4_pixels(srcPixels); + SkBitmap srcBM; + srcBM.installPixels(srcInfo, srcPixels, rowBytes); + + SkImageInfo dstInfo = SkImageInfo::MakeN32Premul(W, H); + SkPMColor dstPixels[16]; + + const struct { + bool fExpectedSuccess; + SkIPoint fRequestedSrcLoc; + SkISize fRequestedDstSize; + // If fExpectedSuccess, check these, otherwise ignore + SkIPoint fExpectedDstLoc; + SkIRect fExpectedSrcR; + } gRec[] = { + { true, { 0, 0 }, { 4, 4 }, { 0, 0 }, { 0, 0, 4, 4 } }, + { true, { 1, 1 }, { 2, 2 }, { 0, 0 }, { 1, 1, 3, 3 } }, + { true, { 2, 2 }, { 4, 4 }, { 0, 0 }, { 2, 2, 4, 4 } }, + { true, {-1,-1 }, { 2, 2 }, { 1, 1 }, { 0, 0, 1, 1 } }, + { false, {-1,-1 }, { 1, 1 }, { 0, 0 }, { 0, 0, 0, 0 } }, + }; + + for (size_t i = 0; i < SK_ARRAY_COUNT(gRec); ++i) { + clear_4x4_pixels(dstPixels); + + dstInfo.fWidth = gRec[i].fRequestedDstSize.width(); + dstInfo.fHeight = gRec[i].fRequestedDstSize.height(); + bool success = srcBM.readPixels(dstInfo, dstPixels, rowBytes, + gRec[i].fRequestedSrcLoc.x(), gRec[i].fRequestedSrcLoc.y()); + + REPORTER_ASSERT(reporter, gRec[i].fExpectedSuccess == success); + if (success) { + const SkIRect srcR = gRec[i].fExpectedSrcR; + const int dstX = gRec[i].fExpectedDstLoc.x(); + const int dstY = gRec[i].fExpectedDstLoc.y(); + // Walk the dst pixels, and check if we got what we expected + for (int y = 0; y < H; ++y) { + for (int x = 0; x < W; ++x) { + SkPMColor dstC = dstPixels[y*4+x]; + // get into src coordinates + int sx = x - dstX + srcR.x(); + int sy = y - dstY + srcR.y(); + if (srcR.contains(sx, sy)) { + REPORTER_ASSERT(reporter, check_4x4_pixel(dstC, sx, sy)); + } else { + REPORTER_ASSERT(reporter, 0 == dstC); + } + } + } + } + } +} + -- cgit v1.2.3 From 7f8c54cefefb855bb0d85d09ce5282ba7e9e352a Mon Sep 17 00:00:00 2001 From: Stephen White Date: Fri, 22 Aug 2014 11:35:47 -0400 Subject: Check all scratch texture allocations for image filters. [Cherry-picked from 673d9732bf37df724500e04afcdf27d5c711ef60 to chrome/m38_2125.] BUG=403677 TBR=bungeman@google.com Review URL: https://codereview.chromium.org/474443003 --- src/core/SkImageFilter.cpp | 3 +++ src/effects/SkDisplacementMapEffect.cpp | 3 +++ src/effects/SkMorphologyImageFilter.cpp | 6 ++++++ src/effects/SkXfermodeImageFilter.cpp | 3 +++ 4 files changed, 15 insertions(+) diff --git a/src/core/SkImageFilter.cpp b/src/core/SkImageFilter.cpp index cf1f07696..4a9a22e63 100644 --- a/src/core/SkImageFilter.cpp +++ b/src/core/SkImageFilter.cpp @@ -256,6 +256,9 @@ bool SkImageFilter::filterImageGPU(Proxy* proxy, const SkBitmap& src, const Cont desc.fConfig = kRGBA_8888_GrPixelConfig; GrAutoScratchTexture dst(context, desc); + if (NULL == dst.texture()) { + return false; + } GrContext::AutoMatrix am; am.setIdentity(context); GrContext::AutoRenderTarget art(context, dst.texture()->asRenderTarget()); diff --git a/src/effects/SkDisplacementMapEffect.cpp b/src/effects/SkDisplacementMapEffect.cpp index 26aaa022a..bae7ac098 100644 --- a/src/effects/SkDisplacementMapEffect.cpp +++ b/src/effects/SkDisplacementMapEffect.cpp @@ -389,6 +389,9 @@ bool SkDisplacementMapEffect::filterImageGPU(Proxy* proxy, const SkBitmap& src, desc.fConfig = kSkia8888_GrPixelConfig; GrAutoScratchTexture ast(context, desc); + if (NULL == ast.texture()) { + return false; + } SkAutoTUnref dst(ast.detach()); GrContext::AutoRenderTarget art(context, dst->asRenderTarget()); diff --git a/src/effects/SkMorphologyImageFilter.cpp b/src/effects/SkMorphologyImageFilter.cpp index 49df65431..67769f29b 100644 --- a/src/effects/SkMorphologyImageFilter.cpp +++ b/src/effects/SkMorphologyImageFilter.cpp @@ -506,6 +506,9 @@ bool apply_morphology(const SkBitmap& input, if (radius.fWidth > 0) { GrAutoScratchTexture ast(context, desc); + if (NULL == ast.texture()) { + return false; + } GrContext::AutoRenderTarget art(context, ast.texture()->asRenderTarget()); apply_morphology_pass(context, src, srcRect, dstRect, radius.fWidth, morphType, Gr1DKernelEffect::kX_Direction); @@ -519,6 +522,9 @@ bool apply_morphology(const SkBitmap& input, } if (radius.fHeight > 0) { GrAutoScratchTexture ast(context, desc); + if (NULL == ast.texture()) { + return false; + } GrContext::AutoRenderTarget art(context, ast.texture()->asRenderTarget()); apply_morphology_pass(context, src, srcRect, dstRect, radius.fHeight, morphType, Gr1DKernelEffect::kY_Direction); diff --git a/src/effects/SkXfermodeImageFilter.cpp b/src/effects/SkXfermodeImageFilter.cpp index cf32cf5cb..c876d5f6b 100644 --- a/src/effects/SkXfermodeImageFilter.cpp +++ b/src/effects/SkXfermodeImageFilter.cpp @@ -132,6 +132,9 @@ bool SkXfermodeImageFilter::filterImageGPU(Proxy* proxy, desc.fConfig = kSkia8888_GrPixelConfig; GrAutoScratchTexture ast(context, desc); + if (NULL == ast.texture()) { + return false; + } SkAutoTUnref dst(ast.detach()); GrContext::AutoRenderTarget art(context, dst->asRenderTarget()); -- cgit v1.2.3 From 1313b3f69370915a4fd911f97b4975d90bdf8869 Mon Sep 17 00:00:00 2001 From: jshin Date: Mon, 18 Aug 2014 08:07:51 -0700 Subject: Add alias mapping for Noto Sans CJK for ja/hans Chrome/Chromium OS are getting a brand new CJK fonts (Noto Sans CJK). We want them to be used in place of common Japanese and Simplified Chinese sans-serif fonts. BUG=chromium:399080 TEST=With CrOS CL (https://chromium-review.googlesource.com/#/c/212624/), web pages using 'Simhei' and 'MSP Gothic' are rendered with Noto Sans CJK. R=reed@chromium.org, bungeman@google.com Author: jshin@chromium.org Review URL: https://codereview.chromium.org/476203003 --- src/ports/SkFontConfigInterface_direct.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/ports/SkFontConfigInterface_direct.cpp b/src/ports/SkFontConfigInterface_direct.cpp index 10fbbcce1..6fff03bb7 100644 --- a/src/ports/SkFontConfigInterface_direct.cpp +++ b/src/ports/SkFontConfigInterface_direct.cpp @@ -221,6 +221,7 @@ FontEquivClass GetFontEquivClass(const char* fontname) { PGOTHIC, "MS PGothic" }, { PGOTHIC, "\xef\xbc\xad\xef\xbc\xb3 \xef\xbc\xb0" "\xe3\x82\xb4\xe3\x82\xb7\xe3\x83\x83\xe3\x82\xaf" }, + { PGOTHIC, "Noto Sans CJK Japanese" }, { PGOTHIC, "IPAPGothic" }, { PGOTHIC, "MotoyaG04Gothic" }, @@ -259,6 +260,7 @@ FontEquivClass GetFontEquivClass(const char* fontname) // 黑体 { SIMHEI, "Simhei" }, { SIMHEI, "\xe9\xbb\x91\xe4\xbd\x93" }, + { SIMHEI, "Noto Sans CJK Simplified Chinese" }, { SIMHEI, "MYingHeiGB18030" }, { SIMHEI, "MYingHeiB5HK" }, -- cgit v1.2.3 From 749e42acb26c37a2d11196fc9e80c93158e5bbda Mon Sep 17 00:00:00 2001 From: Stephen White Date: Tue, 26 Aug 2014 17:31:03 -0400 Subject: Fix saveLayer() with a pixel-moving filter vs SkBBoxHierarchyRecord / SkRecordDraw In SkBBoxHierarchyRecord: Since the bounds we pass to saveLayer are in the pre-filtering coordinate space, they aren't correct for determining the actual device pixels touched by the saveLayer in this case. The easiest fix for now is to pass the clip bounds, since the final draw done in restore() will never draw outside the clip. In SkRecordDraw: We do adjust the bounds passed to saveLayer, so we just need to make sure that when we're using a paint that may affect transparent black, we ignore the calculated bounds of draw ops and use the clip intersected with those adjusted bounds. See originally crrev.com/497773002 BUG=skia: R=reed@google.com, senorblanco@chromium.org, junov@chromium.org, mtklein@google.com Author: mtklein@chromium.org Review URL: https://codereview.chromium.org/496963003 (cherry picked from commit d910f544439fffa6c2bcc5181b79b2811a4c130a) Review URL: https://codereview.chromium.org/504423002 --- src/core/SkBBoxHierarchyRecord.cpp | 11 +++------ src/core/SkRecordDraw.cpp | 16 ++++++++++--- tests/ImageFilterTest.cpp | 48 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 11 deletions(-) diff --git a/src/core/SkBBoxHierarchyRecord.cpp b/src/core/SkBBoxHierarchyRecord.cpp index 1868e6532..8cdd1d961 100644 --- a/src/core/SkBBoxHierarchyRecord.cpp +++ b/src/core/SkBBoxHierarchyRecord.cpp @@ -41,14 +41,9 @@ SkCanvas::SaveLayerStrategy SkBBoxHierarchyRecord::willSaveLayer(const SkRect* b (NULL != paint->getColorFilter())); SkRect drawBounds; if (paintAffectsTransparentBlack) { - if (bounds) { - drawBounds = *bounds; - this->getTotalMatrix().mapRect(&drawBounds); - } else { - SkIRect deviceBounds; - this->getClipDeviceBounds(&deviceBounds); - drawBounds.set(deviceBounds); - } + SkIRect deviceBounds; + this->getClipDeviceBounds(&deviceBounds); + drawBounds.set(deviceBounds); } fStateTree->appendSaveLayer(this->writeStream().bytesWritten()); SkCanvas::SaveLayerStrategy strategy = this->INHERITED::willSaveLayer(bounds, paint, flags); diff --git a/src/core/SkRecordDraw.cpp b/src/core/SkRecordDraw.cpp index c9e029b8d..4ca398574 100644 --- a/src/core/SkRecordDraw.cpp +++ b/src/core/SkRecordDraw.cpp @@ -189,19 +189,29 @@ private: this->pushControl(); } + static bool PaintMayAffectTransparentBlack(const SkPaint* paint) { + // FIXME: this is very conservative + return paint && (paint->getImageFilter() || paint->getColorFilter()); + } + SkIRect popSaveBlock() { // We're done the Save block. Apply the block's bounds to all control ops inside it. SaveBounds sb; fSaveStack.pop(&sb); + + // If the paint affects transparent black, we can't trust any of our calculated bounds. + const SkIRect& bounds = + PaintMayAffectTransparentBlack(sb.paint) ? fCurrentClipBounds : sb.bounds; + while (sb.controlOps --> 0) { - this->popControl(sb.bounds); + this->popControl(bounds); } // This whole Save block may be part another Save block. - this->updateSaveBounds(sb.bounds); + this->updateSaveBounds(bounds); // If called from a real Restore (not a phony one for balance), it'll need the bounds. - return sb.bounds; + return bounds; } void pushControl() { diff --git a/tests/ImageFilterTest.cpp b/tests/ImageFilterTest.cpp index 4ee9f5dcb..9f6144bf2 100644 --- a/tests/ImageFilterTest.cpp +++ b/tests/ImageFilterTest.cpp @@ -408,6 +408,54 @@ DEF_TEST(ImageFilterDrawTiled, reporter) { } } +static void drawSaveLayerPicture(int width, int height, int tileSize, SkBBHFactory* factory, SkBitmap* result) { + + SkMatrix matrix; + matrix.setTranslate(SkIntToScalar(50), 0); + + SkAutoTUnref cf(SkColorFilter::CreateModeFilter(SK_ColorWHITE, SkXfermode::kSrc_Mode)); + SkAutoTUnref cfif(SkColorFilterImageFilter::Create(cf.get())); + SkAutoTUnref imageFilter(SkMatrixImageFilter::Create(matrix, SkPaint::kNone_FilterLevel, cfif.get())); + + SkPaint paint; + paint.setImageFilter(imageFilter.get()); + SkPictureRecorder recorder; + SkRect bounds = SkRect::Make(SkIRect::MakeXYWH(0, 0, 50, 50)); + SkCanvas* recordingCanvas = recorder.beginRecording(width, height, factory, 0); + recordingCanvas->translate(-55, 0); + recordingCanvas->saveLayer(&bounds, &paint); + recordingCanvas->restore(); + SkAutoTUnref picture1(recorder.endRecording()); + + result->allocN32Pixels(width, height); + SkCanvas canvas(*result); + canvas.clear(0); + canvas.clipRect(SkRect::Make(SkIRect::MakeWH(tileSize, tileSize))); + canvas.drawPicture(picture1.get()); +} + +DEF_TEST(ImageFilterDrawMatrixBBH, reporter) { + // Check that matrix filter when drawn tiled with BBH exactly + // matches the same thing drawn without BBH. + // Tests pass by not asserting. + + const int width = 200, height = 200; + const int tileSize = 100; + SkBitmap result1, result2; + SkRTreeFactory factory; + + drawSaveLayerPicture(width, height, tileSize, &factory, &result1); + drawSaveLayerPicture(width, height, tileSize, NULL, &result2); + + for (int y = 0; y < height; y++) { + int diffs = memcmp(result1.getAddr32(0, y), result2.getAddr32(0, y), result1.rowBytes()); + REPORTER_ASSERT(reporter, !diffs); + if (diffs) { + break; + } + } +} + static void drawBlurredRect(SkCanvas* canvas) { SkAutoTUnref filter(SkBlurImageFilter::Create(SkIntToScalar(8), 0)); SkPaint filterPaint; -- cgit v1.2.3 From c519a51b548d08614e86a06dd7c1a840dcf38316 Mon Sep 17 00:00:00 2001 From: Stephen White Date: Tue, 26 Aug 2014 18:12:08 -0400 Subject: Partially revert 749e42acb26c37a2d11196fc9e80c93158e5bbda. Revert the SkRecordDraw changes of my recent cherry-pick on the M38 branch, since they are breaking the build, and SkRecordDraw is not enabled in M38 anyway. BUG=407690 TBR=mtklein@chromium.org Review URL: https://codereview.chromium.org/507113002 --- src/core/SkRecordDraw.cpp | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/src/core/SkRecordDraw.cpp b/src/core/SkRecordDraw.cpp index 4ca398574..c9e029b8d 100644 --- a/src/core/SkRecordDraw.cpp +++ b/src/core/SkRecordDraw.cpp @@ -189,29 +189,19 @@ private: this->pushControl(); } - static bool PaintMayAffectTransparentBlack(const SkPaint* paint) { - // FIXME: this is very conservative - return paint && (paint->getImageFilter() || paint->getColorFilter()); - } - SkIRect popSaveBlock() { // We're done the Save block. Apply the block's bounds to all control ops inside it. SaveBounds sb; fSaveStack.pop(&sb); - - // If the paint affects transparent black, we can't trust any of our calculated bounds. - const SkIRect& bounds = - PaintMayAffectTransparentBlack(sb.paint) ? fCurrentClipBounds : sb.bounds; - while (sb.controlOps --> 0) { - this->popControl(bounds); + this->popControl(sb.bounds); } // This whole Save block may be part another Save block. - this->updateSaveBounds(bounds); + this->updateSaveBounds(sb.bounds); // If called from a real Restore (not a phony one for balance), it'll need the bounds. - return bounds; + return sb.bounds; } void pushControl() { -- cgit v1.2.3 From e82b9bc4fcdc4403995307c44a774b6d3ed89b76 Mon Sep 17 00:00:00 2001 From: Ben Wagner Date: Wed, 27 Aug 2014 19:17:41 -0400 Subject: DirectWrite to use aliased if ClearType is empty. Some CJK fonts with some versions of DirectWrite return valid data for bitmaps, but not for cleartype data. For reference, two screenshots. M37 Stable and then with this patch: http://imgur.com/9pf3rB9,EiTb6Li See https://code.google.com/p/chromium/issues/detail?id=396624#c10 for content of test html file. R=eae@chromium.org, reed@google.com, shrikant@chromium.org, bungeman@chromium.org, cpu@chromium.org BUG=chromium:407945 Review URL: https://codereview.chromium.org/504343007 --- src/core/SkGlyph.h | 2 ++ src/ports/SkScalerContext_win_dw.cpp | 62 ++++++++++++++++++++++++++++-------- src/ports/SkScalerContext_win_dw.h | 9 +++++- 3 files changed, 58 insertions(+), 15 deletions(-) diff --git a/src/core/SkGlyph.h b/src/core/SkGlyph.h index 5e0ac7ec0..4290bb889 100644 --- a/src/core/SkGlyph.h +++ b/src/core/SkGlyph.h @@ -32,6 +32,7 @@ struct SkGlyph { void* fDistanceField; uint8_t fMaskFormat; int8_t fRsbDelta, fLsbDelta; // used by auto-kerning + int8_t fForceBW; void init(uint32_t id) { fID = id; @@ -39,6 +40,7 @@ struct SkGlyph { fPath = NULL; fDistanceField = NULL; fMaskFormat = MASK_FORMAT_UNKNOWN; + fForceBW = 0; } /** diff --git a/src/ports/SkScalerContext_win_dw.cpp b/src/ports/SkScalerContext_win_dw.cpp index f27497be6..216c7dce2 100644 --- a/src/ports/SkScalerContext_win_dw.cpp +++ b/src/ports/SkScalerContext_win_dw.cpp @@ -398,11 +398,11 @@ void SkScalerContext_DW::generateAdvance(SkGlyph* glyph) { glyph->fAdvanceY = SkScalarToFixed(vecs[0].fY); } -void SkScalerContext_DW::generateMetrics(SkGlyph* glyph) { - glyph->fWidth = 0; - - this->generateAdvance(glyph); - +void SkScalerContext_DW::getBoundingBox(SkGlyph* glyph, + DWRITE_RENDERING_MODE renderingMode, + DWRITE_TEXTURE_TYPE textureType, + RECT* bbox) +{ //Measure raster size. fXform.dx = SkFixedToFloat(glyph->getSubXFixed()); fXform.dy = SkFixedToFloat(glyph->getSubYFixed()); @@ -430,16 +430,41 @@ void SkScalerContext_DW::generateMetrics(SkGlyph* glyph) { &run, 1.0f, // pixelsPerDip, &fXform, - fRenderingMode, + renderingMode, fMeasuringMode, 0.0f, // baselineOriginX, 0.0f, // baselineOriginY, &glyphRunAnalysis), "Could not create glyph run analysis."); - RECT bbox; - HRVM(glyphRunAnalysis->GetAlphaTextureBounds(fTextureType, &bbox), + HRVM(glyphRunAnalysis->GetAlphaTextureBounds(textureType, bbox), "Could not get texture bounds."); +} + +void SkScalerContext_DW::generateMetrics(SkGlyph* glyph) { + glyph->fWidth = 0; + + this->generateAdvance(glyph); + + RECT bbox; + this->getBoundingBox(glyph, fRenderingMode, fTextureType, &bbox); + + // GetAlphaTextureBounds succeeds but returns an empty RECT if there are no + // glyphs of the specified texture type. When this happens, try with the + // alternate texture type. + if (bbox.left == bbox.right || bbox.top == bbox.bottom) { + if (DWRITE_TEXTURE_CLEARTYPE_3x1 == fTextureType) { + this->getBoundingBox(glyph, + DWRITE_RENDERING_MODE_ALIASED, + DWRITE_TEXTURE_ALIASED_1x1, + &bbox); + if (bbox.left != bbox.right && bbox.top != bbox.bottom) { + glyph->fForceBW = 1; + } + } + // TODO: handle the case where a request for DWRITE_TEXTURE_ALIASED_1x1 + // fails, and try DWRITE_TEXTURE_CLEARTYPE_3x1. + } glyph->fWidth = SkToU16(bbox.right - bbox.left); glyph->fHeight = SkToU16(bbox.bottom - bbox.top); @@ -602,9 +627,12 @@ static void rgb_to_lcd32(const uint8_t* SK_RESTRICT src, const SkGlyph& glyph, } } -const void* SkScalerContext_DW::drawDWMask(const SkGlyph& glyph) { +const void* SkScalerContext_DW::drawDWMask(const SkGlyph& glyph, + DWRITE_RENDERING_MODE renderingMode, + DWRITE_TEXTURE_TYPE textureType) +{ int sizeNeeded = glyph.fWidth * glyph.fHeight; - if (DWRITE_RENDERING_MODE_ALIASED != fRenderingMode) { + if (DWRITE_RENDERING_MODE_ALIASED != renderingMode) { sizeNeeded *= 3; } if (sizeNeeded > fBits.count()) { @@ -639,7 +667,7 @@ const void* SkScalerContext_DW::drawDWMask(const SkGlyph& glyph) { HRNM(fTypeface->fFactory->CreateGlyphRunAnalysis(&run, 1.0f, // pixelsPerDip, &fXform, - fRenderingMode, + renderingMode, fMeasuringMode, 0.0f, // baselineOriginX, 0.0f, // baselineOriginY, @@ -653,7 +681,7 @@ const void* SkScalerContext_DW::drawDWMask(const SkGlyph& glyph) { bbox.top = glyph.fTop; bbox.right = glyph.fLeft + glyph.fWidth; bbox.bottom = glyph.fTop + glyph.fHeight; - HRNM(glyphRunAnalysis->CreateAlphaTexture(fTextureType, + HRNM(glyphRunAnalysis->CreateAlphaTexture(textureType, &bbox, fBits.begin(), sizeNeeded), @@ -663,7 +691,13 @@ const void* SkScalerContext_DW::drawDWMask(const SkGlyph& glyph) { void SkScalerContext_DW::generateImage(const SkGlyph& glyph) { //Create the mask. - const void* bits = this->drawDWMask(glyph); + DWRITE_RENDERING_MODE renderingMode = fRenderingMode; + DWRITE_TEXTURE_TYPE textureType = fTextureType; + if (glyph.fForceBW) { + renderingMode = DWRITE_RENDERING_MODE_ALIASED; + textureType = DWRITE_TEXTURE_ALIASED_1x1; + } + const void* bits = this->drawDWMask(glyph, renderingMode, textureType); if (!bits) { sk_bzero(glyph.fImage, glyph.computeImageSize()); return; @@ -671,7 +705,7 @@ void SkScalerContext_DW::generateImage(const SkGlyph& glyph) { //Copy the mask into the glyph. const uint8_t* src = (const uint8_t*)bits; - if (DWRITE_RENDERING_MODE_ALIASED == fRenderingMode) { + if (DWRITE_RENDERING_MODE_ALIASED == renderingMode) { bilevel_to_bw(src, glyph); const_cast(glyph).fMaskFormat = SkMask::kBW_Format; } else if (!isLCD(fRec)) { diff --git a/src/ports/SkScalerContext_win_dw.h b/src/ports/SkScalerContext_win_dw.h index e8eff0d6c..e9eab94de 100644 --- a/src/ports/SkScalerContext_win_dw.h +++ b/src/ports/SkScalerContext_win_dw.h @@ -33,7 +33,14 @@ protected: virtual void generateFontMetrics(SkPaint::FontMetrics*) SK_OVERRIDE; private: - const void* drawDWMask(const SkGlyph& glyph); + const void* drawDWMask(const SkGlyph& glyph, + DWRITE_RENDERING_MODE renderingMode, + DWRITE_TEXTURE_TYPE textureType); + + void getBoundingBox(SkGlyph* glyph, + DWRITE_RENDERING_MODE renderingMode, + DWRITE_TEXTURE_TYPE textureType, + RECT* bbox); SkTDArray fBits; /** The total matrix without the text height scale. */ -- cgit v1.2.3 From 57fe880c56133c26a9461375111351566de432c8 Mon Sep 17 00:00:00 2001 From: Justin Novosad Date: Fri, 5 Sep 2014 10:22:37 -0400 Subject: Fallback to moveTo when unable to find the first tangent in cubicTo When calling cubicTo(a, b, c) and if the distance between fPrevPt and a is too small, b is used instead of a to calculate the first tangent, even if the distance between fPrevPt and b is too small. In debug mode, this is causing an assertion to fail in SkPathStroker::preJoinTo() and, in Release, the use of an unitialized value. The first patch set is adding a failing test. The second one add the fix to SkPathStroker::cubicTo() BUG=skia:2820 R=bsalomon@chromium.org, junov@chromium.org, reed@google.com, caryclark@google.com, bsalomon@google.com Author: piotaixr@chromium.org Review URL: https://codereview.chromium.org/460813002 (cherry picked from commit fac4e0e83666ab59373169d6c157d3654cb479a3) Review URL: https://codereview.chromium.org/546823002 --- src/core/SkStroke.cpp | 3 ++- tests/PathTest.cpp | 27 +++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/core/SkStroke.cpp b/src/core/SkStroke.cpp index b138c326b..1e4ae79a5 100644 --- a/src/core/SkStroke.cpp +++ b/src/core/SkStroke.cpp @@ -424,7 +424,8 @@ void SkPathStroker::cubicTo(const SkPoint& pt1, const SkPoint& pt2, bool degenerateBC = SkPath::IsLineDegenerate(pt1, pt2); bool degenerateCD = SkPath::IsLineDegenerate(pt2, pt3); - if (degenerateAB + degenerateBC + degenerateCD >= 2) { + if (degenerateAB + degenerateBC + degenerateCD >= 2 + || (degenerateAB && SkPath::IsLineDegenerate(fPrevPt, pt2))) { this->lineTo(pt3); return; } diff --git a/tests/PathTest.cpp b/tests/PathTest.cpp index 3941ad6e1..0de64b06f 100644 --- a/tests/PathTest.cpp +++ b/tests/PathTest.cpp @@ -64,6 +64,32 @@ static void test_path_crbug364224() { canvas->drawPath(path, paint); } +/** + * In debug mode, this path was causing an assertion to fail in + * SkPathStroker::preJoinTo() and, in Release, the use of an unitialized value. + */ +static void make_path_crbugskia2820(SkPath* path, skiatest::Reporter* reporter) { + SkPoint orig, p1, p2, p3; + orig = SkPoint::Make(1.f, 1.f); + p1 = SkPoint::Make(1.f - SK_ScalarNearlyZero, 1.f); + p2 = SkPoint::Make(1.f, 1.f + SK_ScalarNearlyZero); + p3 = SkPoint::Make(2.f, 2.f); + + path->reset(); + path->moveTo(orig); + path->cubicTo(p1, p2, p3); + path->close(); +} + +static void test_path_crbugskia2820(skiatest::Reporter* reporter) {//GrContext* context) { + SkPath path; + make_path_crbugskia2820(&path, reporter); + + SkStrokeRec stroke(SkStrokeRec::kFill_InitStyle); + stroke.setStrokeStyle(2 * SK_Scalar1); + stroke.applyToPath(&path, path); +} + static void make_path0(SkPath* path) { // from * https://code.google.com/p/skia/issues/detail?id=1706 @@ -3553,4 +3579,5 @@ DEF_TEST(Paths, reporter) { PathTest_Private::TestPathTo(reporter); PathRefTest_Private::TestPathRef(reporter); test_dump(reporter); + test_path_crbugskia2820(reporter); } -- cgit v1.2.3 From 8fe1882de67035ff5f2028b062d47e8c28c33860 Mon Sep 17 00:00:00 2001 From: bungeman Date: Thu, 28 Aug 2014 11:42:29 -0700 Subject: Fix error handling in DirectWrite with tiny text. If there is an error while trying to determine the metrics, we need to bail instead of potentially using uninitialized data. R=reed@google.com, mtklein@google.com Author: bungeman@google.com Review URL: https://codereview.chromium.org/511783003 --- src/ports/SkScalerContext_win_dw.cpp | 86 ++++++++++++++++++++++-------------- src/ports/SkScalerContext_win_dw.h | 8 ++-- 2 files changed, 57 insertions(+), 37 deletions(-) diff --git a/src/ports/SkScalerContext_win_dw.cpp b/src/ports/SkScalerContext_win_dw.cpp index 216c7dce2..33ef0d55e 100644 --- a/src/ports/SkScalerContext_win_dw.cpp +++ b/src/ports/SkScalerContext_win_dw.cpp @@ -398,10 +398,10 @@ void SkScalerContext_DW::generateAdvance(SkGlyph* glyph) { glyph->fAdvanceY = SkScalarToFixed(vecs[0].fY); } -void SkScalerContext_DW::getBoundingBox(SkGlyph* glyph, - DWRITE_RENDERING_MODE renderingMode, - DWRITE_TEXTURE_TYPE textureType, - RECT* bbox) +HRESULT SkScalerContext_DW::getBoundingBox(SkGlyph* glyph, + DWRITE_RENDERING_MODE renderingMode, + DWRITE_TEXTURE_TYPE textureType, + RECT* bbox) { //Measure raster size. fXform.dx = SkFixedToFloat(glyph->getSubXFixed()); @@ -426,50 +426,70 @@ void SkScalerContext_DW::getBoundingBox(SkGlyph* glyph, run.glyphOffsets = &offset; SkTScopedComPtr glyphRunAnalysis; - HRVM(fTypeface->fFactory->CreateGlyphRunAnalysis( - &run, - 1.0f, // pixelsPerDip, - &fXform, - renderingMode, - fMeasuringMode, - 0.0f, // baselineOriginX, - 0.0f, // baselineOriginY, - &glyphRunAnalysis), - "Could not create glyph run analysis."); + HRM(fTypeface->fFactory->CreateGlyphRunAnalysis( + &run, + 1.0f, // pixelsPerDip, + &fXform, + renderingMode, + fMeasuringMode, + 0.0f, // baselineOriginX, + 0.0f, // baselineOriginY, + &glyphRunAnalysis), + "Could not create glyph run analysis."); + + HRM(glyphRunAnalysis->GetAlphaTextureBounds(textureType, bbox), + "Could not get texture bounds."); + + return S_OK; +} - HRVM(glyphRunAnalysis->GetAlphaTextureBounds(textureType, bbox), - "Could not get texture bounds."); +/** GetAlphaTextureBounds succeeds but sometimes returns empty bounds like + * { 0x80000000, 0x80000000, 0x80000000, 0x80000000 } + * for small, but not quite zero, sized glyphs. + * Only set as non-empty if the returned bounds are non-empty. + */ +static bool glyph_check_and_set_bounds(SkGlyph* glyph, const RECT& bbox) { + if (bbox.left >= bbox.right || bbox.top >= bbox.bottom) { + return false; + } + glyph->fWidth = SkToU16(bbox.right - bbox.left); + glyph->fHeight = SkToU16(bbox.bottom - bbox.top); + glyph->fLeft = SkToS16(bbox.left); + glyph->fTop = SkToS16(bbox.top); + return true; } void SkScalerContext_DW::generateMetrics(SkGlyph* glyph) { glyph->fWidth = 0; + glyph->fHeight = 0; + glyph->fLeft = 0; + glyph->fTop = 0; this->generateAdvance(glyph); RECT bbox; - this->getBoundingBox(glyph, fRenderingMode, fTextureType, &bbox); + HRVM(this->getBoundingBox(glyph, fRenderingMode, fTextureType, &bbox), + "Requested bounding box could not be determined."); + + if (glyph_check_and_set_bounds(glyph, bbox)) { + return; + } // GetAlphaTextureBounds succeeds but returns an empty RECT if there are no // glyphs of the specified texture type. When this happens, try with the // alternate texture type. - if (bbox.left == bbox.right || bbox.top == bbox.bottom) { - if (DWRITE_TEXTURE_CLEARTYPE_3x1 == fTextureType) { - this->getBoundingBox(glyph, - DWRITE_RENDERING_MODE_ALIASED, - DWRITE_TEXTURE_ALIASED_1x1, - &bbox); - if (bbox.left != bbox.right && bbox.top != bbox.bottom) { - glyph->fForceBW = 1; - } + if (DWRITE_TEXTURE_CLEARTYPE_3x1 == fTextureType) { + HRVM(this->getBoundingBox(glyph, + DWRITE_RENDERING_MODE_ALIASED, + DWRITE_TEXTURE_ALIASED_1x1, + &bbox), + "Fallback bounding box could not be determined."); + if (glyph_check_and_set_bounds(glyph, bbox)) { + glyph->fForceBW = 1; } - // TODO: handle the case where a request for DWRITE_TEXTURE_ALIASED_1x1 - // fails, and try DWRITE_TEXTURE_CLEARTYPE_3x1. } - - glyph->fWidth = SkToU16(bbox.right - bbox.left); - glyph->fHeight = SkToU16(bbox.bottom - bbox.top); - glyph->fLeft = SkToS16(bbox.left); - glyph->fTop = SkToS16(bbox.top); + // TODO: handle the case where a request for DWRITE_TEXTURE_ALIASED_1x1 + // fails, and try DWRITE_TEXTURE_CLEARTYPE_3x1. } void SkScalerContext_DW::generateFontMetrics(SkPaint::FontMetrics* metrics) { diff --git a/src/ports/SkScalerContext_win_dw.h b/src/ports/SkScalerContext_win_dw.h index e9eab94de..0bd79e7d8 100644 --- a/src/ports/SkScalerContext_win_dw.h +++ b/src/ports/SkScalerContext_win_dw.h @@ -37,10 +37,10 @@ private: DWRITE_RENDERING_MODE renderingMode, DWRITE_TEXTURE_TYPE textureType); - void getBoundingBox(SkGlyph* glyph, - DWRITE_RENDERING_MODE renderingMode, - DWRITE_TEXTURE_TYPE textureType, - RECT* bbox); + HRESULT getBoundingBox(SkGlyph* glyph, + DWRITE_RENDERING_MODE renderingMode, + DWRITE_TEXTURE_TYPE textureType, + RECT* bbox); SkTDArray fBits; /** The total matrix without the text height scale. */ -- cgit v1.2.3 From 773a327b12f653c3bb0630fee1b3ba9629f5717e Mon Sep 17 00:00:00 2001 From: Cary Clark Date: Wed, 10 Sep 2014 15:05:47 -0400 Subject: merge fix from trunk for issue 410552 fail when coincidence is too far apart TBR= BUG=410552 Review URL: https://codereview.chromium.org/558303002 --- src/pathops/SkOpContour.cpp | 37 +++++++++++++++++++------------------ src/pathops/SkOpSegment.cpp | 6 +++++- tests/PathOpsOpTest.cpp | 45 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 19 deletions(-) diff --git a/src/pathops/SkOpContour.cpp b/src/pathops/SkOpContour.cpp index e4dd62a65..467fab31f 100644 --- a/src/pathops/SkOpContour.cpp +++ b/src/pathops/SkOpContour.cpp @@ -57,6 +57,21 @@ SkOpSegment* SkOpContour::nonVerticalSegment(int* start, int* end) { return NULL; } +// if one is very large the smaller may have collapsed to nothing +static void bump_out_close_span(double* startTPtr, double* endTPtr) { + double startT = *startTPtr; + double endT = *endTPtr; + if (approximately_negative(endT - startT)) { + if (endT <= 1 - FLT_EPSILON) { + *endTPtr += FLT_EPSILON; + SkASSERT(*endTPtr <= 1); + } else { + *startTPtr -= FLT_EPSILON; + SkASSERT(*startTPtr >= 0); + } + } +} + // first pass, add missing T values // second pass, determine winding values of overlaps void SkOpContour::addCoincidentPoints() { @@ -82,15 +97,7 @@ void SkOpContour::addCoincidentPoints() { if ((cancelers = startSwapped = startT > endT)) { SkTSwap(startT, endT); } - if (startT == endT) { // if one is very large the smaller may have collapsed to nothing - if (endT <= 1 - FLT_EPSILON) { - endT += FLT_EPSILON; - SkASSERT(endT <= 1); - } else { - startT -= FLT_EPSILON; - SkASSERT(startT >= 0); - } - } + bump_out_close_span(&startT, &endT); SkASSERT(!approximately_negative(endT - startT)); double oStartT = coincidence.fTs[1][0]; double oEndT = coincidence.fTs[1][1]; @@ -98,6 +105,7 @@ void SkOpContour::addCoincidentPoints() { SkTSwap(oStartT, oEndT); cancelers ^= true; } + bump_out_close_span(&oStartT, &oEndT); SkASSERT(!approximately_negative(oEndT - oStartT)); const SkPoint& startPt = coincidence.fPts[0][startSwapped]; if (cancelers) { @@ -559,15 +567,7 @@ void SkOpContour::calcCommonCoincidentWinding(const SkCoincidence& coincidence) SkTSwap(startT, endT); SkTSwap(startPt, endPt); } - if (startT == endT) { // if span is very large, the smaller may have collapsed to nothing - if (endT <= 1 - FLT_EPSILON) { - endT += FLT_EPSILON; - SkASSERT(endT <= 1); - } else { - startT -= FLT_EPSILON; - SkASSERT(startT >= 0); - } - } + bump_out_close_span(&startT, &endT); SkASSERT(!approximately_negative(endT - startT)); double oStartT = coincidence.fTs[1][0]; double oEndT = coincidence.fTs[1][1]; @@ -575,6 +575,7 @@ void SkOpContour::calcCommonCoincidentWinding(const SkCoincidence& coincidence) SkTSwap(oStartT, oEndT); cancelers ^= true; } + bump_out_close_span(&oStartT, &oEndT); SkASSERT(!approximately_negative(oEndT - oStartT)); if (cancelers) { thisOne.addTCancel(*startPt, *endPt, &other); diff --git a/src/pathops/SkOpSegment.cpp b/src/pathops/SkOpSegment.cpp index 747cd9d49..336ac116b 100644 --- a/src/pathops/SkOpSegment.cpp +++ b/src/pathops/SkOpSegment.cpp @@ -1293,7 +1293,8 @@ void SkOpSegment::addTCoincident(const SkPoint& startPt, const SkPoint& endPt, d double testT = test->fT; SkOpSpan* oTest = &other->fTs[oIndex]; const SkPoint* oTestPt = &oTest->fPt; - SkASSERT(AlmostEqualUlps(*testPt, *oTestPt)); + // paths with extreme data will fail this test and eject out of pathops altogether later on + // SkASSERT(AlmostEqualUlps(*testPt, *oTestPt)); do { SkASSERT(test->fT < 1); SkASSERT(oTest->fT < 1); @@ -1476,6 +1477,9 @@ bool SkOpSegment::calcAngles() { const SkOpSpan* span = &fTs[0]; if (firstSpan->fT == 0 || span->fTiny || span->fOtherT != 1 || span->fOther->multipleEnds()) { index = findStartSpan(0); // curve start intersects + if (fTs[index].fT == 0) { + return false; + } SkASSERT(index > 0); if (activePrior >= 0) { addStartSpan(index); diff --git a/tests/PathOpsOpTest.cpp b/tests/PathOpsOpTest.cpp index 2c6744995..866b5b216 100644 --- a/tests/PathOpsOpTest.cpp +++ b/tests/PathOpsOpTest.cpp @@ -3771,7 +3771,52 @@ static void bufferOverflow(skiatest::Reporter* reporter, const char* filename) { testPathFailOp(reporter, path, pathB, kUnion_PathOp, filename); } +// m 100,0 60,170 -160,-110 200,0 -170,11000000000 z +static void fuzz433(skiatest::Reporter* reporter, const char* filename) { + SkPath path1, path2; + path1.moveTo(100,0); + path1.lineTo(60,170); + path1.lineTo(-160,-110); + path1.lineTo(200,0); + path1.lineTo(-170,11000000000.0f); + path1.close(); + + path2.moveTo(100 + 20,0 + 20); + path2.lineTo(60 + 20,170 + 20); + path2.lineTo(-160 + 20,-110 + 20); + path2.lineTo(200 + 20,0 + 20); + path2.lineTo(-170 + 20,11000000000.0f + 20); + path2.close(); + + testPathFailOp(reporter, path1, path2, kIntersect_PathOp, filename); +} + +static void fuzz433b(skiatest::Reporter* reporter, const char* filename) { + SkPath path1, path2; + path1.setFillType(SkPath::kEvenOdd_FillType); + path1.moveTo(140, 40); + path1.lineTo(200, 210); + path1.lineTo(40, 100); + path1.lineTo(240, 100); + path1.lineTo(70, 1.1e+10f); + path1.lineTo(140, 40); + path1.close(); + + path1.setFillType(SkPath::kWinding_FillType); + path2.moveTo(190, 60); + path2.lineTo(250, 230); + path2.lineTo(90, 120); + path2.lineTo(290, 120); + path2.lineTo(120, 1.1e+10f); + path2.lineTo(190, 60); + path2.close(); + + testPathFailOp(reporter, path1, path2, kUnion_PathOp, filename); +} + static struct TestDesc failTests[] = { + TEST(fuzz433b), + TEST(fuzz433), TEST(bufferOverflow), }; -- cgit v1.2.3 From 9eebe400dde5ddd8803b2a6a3a469a42d085b9a5 Mon Sep 17 00:00:00 2001 From: jshin Date: Tue, 9 Sep 2014 12:30:57 -0700 Subject: Update the alias mapping for Noto Sans CJK. To make the family names 'future-proof', we decided to shorten the name of Noto Sans CJK, which requires a change in the alias table in Skia. Chrome OS CL (actually updating the fonts) and Chrome CL (updating the font preferences on CrOS) are going together with this CL. BUG=412151 TEST=With the above two CLs in on Chrome OS, Noto Sans CJK {JP, SC} are used when MS P Gothic / Simhei are asked for by a web page. R=bungeman@google.com Author: jshin@chromium.org Review URL: https://codereview.chromium.org/554943002 Picked from: 7476cf533be14b84e83de1bf30db7eef889c1f23 --- src/ports/SkFontConfigInterface_direct.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ports/SkFontConfigInterface_direct.cpp b/src/ports/SkFontConfigInterface_direct.cpp index 6fff03bb7..039d8689e 100644 --- a/src/ports/SkFontConfigInterface_direct.cpp +++ b/src/ports/SkFontConfigInterface_direct.cpp @@ -221,7 +221,7 @@ FontEquivClass GetFontEquivClass(const char* fontname) { PGOTHIC, "MS PGothic" }, { PGOTHIC, "\xef\xbc\xad\xef\xbc\xb3 \xef\xbc\xb0" "\xe3\x82\xb4\xe3\x82\xb7\xe3\x83\x83\xe3\x82\xaf" }, - { PGOTHIC, "Noto Sans CJK Japanese" }, + { PGOTHIC, "Noto Sans CJK JP" }, { PGOTHIC, "IPAPGothic" }, { PGOTHIC, "MotoyaG04Gothic" }, @@ -260,7 +260,7 @@ FontEquivClass GetFontEquivClass(const char* fontname) // 黑体 { SIMHEI, "Simhei" }, { SIMHEI, "\xe9\xbb\x91\xe4\xbd\x93" }, - { SIMHEI, "Noto Sans CJK Simplified Chinese" }, + { SIMHEI, "Noto Sans CJK SC" }, { SIMHEI, "MYingHeiGB18030" }, { SIMHEI, "MYingHeiB5HK" }, -- cgit v1.2.3 From 20349133f46a35b35eb9e85d333d9c7cd570c7bd Mon Sep 17 00:00:00 2001 From: caryclark Date: Fri, 19 Sep 2014 06:33:31 -0700 Subject: fail early if coincidence can't be resolved Bail out if a very large value causes coincidence resolution to fail. TBR= BUG=415866 Author: caryclark@google.com Review URL: https://codereview.chromium.org/585913002 --- src/pathops/SkAddIntersections.cpp | 7 ++- src/pathops/SkAddIntersections.h | 2 +- src/pathops/SkOpContour.cpp | 27 ++++++----- src/pathops/SkOpContour.h | 4 +- src/pathops/SkOpSegment.cpp | 18 +++++--- src/pathops/SkOpSegment.h | 2 +- src/pathops/SkPathOpsCommon.cpp | 4 +- tests/PathOpsOpTest.cpp | 94 ++++++++++++++++++++++++++++++++++++++ 8 files changed, 133 insertions(+), 25 deletions(-) diff --git a/src/pathops/SkAddIntersections.cpp b/src/pathops/SkAddIntersections.cpp index 52e751bd0..27422eda5 100644 --- a/src/pathops/SkAddIntersections.cpp +++ b/src/pathops/SkAddIntersections.cpp @@ -434,7 +434,7 @@ void AddSelfIntersectTs(SkOpContour* test) { // resolve any coincident pairs found while intersecting, and // see if coincidence is formed by clipping non-concident segments -void CoincidenceCheck(SkTArray* contourList, int total) { +bool CoincidenceCheck(SkTArray* contourList, int total) { int contourCount = (*contourList).count(); for (int cIndex = 0; cIndex < contourCount; ++cIndex) { SkOpContour* contour = (*contourList)[cIndex]; @@ -446,10 +446,13 @@ void CoincidenceCheck(SkTArray* contourList, int total) { } for (int cIndex = 0; cIndex < contourCount; ++cIndex) { SkOpContour* contour = (*contourList)[cIndex]; - contour->calcCoincidentWinding(); + if (!contour->calcCoincidentWinding()) { + return false; + } } for (int cIndex = 0; cIndex < contourCount; ++cIndex) { SkOpContour* contour = (*contourList)[cIndex]; contour->calcPartialCoincidentWinding(); } + return true; } diff --git a/src/pathops/SkAddIntersections.h b/src/pathops/SkAddIntersections.h index 94ea436b7..4c1947b63 100644 --- a/src/pathops/SkAddIntersections.h +++ b/src/pathops/SkAddIntersections.h @@ -13,6 +13,6 @@ bool AddIntersectTs(SkOpContour* test, SkOpContour* next); void AddSelfIntersectTs(SkOpContour* test); -void CoincidenceCheck(SkTArray* contourList, int total); +bool CoincidenceCheck(SkTArray* contourList, int total); #endif diff --git a/src/pathops/SkOpContour.cpp b/src/pathops/SkOpContour.cpp index 467fab31f..6d6ad7926 100644 --- a/src/pathops/SkOpContour.cpp +++ b/src/pathops/SkOpContour.cpp @@ -267,7 +267,7 @@ bool SkOpContour::calcAngles() { return true; } -void SkOpContour::calcCoincidentWinding() { +bool SkOpContour::calcCoincidentWinding() { int count = fCoincidences.count(); #if DEBUG_CONCIDENT if (count > 0) { @@ -276,8 +276,11 @@ void SkOpContour::calcCoincidentWinding() { #endif for (int index = 0; index < count; ++index) { SkCoincidence& coincidence = fCoincidences[index]; - calcCommonCoincidentWinding(coincidence); + if (!calcCommonCoincidentWinding(coincidence)) { + return false; + } } + return true; } void SkOpContour::calcPartialCoincidentWinding() { @@ -471,11 +474,11 @@ void SkOpContour::checkCoincidentPair(const SkCoincidence& oneCoin, int oneIdx, addTo2->addTCancel(missingPt1, missingPt2, addOther2); } } else if (missingT1 >= 0) { - addTo1->addTCoincident(missingPt1, missingPt2, addTo1 == addTo2 ? missingT2 : otherT2, - addOther1); + SkAssertResult(addTo1->addTCoincident(missingPt1, missingPt2, + addTo1 == addTo2 ? missingT2 : otherT2, addOther1)); } else { - addTo2->addTCoincident(missingPt2, missingPt1, addTo2 == addTo1 ? missingT1 : otherT1, - addOther2); + SkAssertResult(addTo2->addTCoincident(missingPt2, missingPt1, + addTo2 == addTo1 ? missingT1 : otherT1, addOther2)); } } @@ -543,20 +546,20 @@ void SkOpContour::joinCoincidence(const SkTArray& coinciden } } -void SkOpContour::calcCommonCoincidentWinding(const SkCoincidence& coincidence) { +bool SkOpContour::calcCommonCoincidentWinding(const SkCoincidence& coincidence) { if (coincidence.fNearly[0] && coincidence.fNearly[1]) { - return; + return true; } int thisIndex = coincidence.fSegments[0]; SkOpSegment& thisOne = fSegments[thisIndex]; if (thisOne.done()) { - return; + return true; } SkOpContour* otherContour = coincidence.fOther; int otherIndex = coincidence.fSegments[1]; SkOpSegment& other = otherContour->fSegments[otherIndex]; if (other.done()) { - return; + return true; } double startT = coincidence.fTs[0][0]; double endT = coincidence.fTs[0][1]; @@ -577,15 +580,17 @@ void SkOpContour::calcCommonCoincidentWinding(const SkCoincidence& coincidence) } bump_out_close_span(&oStartT, &oEndT); SkASSERT(!approximately_negative(oEndT - oStartT)); + bool success = true; if (cancelers) { thisOne.addTCancel(*startPt, *endPt, &other); } else { - thisOne.addTCoincident(*startPt, *endPt, endT, &other); + success = thisOne.addTCoincident(*startPt, *endPt, endT, &other); } #if DEBUG_CONCIDENT thisOne.debugShowTs("p"); other.debugShowTs("o"); #endif + return success; } void SkOpContour::resolveNearCoincidence() { diff --git a/src/pathops/SkOpContour.h b/src/pathops/SkOpContour.h index d1b3cd017..899367ab0 100644 --- a/src/pathops/SkOpContour.h +++ b/src/pathops/SkOpContour.h @@ -114,7 +114,7 @@ public: } bool calcAngles(); - void calcCoincidentWinding(); + bool calcCoincidentWinding(); void calcPartialCoincidentWinding(); void checkDuplicates() { @@ -325,7 +325,7 @@ public: private: void alignPt(int index, SkPoint* point, int zeroPt) const; int alignT(bool swap, int tIndex, SkIntersections* ts) const; - void calcCommonCoincidentWinding(const SkCoincidence& ); + bool calcCommonCoincidentWinding(const SkCoincidence& ); void checkCoincidentPair(const SkCoincidence& oneCoin, int oneIdx, const SkCoincidence& twoCoin, int twoIdx, bool partial); void joinCoincidence(const SkTArray& , bool partial); diff --git a/src/pathops/SkOpSegment.cpp b/src/pathops/SkOpSegment.cpp index 336ac116b..5304e4b0e 100644 --- a/src/pathops/SkOpSegment.cpp +++ b/src/pathops/SkOpSegment.cpp @@ -1265,7 +1265,7 @@ void SkOpSegment::bumpCoincidentOther(const SkOpSpan& test, int* oIndexPtr, // set spans from start to end to increment the greater by one and decrement // the lesser -void SkOpSegment::addTCoincident(const SkPoint& startPt, const SkPoint& endPt, double endT, +bool SkOpSegment::addTCoincident(const SkPoint& startPt, const SkPoint& endPt, double endT, SkOpSegment* other) { bool binary = fOperand != other->fOperand; int index = 0; @@ -1297,7 +1297,10 @@ void SkOpSegment::addTCoincident(const SkPoint& startPt, const SkPoint& endPt, d // SkASSERT(AlmostEqualUlps(*testPt, *oTestPt)); do { SkASSERT(test->fT < 1); - SkASSERT(oTest->fT < 1); + if (oTest->fT == 1) { + // paths with extreme data may be so mismatched that we fail here + return false; + } // consolidate the winding count even if done if ((test->fWindValue == 0 && test->fOppValue == 0) @@ -1403,6 +1406,7 @@ void SkOpSegment::addTCoincident(const SkPoint& startPt, const SkPoint& endPt, d } setCoincidentRange(startPt, endPt, other); other->setCoincidentRange(startPt, endPt, this); + return true; } // FIXME: this doesn't prevent the same span from being added twice @@ -2385,8 +2389,8 @@ nextSmallCheck: do { ++nextSpan; } while (nextSpan->fSmall); - missing.fSegment->addTCoincident(missing.fPt, nextSpan->fPt, nextSpan->fT, - missingOther); + SkAssertResult(missing.fSegment->addTCoincident(missing.fPt, nextSpan->fPt, + nextSpan->fT, missingOther)); } else if (otherSpan.fT > 0) { const SkOpSpan* priorSpan = &otherSpan; do { @@ -2457,7 +2461,7 @@ void SkOpSegment::checkSmallCoincidence(const SkOpSpan& span, } SkASSERT(oSpan.fSmall); if (oStartIndex < oEndIndex) { - addTCoincident(span.fPt, next->fPt, next->fT, other); + SkAssertResult(addTCoincident(span.fPt, next->fPt, next->fT, other)); } else { addTCancel(span.fPt, next->fPt, other); } @@ -2502,7 +2506,7 @@ void SkOpSegment::checkSmallCoincidence(const SkOpSpan& span, oTest->fOtherT, tTest->fT); #endif if (tTest->fT < oTest->fOtherT) { - addTCoincident(span.fPt, next->fPt, next->fT, testOther); + SkAssertResult(addTCoincident(span.fPt, next->fPt, next->fT, testOther)); } else { addTCancel(span.fPt, next->fPt, testOther); } @@ -3390,7 +3394,7 @@ bool SkOpSegment::joinCoincidence(SkOpSegment* other, double otherT, const SkPoi if (cancel) { match->addTCancel(startPt, endPt, other); } else { - match->addTCoincident(startPt, endPt, endT, other); + SkAssertResult(match->addTCoincident(startPt, endPt, endT, other)); } return true; } diff --git a/src/pathops/SkOpSegment.h b/src/pathops/SkOpSegment.h index df87d058b..24d08bd81 100644 --- a/src/pathops/SkOpSegment.h +++ b/src/pathops/SkOpSegment.h @@ -284,7 +284,7 @@ public: void addStartSpan(int endIndex); int addT(SkOpSegment* other, const SkPoint& pt, double newT); void addTCancel(const SkPoint& startPt, const SkPoint& endPt, SkOpSegment* other); - void addTCoincident(const SkPoint& startPt, const SkPoint& endPt, double endT, + bool addTCoincident(const SkPoint& startPt, const SkPoint& endPt, double endT, SkOpSegment* other); const SkOpSpan* addTPair(double t, SkOpSegment* other, double otherT, bool borrowWind, const SkPoint& pt); diff --git a/src/pathops/SkPathOpsCommon.cpp b/src/pathops/SkPathOpsCommon.cpp index 9a8a2cf4e..f7b7273a8 100644 --- a/src/pathops/SkPathOpsCommon.cpp +++ b/src/pathops/SkPathOpsCommon.cpp @@ -699,7 +699,9 @@ bool HandleCoincidence(SkTArray* contourList, int total) { #if DEBUG_SHOW_WINDING SkOpContour::debugShowWindingValues(contourList); #endif - CoincidenceCheck(contourList, total); + if (!CoincidenceCheck(contourList, total)) { + return false; + } #if DEBUG_SHOW_WINDING SkOpContour::debugShowWindingValues(contourList); #endif diff --git a/tests/PathOpsOpTest.cpp b/tests/PathOpsOpTest.cpp index 866b5b216..84485c1cf 100644 --- a/tests/PathOpsOpTest.cpp +++ b/tests/PathOpsOpTest.cpp @@ -3814,7 +3814,101 @@ static void fuzz433b(skiatest::Reporter* reporter, const char* filename) { testPathFailOp(reporter, path1, path2, kUnion_PathOp, filename); } +static void fuzz487a(skiatest::Reporter* reporter, const char* filename) { + SkPath path; + path.setFillType((SkPath::FillType) 0); +path.moveTo(SkBits2Float(0x432c8000), SkBits2Float(0x42c00000)); +path.lineTo(SkBits2Float(0x4309999a), SkBits2Float(0x42c00000)); +path.cubicTo(SkBits2Float(0x4309999a), SkBits2Float(0x429a6666), SkBits2Float(0x42f9999a), SkBits2Float(0x4275999a), SkBits2Float(0x42d70001), SkBits2Float(0x42633333)); +path.lineTo(SkBits2Float(0x42e90001), SkBits2Float(0x41b8cccc)); +path.cubicTo(SkBits2Float(0x42dc6667), SkBits2Float(0x41ab3332), SkBits2Float(0x42cf3334), SkBits2Float(0x41a3ffff), SkBits2Float(0x42c20001), SkBits2Float(0x41a3ffff)); +path.lineTo(SkBits2Float(0x42c20001), SkBits2Float(0x425d999a)); +path.lineTo(SkBits2Float(0x42c20001), SkBits2Float(0x425d999a)); +path.cubicTo(SkBits2Float(0x429c6668), SkBits2Float(0x425d999a), SkBits2Float(0x4279999c), SkBits2Float(0x42886667), SkBits2Float(0x42673335), SkBits2Float(0x42ab0000)); +path.lineTo(SkBits2Float(0x41c0ccd0), SkBits2Float(0x42990000)); +path.cubicTo(SkBits2Float(0x41b33336), SkBits2Float(0x42a5999a), SkBits2Float(0x41ac0003), SkBits2Float(0x42b2cccd), SkBits2Float(0x41ac0003), SkBits2Float(0x42c00000)); +path.lineTo(SkBits2Float(0x4261999c), SkBits2Float(0x42c00000)); +path.lineTo(SkBits2Float(0x4261999c), SkBits2Float(0x42c00000)); +path.cubicTo(SkBits2Float(0x4261999c), SkBits2Float(0x434d3333), SkBits2Float(0x4364e667), SkBits2Float(0x4346b333), SkBits2Float(0x4364e667), SkBits2Float(0x43400000)); +path.lineTo(SkBits2Float(0x432c8000), SkBits2Float(0x42c00000)); +path.close(); + + SkPath path1(path); + path.reset(); + path.setFillType((SkPath::FillType) 0); +path.moveTo(SkBits2Float(0x432c8000), SkBits2Float(0x42c00000)); +path.lineTo(SkBits2Float(0x4309999a), SkBits2Float(0x42c00000)); +path.cubicTo(SkBits2Float(0x4309999a), SkBits2Float(0x42a20000), SkBits2Float(0x43016667), SkBits2Float(0x4287cccd), SkBits2Float(0x42ea999a), SkBits2Float(0x4273999a)); +path.lineTo(SkBits2Float(0x4306cccd), SkBits2Float(0x41f5999a)); +path.cubicTo(SkBits2Float(0x42f76667), SkBits2Float(0x41c26667), SkBits2Float(0x42dd999a), SkBits2Float(0x41a4cccd), SkBits2Float(0x42c23334), SkBits2Float(0x41a4cccd)); +path.lineTo(SkBits2Float(0x42c23334), SkBits2Float(0x425e0000)); +path.cubicTo(SkBits2Float(0x42a43334), SkBits2Float(0x425e0000), SkBits2Float(0x428a0001), SkBits2Float(0x427ecccd), SkBits2Float(0x42780002), SkBits2Float(0x4297999a)); +path.lineTo(SkBits2Float(0x41fccccd), SkBits2Float(0x42693333)); +path.cubicTo(SkBits2Float(0x41c9999a), SkBits2Float(0x428acccd), SkBits2Float(0x41ac0000), SkBits2Float(0x42a4999a), SkBits2Float(0x41ac0000), SkBits2Float(0x42c00000)); +path.lineTo(SkBits2Float(0x4261999a), SkBits2Float(0x42c00000)); +path.cubicTo(SkBits2Float(0x4261999a), SkBits2Float(0x42de0000), SkBits2Float(0x42813333), SkBits2Float(0x42f83333), SkBits2Float(0x42996666), SkBits2Float(0x4303199a)); +path.cubicTo(SkBits2Float(0x4272cccc), SkBits2Float(0x4303199a), SkBits2Float(0x423d3332), SkBits2Float(0x430de667), SkBits2Float(0x422d9999), SkBits2Float(0x431cb334)); +path.lineTo(SkBits2Float(0x7086a1dc), SkBits2Float(0x42eecccd)); +path.lineTo(SkBits2Float(0x41eb3333), SkBits2Float(0xc12ccccd)); +path.lineTo(SkBits2Float(0x42053333), SkBits2Float(0xc1cccccd)); +path.lineTo(SkBits2Float(0x42780000), SkBits2Float(0xc18f3334)); +path.cubicTo(SkBits2Float(0x43206666), SkBits2Float(0x43134ccd), SkBits2Float(0x43213333), SkBits2Float(0x430db333), SkBits2Float(0x43213333), SkBits2Float(0x43080000)); +path.lineTo(SkBits2Float(0x432c8000), SkBits2Float(0x42c00000)); +path.close(); + + SkPath path2(path); + testPathFailOp(reporter, path1, path2, (SkPathOp) 2, filename); +} + +static void fuzz487b(skiatest::Reporter* reporter, const char* filename) { + SkPath path; + path.setFillType((SkPath::FillType) 0); +path.moveTo(SkBits2Float(0x432c8000), SkBits2Float(0x42c00000)); +path.lineTo(SkBits2Float(0x4309999a), SkBits2Float(0x42c00000)); +path.cubicTo(SkBits2Float(0x4309999a), SkBits2Float(0x429a6666), SkBits2Float(0x42f9999a), SkBits2Float(0x4275999a), SkBits2Float(0x42d70001), SkBits2Float(0x42633333)); +path.lineTo(SkBits2Float(0x42e90001), SkBits2Float(0x41b8cccc)); +path.cubicTo(SkBits2Float(0x42dc6667), SkBits2Float(0x41ab3332), SkBits2Float(0x42cf3334), SkBits2Float(0x41a3ffff), SkBits2Float(0x42c20001), SkBits2Float(0x41a3ffff)); +path.lineTo(SkBits2Float(0x42c20001), SkBits2Float(0x425d999a)); +path.lineTo(SkBits2Float(0x42c20001), SkBits2Float(0x425d999a)); +path.cubicTo(SkBits2Float(0x429c6668), SkBits2Float(0x425d999a), SkBits2Float(0x4279999c), SkBits2Float(0x42886667), SkBits2Float(0x42673335), SkBits2Float(0x42ab0000)); +path.lineTo(SkBits2Float(0x41c0ccd0), SkBits2Float(0x42990000)); +path.cubicTo(SkBits2Float(0x41b33336), SkBits2Float(0x42a5999a), SkBits2Float(0x41ac0003), SkBits2Float(0x42b2cccd), SkBits2Float(0x41ac0003), SkBits2Float(0x42c00000)); +path.lineTo(SkBits2Float(0x4261999c), SkBits2Float(0x42c00000)); +path.lineTo(SkBits2Float(0x4261999c), SkBits2Float(0x42c00000)); +path.cubicTo(SkBits2Float(0x4261999c), SkBits2Float(0x434d3333), SkBits2Float(0x4364e667), SkBits2Float(0x4346b333), SkBits2Float(0x4364e667), SkBits2Float(0x43400000)); +path.lineTo(SkBits2Float(0x432c8000), SkBits2Float(0x42c00000)); +path.close(); + + SkPath path1(path); + path.reset(); + path.setFillType((SkPath::FillType) 0); +path.moveTo(SkBits2Float(0x432c8000), SkBits2Float(0x42c00000)); +path.lineTo(SkBits2Float(0x4309999a), SkBits2Float(0x42c00000)); +path.cubicTo(SkBits2Float(0x4309999a), SkBits2Float(0x42a20000), SkBits2Float(0x43016667), SkBits2Float(0x4287cccd), SkBits2Float(0x42ea999a), SkBits2Float(0x4273999a)); +path.lineTo(SkBits2Float(0x4306cccd), SkBits2Float(0x41f5999a)); +path.cubicTo(SkBits2Float(0x42f76667), SkBits2Float(0x41c26667), SkBits2Float(0x42dd999a), SkBits2Float(0x41a4cccd), SkBits2Float(0x42c23334), SkBits2Float(0x41a4cccd)); +path.lineTo(SkBits2Float(0x42c23334), SkBits2Float(0x425e0000)); +path.cubicTo(SkBits2Float(0x42a43334), SkBits2Float(0x425e0000), SkBits2Float(0x428a0001), SkBits2Float(0x427ecccd), SkBits2Float(0x42780002), SkBits2Float(0x4297999a)); +path.lineTo(SkBits2Float(0x41fccccd), SkBits2Float(0x42693333)); +path.cubicTo(SkBits2Float(0x41c9999a), SkBits2Float(0x428acccd), SkBits2Float(0x41ac0000), SkBits2Float(0x42a4999a), SkBits2Float(0x41ac0000), SkBits2Float(0x42c00000)); +path.lineTo(SkBits2Float(0x4261999a), SkBits2Float(0x42c00000)); +path.cubicTo(SkBits2Float(0x4261999a), SkBits2Float(0x42de0000), SkBits2Float(0x42813333), SkBits2Float(0x42f83333), SkBits2Float(0x42996666), SkBits2Float(0x4303199a)); +path.cubicTo(SkBits2Float(0x4272cccc), SkBits2Float(0x4303199a), SkBits2Float(0x423d3332), SkBits2Float(0x430de667), SkBits2Float(0x422d9999), SkBits2Float(0x431cb334)); +path.lineTo(SkBits2Float(0x7086a1dc), SkBits2Float(0x42eecccd)); +path.lineTo(SkBits2Float(0x41eb3333), SkBits2Float(0xc12ccccd)); +path.lineTo(SkBits2Float(0x42053333), SkBits2Float(0xc1cccccd)); +path.lineTo(SkBits2Float(0x42780000), SkBits2Float(0xc18f3334)); +path.cubicTo(SkBits2Float(0x43206666), SkBits2Float(0x43134ccd), SkBits2Float(0x43213333), SkBits2Float(0x430db333), SkBits2Float(0x43213333), SkBits2Float(0x43080000)); +path.lineTo(SkBits2Float(0x432c8000), SkBits2Float(0x42c00000)); +path.close(); + + SkPath path2(path); + testPathFailOp(reporter, path1, path2, (SkPathOp) 2, filename); +} + static struct TestDesc failTests[] = { + TEST(fuzz487a), + TEST(fuzz487b), TEST(fuzz433b), TEST(fuzz433), TEST(bufferOverflow), -- cgit v1.2.3 From a93c327924efbe88e1ab88197c34b2b0d4c09da1 Mon Sep 17 00:00:00 2001 From: Robert Phillips Date: Wed, 24 Sep 2014 08:15:49 -0400 Subject: Fix convexicator bug BUG=crbug.com/412640 R=caryclark@google.com, bsalomon@google.com Author: robertphillips@google.com Review URL: https://codereview.chromium.org/573763002 Review URL: https://codereview.chromium.org/597963002 --- src/core/SkPath.cpp | 74 +++++++++++++++++++++++++++++++++++++++++------------ tests/PathTest.cpp | 11 ++++++++ 2 files changed, 69 insertions(+), 16 deletions(-) diff --git a/src/core/SkPath.cpp b/src/core/SkPath.cpp index aa7943cf5..457ad9df0 100644 --- a/src/core/SkPath.cpp +++ b/src/core/SkPath.cpp @@ -2132,6 +2132,16 @@ void SkPath::validate() const { static int sign(SkScalar x) { return x < 0; } #define kValueNeverReturnedBySign 2 +enum DirChange { + kLeft_DirChange, + kRight_DirChange, + kStraight_DirChange, + kBackwards_DirChange, + + kInvalid_DirChange +}; + + static bool almost_equal(SkScalar compA, SkScalar compB) { // The error epsilon was empirically derived; worse case round rects // with a mid point outset by 2x float epsilon in tests had an error @@ -2146,13 +2156,37 @@ static bool almost_equal(SkScalar compA, SkScalar compB) { return aBits < bBits + epsilon && bBits < aBits + epsilon; } +static DirChange direction_change(const SkPoint& lastPt, const SkVector& curPt, + const SkVector& lastVec, const SkVector& curVec) { + SkScalar cross = SkPoint::CrossProduct(lastVec, curVec); + + SkScalar smallest = SkTMin(curPt.fX, SkTMin(curPt.fY, SkTMin(lastPt.fX, lastPt.fY))); + SkScalar largest = SkTMax(curPt.fX, SkTMax(curPt.fY, SkTMax(lastPt.fX, lastPt.fY))); + largest = SkTMax(largest, -smallest); + + if (!almost_equal(largest, largest + cross)) { + int sign = SkScalarSignAsInt(cross); + if (sign) { + return (1 == sign) ? kRight_DirChange : kLeft_DirChange; + } + } + + if (!SkScalarNearlyZero(lastVec.lengthSqd(), SK_ScalarNearlyZero*SK_ScalarNearlyZero) && + !SkScalarNearlyZero(curVec.lengthSqd(), SK_ScalarNearlyZero*SK_ScalarNearlyZero) && + lastVec.dot(curVec) < 0.0f) { + return kBackwards_DirChange; + } + + return kStraight_DirChange; +} + // only valid for a single contour struct Convexicator { Convexicator() : fPtCount(0) , fConvexity(SkPath::kConvex_Convexity) , fDirection(SkPath::kUnknown_Direction) { - fSign = 0; + fExpectedDir = kInvalid_DirChange; // warnings fLastPt.set(0, 0); fCurrPt.set(0, 0); @@ -2211,20 +2245,28 @@ struct Convexicator { private: void addVec(const SkVector& vec) { SkASSERT(vec.fX || vec.fY); - SkScalar cross = SkPoint::CrossProduct(fLastVec, vec); - SkScalar smallest = SkTMin(fCurrPt.fX, SkTMin(fCurrPt.fY, SkTMin(fLastPt.fX, fLastPt.fY))); - SkScalar largest = SkTMax(fCurrPt.fX, SkTMax(fCurrPt.fY, SkTMax(fLastPt.fX, fLastPt.fY))); - largest = SkTMax(largest, -smallest); - if (!almost_equal(largest, largest + cross)) { - int sign = SkScalarSignAsInt(cross); - if (0 == fSign) { - fSign = sign; - fDirection = (1 == sign) ? SkPath::kCW_Direction : SkPath::kCCW_Direction; - } else if (sign && fSign != sign) { - fConvexity = SkPath::kConcave_Convexity; - fDirection = SkPath::kUnknown_Direction; - } - fLastVec = vec; + DirChange dir = direction_change(fLastPt, fCurrPt, fLastVec, vec); + switch (dir) { + case kLeft_DirChange: // fall through + case kRight_DirChange: + if (kInvalid_DirChange == fExpectedDir) { + fExpectedDir = dir; + fDirection = (kRight_DirChange == dir) ? SkPath::kCW_Direction + : SkPath::kCCW_Direction; + } else if (dir != fExpectedDir) { + fConvexity = SkPath::kConcave_Convexity; + fDirection = SkPath::kUnknown_Direction; + } + fLastVec = vec; + break; + case kStraight_DirChange: + break; + case kBackwards_DirChange: + fLastVec = vec; + break; + case kInvalid_DirChange: + SkFAIL("Use of invalid direction change flag"); + break; } } @@ -2234,7 +2276,7 @@ private: // value with the current vec is deemed to be of a significant value. SkVector fLastVec, fFirstVec; int fPtCount; // non-degenerate points - int fSign; + DirChange fExpectedDir; SkPath::Convexity fConvexity; SkPath::Direction fDirection; int fDx, fDy, fSx, fSy; diff --git a/tests/PathTest.cpp b/tests/PathTest.cpp index 0de64b06f..798185c20 100644 --- a/tests/PathTest.cpp +++ b/tests/PathTest.cpp @@ -1195,6 +1195,17 @@ static void test_convexity2(skiatest::Reporter* reporter) { stroke.applyToPath(&strokedSin, strokedSin); check_convexity(reporter, strokedSin, SkPath::kConcave_Convexity); check_direction(reporter, strokedSin, kDontCheckDir); + + // http://crbug.com/412640 + SkPath degenerateConcave; + degenerateConcave.moveTo(148.67912f, 191.875f); + degenerateConcave.lineTo(470.37695f, 7.5f); + degenerateConcave.lineTo(148.67912f, 191.875f); + degenerateConcave.lineTo(41.446522f, 376.25f); + degenerateConcave.lineTo(-55.971577f, 460.0f); + degenerateConcave.lineTo(41.446522f, 376.25f); + check_convexity(reporter, degenerateConcave, SkPath::kConcave_Convexity); + check_direction(reporter, degenerateConcave, SkPath::kUnknown_Direction); } static void check_convex_bounds(skiatest::Reporter* reporter, const SkPath& p, -- cgit v1.2.3 From f76069a937da8c92adb1af4912b5ac92f0fb5b97 Mon Sep 17 00:00:00 2001 From: Justin Novosad Date: Mon, 29 Sep 2014 14:53:43 -0400 Subject: Cherry-pick of https://skia.googlesource.com/skia.git/+/96c118edff293af93db0a2b1b6775428117924b1 to m39 branch Change GrContext::copyTexture to go through GrDrawTarget BUG=crbug.com/415100 R=bsalomon@google.com, robertphillips@google.com Author: junov@chromium.org Review URL: https://codereview.chromium.org/607993002 Conflicts: src/gpu/GrContext.cpp Review URL: https://codereview.chromium.org/608353002 --- src/gpu/GrContext.cpp | 23 ++++++++--------------- src/gpu/SkGrPixelRef.cpp | 17 ++++++----------- 2 files changed, 14 insertions(+), 26 deletions(-) diff --git a/src/gpu/GrContext.cpp b/src/gpu/GrContext.cpp index 091c4a899..9628645cc 100755 --- a/src/gpu/GrContext.cpp +++ b/src/gpu/GrContext.cpp @@ -1556,17 +1556,6 @@ void GrContext::copyTexture(GrTexture* src, GrRenderTarget* dst, const SkIPoint* } ASSERT_OWNED_RESOURCE(src); - // Writes pending to the source texture are not tracked, so a flush - // is required to ensure that the copy captures the most recent contents - // of the source texture. See similar behavior in - // GrContext::resolveRenderTarget. - this->flush(); - - GrDrawTarget::AutoStateRestore asr(fGpu, GrDrawTarget::kReset_ASRInit); - GrDrawState* drawState = fGpu->drawState(); - drawState->setRenderTarget(dst); - SkMatrix sampleM; - sampleM.setIDiv(src->width(), src->height()); SkIRect srcRect = SkIRect::MakeWH(dst->width(), dst->height()); if (NULL != topLeft) { srcRect.offset(*topLeft); @@ -1575,10 +1564,14 @@ void GrContext::copyTexture(GrTexture* src, GrRenderTarget* dst, const SkIPoint* if (!srcRect.intersect(srcBounds)) { return; } - sampleM.preTranslate(SkIntToScalar(srcRect.fLeft), SkIntToScalar(srcRect.fTop)); - drawState->addColorTextureEffect(src, sampleM); - SkRect dstR = SkRect::MakeWH(SkIntToScalar(srcRect.width()), SkIntToScalar(srcRect.height())); - fGpu->drawSimpleRect(dstR); + + GrDrawTarget* target = this->prepareToDraw(NULL, BUFFERED_DRAW, NULL, NULL); + if (NULL == target) { + return; + } + SkIPoint dstPoint; + dstPoint.setZero(); + target->copySurface(dst, src, srcRect, dstPoint); } bool GrContext::writeRenderTargetPixels(GrRenderTarget* target, diff --git a/src/gpu/SkGrPixelRef.cpp b/src/gpu/SkGrPixelRef.cpp index 2131f41b1..b064b7e6a 100644 --- a/src/gpu/SkGrPixelRef.cpp +++ b/src/gpu/SkGrPixelRef.cpp @@ -51,7 +51,7 @@ bool SkROLockPixelsPixelRef::onLockPixelsAreWritable() const { /////////////////////////////////////////////////////////////////////////////// -static SkGrPixelRef* copyToTexturePixelRef(GrTexture* texture, SkColorType dstCT, +static SkGrPixelRef* copy_to_new_texture_pixelref(GrTexture* texture, SkColorType dstCT, const SkIRect* subset) { if (NULL == texture || kUnknown_SkColorType == dstCT) { return NULL; @@ -86,15 +86,10 @@ static SkGrPixelRef* copyToTexturePixelRef(GrTexture* texture, SkColorType dstCT context->copyTexture(texture, dst->asRenderTarget(), topLeft); - // TODO: figure out if this is responsible for Chrome canvas errors -#if 0 - // The render texture we have created (to perform the copy) isn't fully - // functional (since it doesn't have a stencil buffer). Release it here - // so the caller doesn't try to render to it. - // TODO: we can undo this release when dynamic stencil buffer attach/ - // detach has been implemented - dst->releaseRenderTarget(); -#endif + // Blink is relying on the above copy being sent to GL immediately in the case when the source + // is a WebGL canvas backing store. We could have a TODO to remove this flush, but we have a + // larger TODO to remove SkGrPixelRef entirely. + context->flush(); SkImageInfo info = SkImageInfo::Make(desc.fWidth, desc.fHeight, dstCT, kPremul_SkAlphaType); SkGrPixelRef* pixelRef = SkNEW_ARGS(SkGrPixelRef, (info, dst)); @@ -156,7 +151,7 @@ SkPixelRef* SkGrPixelRef::deepCopy(SkColorType dstCT, const SkIRect* subset) { // a GrTexture owned elsewhere (e.g., SkGpuDevice), and cannot live // independently of that texture. Texture-backed pixel refs, on the other // hand, own their GrTextures, and are thus self-contained. - return copyToTexturePixelRef(fSurface->asTexture(), dstCT, subset); + return copy_to_new_texture_pixelref(fSurface->asTexture(), dstCT, subset); } bool SkGrPixelRef::onReadPixels(SkBitmap* dst, const SkIRect* subset) { -- cgit v1.2.3 From f14866df6ca3ecce221916fa0c061af49385a863 Mon Sep 17 00:00:00 2001 From: Stephen White Date: Tue, 30 Sep 2014 17:37:36 -0400 Subject: Port of https://skia.googlesource.com/skia.git/+/3a49520696b2eca69e57884657d23fd2402ccfd1 to M38 branch. Sanitize SkMatrixConvolutionImageFilter creation params. Apply the same memory limit in the Create() function that we do when deserializing. Author: senorblanco@chromium.org Review URL: https://codereview.chromium.org/610723002 BUG=skia: TBR=reed@google.com Review URL: https://codereview.chromium.org/612483005 --- include/effects/SkMatrixConvolutionImageFilter.h | 6 +-- src/effects/SkMatrixConvolutionImageFilter.cpp | 35 ++++++++++++--- tests/ImageFilterTest.cpp | 54 ++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 10 deletions(-) diff --git a/include/effects/SkMatrixConvolutionImageFilter.h b/include/effects/SkMatrixConvolutionImageFilter.h index 0cb848b63..f021bf079 100644 --- a/include/effects/SkMatrixConvolutionImageFilter.h +++ b/include/effects/SkMatrixConvolutionImageFilter.h @@ -60,11 +60,7 @@ public: TileMode tileMode, bool convolveAlpha, SkImageFilter* input = NULL, - const CropRect* cropRect = NULL) { - return SkNEW_ARGS(SkMatrixConvolutionImageFilter, (kernelSize, kernel, gain, bias, - kernelOffset, tileMode, convolveAlpha, - input, cropRect)); - } + const CropRect* cropRect = NULL); SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS(SkMatrixConvolutionImageFilter) diff --git a/src/effects/SkMatrixConvolutionImageFilter.cpp b/src/effects/SkMatrixConvolutionImageFilter.cpp index 6e1d1a465..f61774203 100644 --- a/src/effects/SkMatrixConvolutionImageFilter.cpp +++ b/src/effects/SkMatrixConvolutionImageFilter.cpp @@ -29,6 +29,10 @@ static bool tile_mode_is_valid(SkMatrixConvolutionImageFilter::TileMode tileMode return false; } +// We need to be able to read at most SK_MaxS32 bytes, so divide that +// by the size of a scalar to know how many scalars we can read. +static const int32_t gMaxKernelSize = SK_MaxS32 / sizeof(SkScalar); + SkMatrixConvolutionImageFilter::SkMatrixConvolutionImageFilter( const SkISize& kernelSize, const SkScalar* kernel, @@ -46,7 +50,7 @@ SkMatrixConvolutionImageFilter::SkMatrixConvolutionImageFilter( fKernelOffset(kernelOffset), fTileMode(tileMode), fConvolveAlpha(convolveAlpha) { - uint32_t size = fKernelSize.fWidth * fKernelSize.fHeight; + size_t size = (size_t) sk_64_mul(fKernelSize.width(), fKernelSize.height()); fKernel = SkNEW_ARRAY(SkScalar, size); memcpy(fKernel, kernel, size * sizeof(SkScalar)); SkASSERT(kernelSize.fWidth >= 1 && kernelSize.fHeight >= 1); @@ -54,18 +58,39 @@ SkMatrixConvolutionImageFilter::SkMatrixConvolutionImageFilter( SkASSERT(kernelOffset.fY >= 0 && kernelOffset.fY < kernelSize.fHeight); } +SkMatrixConvolutionImageFilter* SkMatrixConvolutionImageFilter::Create( + const SkISize& kernelSize, + const SkScalar* kernel, + SkScalar gain, + SkScalar bias, + const SkIPoint& kernelOffset, + TileMode tileMode, + bool convolveAlpha, + SkImageFilter* input, + const CropRect* cropRect) { + if (kernelSize.width() < 1 || kernelSize.height() < 1) { + return NULL; + } + if (gMaxKernelSize / kernelSize.fWidth < kernelSize.fHeight) { + return NULL; + } + if (!kernel) { + return NULL; + } + return SkNEW_ARGS(SkMatrixConvolutionImageFilter, (kernelSize, kernel, gain, bias, + kernelOffset, tileMode, convolveAlpha, + input, cropRect)); +} + SkMatrixConvolutionImageFilter::SkMatrixConvolutionImageFilter(SkReadBuffer& buffer) : INHERITED(1, buffer) { - // We need to be able to read at most SK_MaxS32 bytes, so divide that - // by the size of a scalar to know how many scalars we can read. - static const int32_t kMaxSize = SK_MaxS32 / sizeof(SkScalar); fKernelSize.fWidth = buffer.readInt(); fKernelSize.fHeight = buffer.readInt(); if ((fKernelSize.fWidth >= 1) && (fKernelSize.fHeight >= 1) && // Make sure size won't be larger than a signed int, // which would still be extremely large for a kernel, // but we don't impose a hard limit for kernel size - (kMaxSize / fKernelSize.fWidth >= fKernelSize.fHeight)) { + (gMaxKernelSize / fKernelSize.fWidth >= fKernelSize.fHeight)) { size_t size = fKernelSize.fWidth * fKernelSize.fHeight; fKernel = SkNEW_ARRAY(SkScalar, size); SkDEBUGCODE(bool success =) buffer.readScalarArray(fKernel, size); diff --git a/tests/ImageFilterTest.cpp b/tests/ImageFilterTest.cpp index 9f6144bf2..8767ff204 100644 --- a/tests/ImageFilterTest.cpp +++ b/tests/ImageFilterTest.cpp @@ -774,6 +774,60 @@ DEF_TEST(HugeBlurImageFilter, reporter) { test_huge_blur(&device, reporter); } +DEF_TEST(MatrixConvolutionSanityTest, reporter) { + SkScalar kernel[1] = { 0 }; + SkScalar gain = SK_Scalar1, bias = 0; + SkIPoint kernelOffset = SkIPoint::Make(1, 1); + + // Check that an enormous (non-allocatable) kernel gives a NULL filter. + SkAutoTUnref conv(SkMatrixConvolutionImageFilter::Create( + SkISize::Make(1<<30, 1<<30), + kernel, + gain, + bias, + kernelOffset, + SkMatrixConvolutionImageFilter::kRepeat_TileMode, + false)); + + REPORTER_ASSERT(reporter, NULL == conv.get()); + + // Check that a NULL kernel gives a NULL filter. + conv.reset(SkMatrixConvolutionImageFilter::Create( + SkISize::Make(1, 1), + NULL, + gain, + bias, + kernelOffset, + SkMatrixConvolutionImageFilter::kRepeat_TileMode, + false)); + + REPORTER_ASSERT(reporter, NULL == conv.get()); + + // Check that a kernel width < 1 gives a NULL filter. + conv.reset(SkMatrixConvolutionImageFilter::Create( + SkISize::Make(0, 1), + kernel, + gain, + bias, + kernelOffset, + SkMatrixConvolutionImageFilter::kRepeat_TileMode, + false)); + + REPORTER_ASSERT(reporter, NULL == conv.get()); + + // Check that kernel height < 1 gives a NULL filter. + conv.reset(SkMatrixConvolutionImageFilter::Create( + SkISize::Make(1, -1), + kernel, + gain, + bias, + kernelOffset, + SkMatrixConvolutionImageFilter::kRepeat_TileMode, + false)); + + REPORTER_ASSERT(reporter, NULL == conv.get()); +} + static void test_xfermode_cropped_input(SkBaseDevice* device, skiatest::Reporter* reporter) { SkCanvas canvas(device); canvas.clear(0); -- cgit v1.2.3