From 1eea9fcb2443192e9034eb520aff0564b0950364 Mon Sep 17 00:00:00 2001 From: mtklein Date: Fri, 23 Jan 2015 16:09:32 -0800 Subject: [PATCH] Revert of Fix Morphology effects sourcing outside of the crop rect. (patchset #6 id:100001 of https://codereview.chromium.org/781153002/) Reason for revert: Looks like this is causing memory leaks: http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-ASAN/builds/1155/steps/dm/logs/stdio And causing crashes on Mac 10.6: http://build.chromium.org/p/client.skia/builders/Test-Mac10.6-MacMini4.1-GeForce320M-x86_64-Debug/builds/1417/steps/dm/logs/stdio Original issue's description: > Fix Morphology effects sourcing outside of the crop rect. > > BUG=skia:1766 > > Committed: https://skia.googlesource.com/skia/+/f6be925b5615f07039ce95c3433039694a8d1679 TBR=junov@google.com,junov@chromium.org,bsalomon@google.com,reed@google.com,cwallez@google.com NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=skia:1766 Review URL: https://codereview.chromium.org/868973005 --- gm/imagefilterscropped.cpp | 37 +---- src/effects/SkMorphologyImageFilter.cpp | 176 +++--------------------- 2 files changed, 21 insertions(+), 192 deletions(-) diff --git a/gm/imagefilterscropped.cpp b/gm/imagefilterscropped.cpp index 58edcac034..49b0a97f97 100644 --- a/gm/imagefilterscropped.cpp +++ b/gm/imagefilterscropped.cpp @@ -12,9 +12,7 @@ #include "SkShader.h" #include "SkBlurImageFilter.h" -#include "SkMorphologyImageFilter.h" #include "SkColorFilterImageFilter.h" -#include "SkBitmapSource.h" #include "SkMergeImageFilter.h" #include "SkOffsetImageFilter.h" #include "SkTestImageFilters.h" @@ -100,28 +98,7 @@ protected: return SkString("imagefilterscropped"); } - virtual SkISize onISize() { return SkISize::Make(400, 880); } - - void make_checkerboard() { - fCheckerboard.allocN32Pixels(80, 80); - SkCanvas canvas(fCheckerboard); - canvas.clear(SK_ColorTRANSPARENT); - SkPaint darkPaint; - darkPaint.setColor(0xFF404040); - SkPaint lightPaint; - lightPaint.setColor(0xFFA0A0A0); - for (int y = 0; y < 80; y += 16) { - for (int x = 0; x < 80; x += 16) { - canvas.save(); - canvas.translate(SkIntToScalar(x), SkIntToScalar(y)); - canvas.drawRect(SkRect::MakeXYWH(0, 0, 8, 8), darkPaint); - canvas.drawRect(SkRect::MakeXYWH(8, 0, 8, 8), lightPaint); - canvas.drawRect(SkRect::MakeXYWH(0, 8, 8, 8), lightPaint); - canvas.drawRect(SkRect::MakeXYWH(8, 8, 8, 8), darkPaint); - canvas.restore(); - } - } - } + virtual SkISize onISize() { return SkISize::Make(400, 640); } void draw_frame(SkCanvas* canvas, const SkRect& r) { SkPaint paint; @@ -130,10 +107,6 @@ protected: canvas->drawRect(r, paint); } - virtual void onOnceBeforeDraw() SK_OVERRIDE{ - make_checkerboard(); - } - virtual void onDraw(SkCanvas* canvas) { void (*drawProc[])(SkCanvas*, const SkRect&, SkImageFilter*) = { draw_sprite, draw_bitmap, draw_path, draw_paint, draw_text @@ -148,6 +121,7 @@ protected: SkIntToScalar(-10), SkIntToScalar(-10))); SkAutoTUnref cfOffset(SkColorFilterImageFilter::Create(cf.get(), offset.get())); + SkImageFilter* filters[] = { NULL, SkColorFilterImageFilter::Create(cf.get(), NULL, &cropRect), @@ -155,10 +129,6 @@ protected: SkBlurImageFilter::Create(8.0f, 0.0f, NULL, &cropRect), SkBlurImageFilter::Create(0.0f, 8.0f, NULL, &cropRect), SkBlurImageFilter::Create(8.0f, 8.0f, NULL, &cropRect), - SkErodeImageFilter::Create(1, 1, NULL, &cropRect), - SkErodeImageFilter::Create(8, 0, SkErodeImageFilter::Create(0, 8, NULL, &cropRect), &cropRect), - SkErodeImageFilter::Create(0, 8, SkErodeImageFilter::Create(8, 0, NULL, &cropRect), &cropRect), - SkErodeImageFilter::Create(8, 8, NULL, &cropRect), SkMergeImageFilter::Create(NULL, cfOffset.get(), SkXfermode::kSrcOver_Mode, &cropRect), SkBlurImageFilter::Create(8.0f, 8.0f, NULL, &bogusRect), SkColorFilterImageFilter::Create(cf.get(), NULL, &bogusRect), @@ -173,8 +143,6 @@ protected: for (size_t j = 0; j < SK_ARRAY_COUNT(drawProc); ++j) { canvas->save(); for (size_t i = 0; i < SK_ARRAY_COUNT(filters); ++i) { - SkPaint paint; - canvas->drawBitmap(fCheckerboard, 0, 0); drawProc[j](canvas, r, filters[i]); canvas->translate(0, DY); } @@ -188,7 +156,6 @@ protected: } private: - SkBitmap fCheckerboard; typedef GM INHERITED; }; diff --git a/src/effects/SkMorphologyImageFilter.cpp b/src/effects/SkMorphologyImageFilter.cpp index 20072f01ed..e895cacf13 100644 --- a/src/effects/SkMorphologyImageFilter.cpp +++ b/src/effects/SkMorphologyImageFilter.cpp @@ -303,16 +303,9 @@ public: return SkNEW_ARGS(GrMorphologyEffect, (tex, dir, radius, type)); } - static GrFragmentProcessor* Create(GrTexture* tex, Direction dir, int radius, - MorphologyType type, float bounds[2]) { - return SkNEW_ARGS(GrMorphologyEffect, (tex, dir, radius, type, bounds)); - } - virtual ~GrMorphologyEffect(); MorphologyType type() const { return fType; } - bool useRange() const { return fUseRange; } - const float* range() const { return fRange; } const char* name() const SK_OVERRIDE { return "Morphology"; } @@ -323,8 +316,6 @@ public: protected: MorphologyType fType; - bool fUseRange; - float fRange[2]; private: bool onIsEqual(const GrFragmentProcessor&) const SK_OVERRIDE; @@ -332,7 +323,6 @@ private: void onComputeInvariantOutput(GrInvariantOutput* inout) const SK_OVERRIDE; GrMorphologyEffect(GrTexture*, Direction, int radius, MorphologyType); - GrMorphologyEffect(GrTexture*, Direction, int radius, MorphologyType, float bounds[2]); GR_DECLARE_FRAGMENT_PROCESSOR_TEST; @@ -360,11 +350,8 @@ private: int width() const { return GrMorphologyEffect::WidthFromRadius(fRadius); } int fRadius; - Gr1DKernelEffect::Direction fDirection; - bool fUseRange; GrMorphologyEffect::MorphologyType fType; - GrGLProgramDataManager::UniformHandle fPixelSizeUni; - GrGLProgramDataManager::UniformHandle fRangeUni; + GrGLProgramDataManager::UniformHandle fImageIncrementUni; typedef GrGLFragmentProcessor INHERITED; }; @@ -372,8 +359,6 @@ private: GrGLMorphologyEffect::GrGLMorphologyEffect(const GrProcessor& proc) { const GrMorphologyEffect& m = proc.cast(); fRadius = m.radius(); - fDirection = m.direction(); - fUseRange = m.useRange(); fType = m.type(); } @@ -383,14 +368,9 @@ void GrGLMorphologyEffect::emitCode(GrGLFPBuilder* builder, const char* inputColor, const TransformedCoordsArray& coords, const TextureSamplerArray& samplers) { - fPixelSizeUni = builder->addUniform(GrGLProgramBuilder::kFragment_Visibility, - kFloat_GrSLType, kDefault_GrSLPrecision, - "PixelSize"); - const char* pixelSizeInc = builder->getUniformCStr(fPixelSizeUni); - fRangeUni = builder->addUniform(GrGLProgramBuilder::kFragment_Visibility, - kVec2f_GrSLType, kDefault_GrSLPrecision, - "Range"); - const char* range = builder->getUniformCStr(fRangeUni); + fImageIncrementUni = builder->addUniform(GrGLProgramBuilder::kFragment_Visibility, + kVec2f_GrSLType, kDefault_GrSLPrecision, + "ImageIncrement"); GrGLFPFragmentBuilder* fsBuilder = builder->getFragmentShaderBuilder(); SkString coords2D = fsBuilder->ensureFSCoords2D(coords, 0); @@ -409,41 +389,14 @@ void GrGLMorphologyEffect::emitCode(GrGLFPBuilder* builder, func = ""; // suppress warning break; } + const char* imgInc = builder->getUniformCStr(fImageIncrementUni); - const char* dir; - switch (fDirection) { - case Gr1DKernelEffect::kX_Direction: - dir = "x"; - break; - case Gr1DKernelEffect::kY_Direction: - dir = "y"; - break; - default: - SkFAIL("Unknown filter direction."); - dir = ""; // suppress warning - } - - // vec2 coord = coord2D; - fsBuilder->codeAppendf("\t\tvec2 coord = %s;\n", coords2D.c_str()); - // coord.x -= radius * pixelSize; - fsBuilder->codeAppendf("\t\tcoord.%s -= %d.0 * %s; \n", dir, fRadius, pixelSizeInc); - if (fUseRange) { - // highBound = min(highBound, coord.x + (width-1) * pixelSize); - fsBuilder->codeAppendf("\t\tfloat highBound = min(%s.y, coord.%s + %d * %s);", - range, dir, width() - 1, pixelSizeInc); - // coord.x = max(lowBound, coord.x); - fsBuilder->codeAppendf("\t\tcoord.%s = max(%s.x, coord.%s);", dir, range, dir); - } - fsBuilder->codeAppendf("\t\tfor (int i = 0; i < %d; i++) {\n", width()); + fsBuilder->codeAppendf("\t\tvec2 coord = %s - %d.0 * %s;\n", coords2D.c_str(), fRadius, imgInc); + fsBuilder->codeAppendf("\t\tfor (int i = 0; i < %d; i++) {\n", this->width()); fsBuilder->codeAppendf("\t\t\t%s = %s(%s, ", outputColor, func, outputColor); fsBuilder->appendTextureLookup(samplers[0], "coord"); fsBuilder->codeAppend(");\n"); - // coord.x += pixelSize; - fsBuilder->codeAppendf("\t\t\tcoord.%s += %s;\n", dir, pixelSizeInc); - if (fUseRange) { - // coord.x = min(highBound, coord.x); - fsBuilder->codeAppendf("\t\t\tcoord.%s = min(highBound, coord.%s);", dir, dir); - } + fsBuilder->codeAppendf("\t\t\tcoord += %s;\n", imgInc); fsBuilder->codeAppend("\t\t}\n"); SkString modulate; GrGLSLMulVarBy4f(&modulate, outputColor, inputColor); @@ -455,41 +408,27 @@ void GrGLMorphologyEffect::GenKey(const GrProcessor& proc, const GrMorphologyEffect& m = proc.cast(); uint32_t key = static_cast(m.radius()); key |= (m.type() << 8); - key |= (m.direction() << 9); - if (m.useRange()) key |= 1 << 10; b->add32(key); } void GrGLMorphologyEffect::setData(const GrGLProgramDataManager& pdman, const GrProcessor& proc) { - const GrMorphologyEffect& m = proc.cast(); - GrTexture& texture = *m.texture(0); - // the code we generated was for a specific kernel radius, direction and bound usage - SkASSERT(m.radius() == fRadius); - SkASSERT(m.direction() == fDirection); - SkASSERT(m.useRange() == fUseRange); - - float pixelSize = 0.0f; - switch (fDirection) { + const Gr1DKernelEffect& kern = proc.cast(); + GrTexture& texture = *kern.texture(0); + // the code we generated was for a specific kernel radius + SkASSERT(kern.radius() == fRadius); + float imageIncrement[2] = { 0 }; + switch (kern.direction()) { case Gr1DKernelEffect::kX_Direction: - pixelSize = 1.0f / texture.width(); + imageIncrement[0] = 1.0f / texture.width(); break; case Gr1DKernelEffect::kY_Direction: - pixelSize = 1.0f / texture.height(); + imageIncrement[1] = 1.0f / texture.height(); break; default: SkFAIL("Unknown filter direction."); } - pdman.set1f(fPixelSizeUni, pixelSize); - - if (fUseRange) { - const float* range = m.range(); - if (fDirection && texture.origin() == kBottomLeft_GrSurfaceOrigin) { - pdman.set2f(fRangeUni, 1.0f - range[1], 1.0f - range[0]); - } else { - pdman.set2f(fRangeUni, range[0], range[1]); - } - } + pdman.set2fv(fImageIncrementUni, 1, imageIncrement); } /////////////////////////////////////////////////////////////////////////////// @@ -499,22 +438,10 @@ GrMorphologyEffect::GrMorphologyEffect(GrTexture* texture, int radius, MorphologyType type) : Gr1DKernelEffect(texture, direction, radius) - , fType(type), fUseRange(false) { + , fType(type) { this->initClassID(); } -GrMorphologyEffect::GrMorphologyEffect(GrTexture* texture, - Direction direction, - int radius, - MorphologyType type, - float range[2]) - : Gr1DKernelEffect(texture, direction, radius) - , fType(type), fUseRange(true) { - this->initClassID(); - fRange[0] = range[0]; - fRange[1] = range[1]; -} - GrMorphologyEffect::~GrMorphologyEffect() { } @@ -529,7 +456,6 @@ bool GrMorphologyEffect::onIsEqual(const GrFragmentProcessor& sBase) const { const GrMorphologyEffect& s = sBase.cast(); return (this->radius() == s.radius() && this->direction() == s.direction() && - this->useRange() == s.useRange() && this->type() == s.type()); } @@ -560,26 +486,7 @@ GrFragmentProcessor* GrMorphologyEffect::TestCreate(SkRandom* random, namespace { - -void apply_morphology_rect(GrContext* context, - GrTexture* texture, - const SkIRect& srcRect, - const SkIRect& dstRect, - int radius, - GrMorphologyEffect::MorphologyType morphType, - float bounds[2], - Gr1DKernelEffect::Direction direction) { - GrPaint paint; - paint.addColorProcessor(GrMorphologyEffect::Create(texture, - direction, - radius, - morphType, - bounds))->unref(); - context->drawNonAARectToRect(paint, SkMatrix::I(), SkRect::Make(dstRect), - SkRect::Make(srcRect)); -} - -void apply_morphology_rect_no_bounds(GrContext* context, +void apply_morphology_pass(GrContext* context, GrTexture* texture, const SkIRect& srcRect, const SkIRect& dstRect, @@ -595,51 +502,6 @@ void apply_morphology_rect_no_bounds(GrContext* context, SkRect::Make(srcRect)); } -void apply_morphology_pass(GrContext* context, - GrTexture* texture, - const SkIRect& srcRect, - const SkIRect& dstRect, - int radius, - GrMorphologyEffect::MorphologyType morphType, - Gr1DKernelEffect::Direction direction) { - float bounds[2] = { 0.0f, 1.0f }; - SkIRect lowerSrcRect = srcRect, lowerDstRect = dstRect; - SkIRect middleSrcRect = srcRect, middleDstRect = dstRect; - SkIRect upperSrcRect = srcRect, upperDstRect = dstRect; - if (direction == Gr1DKernelEffect::kX_Direction) { - bounds[0] = (SkIntToScalar(srcRect.left()) + 0.5f) / texture->width(); - bounds[1] = (SkIntToScalar(srcRect.right()) - 0.5f) / texture->width(); - lowerSrcRect.fRight = srcRect.left() + radius; - lowerDstRect.fRight = dstRect.left() + radius; - upperSrcRect.fLeft = srcRect.right() - radius; - upperDstRect.fLeft = dstRect.right() - radius; - middleSrcRect.inset(radius, 0); - middleDstRect.inset(radius, 0); - } else { - bounds[0] = (SkIntToScalar(srcRect.top()) + 0.5f) / texture->height(); - bounds[1] = (SkIntToScalar(srcRect.bottom()) - 0.5f) / texture->height(); - lowerSrcRect.fBottom = srcRect.top() + radius; - lowerDstRect.fBottom = dstRect.top() + radius; - upperSrcRect.fTop = srcRect.bottom() - radius; - upperDstRect.fTop = dstRect.bottom() - radius; - middleSrcRect.inset(0, radius); - middleDstRect.inset(0, radius); - } - if (middleSrcRect.fLeft - middleSrcRect.fRight >= 0) { - // radius covers srcRect; use bounds over entire draw - apply_morphology_rect(context, texture, srcRect, dstRect, radius, - morphType, bounds, direction); - } else { - // Draw upper and lower margins with bounds; middle without. - apply_morphology_rect(context, texture, lowerSrcRect, lowerDstRect, radius, - morphType, bounds, direction); - apply_morphology_rect(context, texture, upperSrcRect, upperDstRect, radius, - morphType, bounds, direction); - apply_morphology_rect_no_bounds(context, texture, middleSrcRect, middleDstRect, radius, - morphType, direction); - } -} - bool apply_morphology(const SkBitmap& input, const SkIRect& rect, GrMorphologyEffect::MorphologyType morphType,