From 0e56972e5f69f75cf098fec10687f128430c13e6 Mon Sep 17 00:00:00 2001 From: rmistry Date: Fri, 8 Jul 2016 06:23:35 -0700 Subject: [PATCH] Revert of Abstracted diffuse color map out of SkLightingShaderImpl into a generic SkShader (patchset #11 id:200001 of https://codereview.chromium.org/2062703003/ ) Reason for revert: Causing the Test-Ubuntu-GCC-ShuttleA-GPU-GTX550Ti-x86_64-Release-Valgrind builder to fail. Started failing at this build: https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU-GTX550Ti-x86_64-Release-Valgrind/builds/1214 Snippet from build log: ==3016== Invalid read of size 4 ==3016== at 0xCD1901: GrFragmentProcessor::registerChildProcessor(sk_sp) (SkTArray.h:163) ==3016== by 0xA58B47: _Z10sk_make_spI11NormalMapFPI5sk_spI19GrFragmentProcessorERK8SkMatrixEES1_IT_EDpOT0_ (SkNormalSource.cpp:84) ==3016== by 0xA582E8: NormalMapSourceImpl::asFragmentProcessor(GrContext*, SkMatrix const&, SkMatrix const*, SkFilterQuality, SkSourceGammaTreatment) const (SkNormalSource.cpp:187) ==3016== by 0xA0ADB5: SkLightingShaderImpl::asFragmentProcessor(GrContext*, SkMatrix const&, SkMatrix const*, SkFilterQuality, SkSourceGammaTreatment) const (SkLightingShader.cpp:268) ==3016== by 0xDA17E6: skpaint_to_grpaint_impl(GrContext*, SkPaint const&, SkMatrix const&, sk_sp*, SkXfermode::Mode*, bool, bool, GrPaint*) (SkGr.cpp:552) ==3016== by 0xDA4532: SkPaintToGrPaint(GrContext*, SkPaint const&, SkMatrix const&, bool, GrPaint*) (SkGr.cpp:668) ==3016== by 0xD99B51: SkGpuDevice::drawRect(SkDraw const&, SkRect const&, SkPaint const&) (SkGpuDevice.cpp:534) ==3016== by 0x9DDB1C: SkCanvas::onDrawRect(SkRect const&, SkPaint const&) (SkCanvas.cpp:2148) ==3016== by 0x9DBF40: SkCanvas::drawRect(SkRect const&, SkPaint const&) (SkCanvas.cpp:1919) ==3016== by 0x8D2A55: skiagm::LightingShaderGM::onDraw(SkCanvas*) (lightingshader.cpp:110) ==3016== by 0x625659: skiagm::GM::drawContent(SkCanvas*) (gm.cpp:32) ==3016== by 0x6256AD: skiagm::GM::draw(SkCanvas*) (gm.cpp:24) ==3016== by 0x61B747: DM::GMSrc::draw(SkCanvas*) const (DMSrcSink.cpp:61) ==3016== by 0x61F37B: DM::GPUSink::draw(DM::Src const&, SkBitmap*, SkWStream*, SkString*) const (DMSrcSink.cpp:1115) ==3016== by 0x619261: dm_main() (DM.cpp:1016) ==3016== by 0x619841: main (DM.cpp:1409) ==3016== Address 0xf4 is not stack'd, malloc'd or (recently) free'd ==3016== Caught signal 11 [Segmentation fault], was running: gpu gm lightingshader Original issue's description: > Abstracted diffuse color map out of SkLightingShaderImpl into a generic SkShader > > Will not run until after landing https://codereview.chromium.org/2106893003/ > > This CL's base is the CL for the addition of NormalSource to the API: https://codereview.chromium.org/2063793002/ > > BUG=skia: > GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2062703003 > > Committed: https://skia.googlesource.com/skia/+/3f58cd0eb8bf4499c4f179aa6111d7e005809df8 TBR=egdaniel@google.com,robertphillips@google.com,dvonbeck@google.com # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=skia: NOTRY=true Review-Url: https://codereview.chromium.org/2137513002 --- src/core/SkLightingShader.cpp | 205 ++++++++++++++++++++++++++++++------------ src/core/SkNormalSource.cpp | 1 + 2 files changed, 149 insertions(+), 57 deletions(-) diff --git a/src/core/SkLightingShader.cpp b/src/core/SkLightingShader.cpp index 4730281..b40f4a7 100644 --- a/src/core/SkLightingShader.cpp +++ b/src/core/SkLightingShader.cpp @@ -41,18 +41,22 @@ */ class SkLightingShaderImpl : public SkShader { public: + /** Create a new lighting shader that uses the provided normal map and lights to light the diffuse bitmap. - @param diffuseShader the shader that provides the diffuse colors + @param diffuse the diffuse bitmap + @param lights the lights applied to the normal map + @param diffLocalM the local matrix for the diffuse coordinates @param normalSource the source of normals for lighting computation - @param lights the lights applied to the geometry */ - SkLightingShaderImpl(sk_sp diffuseShader, - sk_sp normalSource, - const sk_sp lights) - : fDiffuseShader(std::move(diffuseShader)) - , fNormalSource(std::move(normalSource)) - , fLights(std::move(lights)) {} + SkLightingShaderImpl(const SkBitmap& diffuse, + const sk_sp lights, + const SkMatrix* diffLocalM, + sk_sp normalSource) + : INHERITED(diffLocalM) + , fDiffuseMap(diffuse) + , fLights(std::move(lights)) + , fNormalSource(std::move(normalSource)) {} bool isOpaque() const override; @@ -69,9 +73,8 @@ public: // The context takes ownership of the states. It will call their destructors // but will NOT free the memory. LightingShaderContext(const SkLightingShaderImpl&, const ContextRec&, - SkShader::Context* diffuseContext, SkNormalSource::Provider*, + SkBitmapProcState* diffuseState, SkNormalSource::Provider*, void* heapAllocated); - ~LightingShaderContext() override; void shadeSpan(int x, int y, SkPMColor[], int count) override; @@ -79,7 +82,7 @@ public: uint32_t getFlags() const override { return fFlags; } private: - SkShader::Context* fDiffuseContext; + SkBitmapProcState* fDiffuseState; SkNormalSource::Provider* fNormalProvider; uint32_t fFlags; @@ -97,10 +100,11 @@ protected: Context* onCreateContext(const ContextRec&, void*) const override; private: - sk_sp fDiffuseShader; - sk_sp fNormalSource; + SkBitmap fDiffuseMap; sk_sp fLights; + sk_sp fNormalSource; + friend class SkLightingShader; typedef SkShader INHERITED; @@ -123,7 +127,12 @@ private: class LightingFP : public GrFragmentProcessor { public: - LightingFP(sk_sp normalFP, sk_sp lights) { + LightingFP(GrTexture* diffuse, const SkMatrix& diffMatrix, const GrTextureParams& diffParams, + sk_sp lights, sk_sp normalFP) + : fDiffDeviceTransform(kLocal_GrCoordSet, diffMatrix, diffuse, diffParams.filterMode()) + , fDiffuseTextureAccess(diffuse, diffParams) { + this->addCoordTransform(&fDiffDeviceTransform); + this->addTextureAccess(&fDiffuseTextureAccess); // fuse all ambient lights into a single one fAmbientColor.set(0.0f, 0.0f, 0.0f); @@ -170,7 +179,11 @@ public: kVec3f_GrSLType, kDefault_GrSLPrecision, "AmbientColor", &ambientColorUniName); - fragBuilder->codeAppendf("vec4 diffuseColor = %s;", args.fInputColor); + fragBuilder->codeAppend("vec4 diffuseColor = "); + fragBuilder->appendTextureLookupAndModulate(args.fInputColor, args.fTexSamplers[0], + args.fCoords[0].c_str(), + args.fCoords[0].getType()); + fragBuilder->codeAppend(";"); SkString dstNormalName("dstNormal"); this->emitChild(0, nullptr, &dstNormalName, args); @@ -245,11 +258,15 @@ private: bool onIsEqual(const GrFragmentProcessor& proc) const override { const LightingFP& lightingFP = proc.cast(); - return fLightDir == lightingFP.fLightDir && + return fDiffDeviceTransform == lightingFP.fDiffDeviceTransform && + fDiffuseTextureAccess == lightingFP.fDiffuseTextureAccess && + fLightDir == lightingFP.fLightDir && fLightColor == lightingFP.fLightColor && fAmbientColor == lightingFP.fAmbientColor; } + GrCoordTransform fDiffDeviceTransform; + GrTextureAccess fDiffuseTextureAccess; SkVector3 fLightDir; SkColor3f fLightColor; SkColor3f fAmbientColor; @@ -257,21 +274,66 @@ private: //////////////////////////////////////////////////////////////////////////// +static bool make_mat(const SkBitmap& bm, + const SkMatrix& localMatrix1, + const SkMatrix* localMatrix2, + SkMatrix* result) { + + result->setIDiv(bm.width(), bm.height()); + + SkMatrix lmInverse; + if (!localMatrix1.invert(&lmInverse)) { + return false; + } + if (localMatrix2) { + SkMatrix inv; + if (!localMatrix2->invert(&inv)) { + return false; + } + lmInverse.postConcat(inv); + } + result->preConcat(lmInverse); + + return true; +} + sk_sp SkLightingShaderImpl::asFragmentProcessor( GrContext* context, const SkMatrix& viewM, const SkMatrix* localMatrix, SkFilterQuality filterQuality, SkSourceGammaTreatment gammaTreatment) const { + // we assume diffuse and normal maps have same width and height + // TODO: support different sizes, will be addressed when diffuse maps are factored out of + // SkLightingShader in a future CL + SkMatrix diffM; + + if (!make_mat(fDiffuseMap, this->getLocalMatrix(), localMatrix, &diffM)) { + return nullptr; + } + + bool doBicubic; + GrTextureParams::FilterMode diffFilterMode = GrSkFilterQualityToGrFilterMode( + SkTMin(filterQuality, kMedium_SkFilterQuality), + viewM, + this->getLocalMatrix(), + &doBicubic); + SkASSERT(!doBicubic); + + // TODO: support other tile modes + GrTextureParams diffParams(kClamp_TileMode, diffFilterMode); + SkAutoTUnref diffuseTexture(GrRefCachedBitmapTexture(context, fDiffuseMap, + diffParams, gammaTreatment)); + if (!diffuseTexture) { + SkErrorInternals::SetError(kInternalError_SkError, "Couldn't convert bitmap to texture."); + return nullptr; + } + sk_sp normalFP( fNormalSource->asFragmentProcessor(context, viewM, localMatrix, filterQuality, gammaTreatment)); - sk_sp fpPipeline[] = { - fDiffuseShader->asFragmentProcessor(context, viewM, localMatrix, filterQuality, - gammaTreatment), - sk_make_sp(std::move(normalFP), fLights) - }; - sk_sp inner(GrFragmentProcessor::RunInSeries(fpPipeline, 2)); + sk_sp inner ( + new LightingFP(diffuseTexture, diffM, diffParams, fLights, std::move(normalFP))); return GrFragmentProcessor::MulOutputByInputAlpha(std::move(inner)); } @@ -281,18 +343,18 @@ sk_sp SkLightingShaderImpl::asFragmentProcessor( //////////////////////////////////////////////////////////////////////////// bool SkLightingShaderImpl::isOpaque() const { - return fDiffuseShader->isOpaque(); + return fDiffuseMap.isOpaque(); } SkLightingShaderImpl::LightingShaderContext::LightingShaderContext( - const SkLightingShaderImpl& shader, const ContextRec& rec, - SkShader::Context* diffuseContext, SkNormalSource::Provider* normalProvider, - void* heapAllocated) + const SkLightingShaderImpl& shader, const ContextRec& rec, SkBitmapProcState* diffuseState, + SkNormalSource::Provider* normalProvider, void* heapAllocated) : INHERITED(shader, rec) - , fDiffuseContext(diffuseContext) + , fDiffuseState(diffuseState) , fNormalProvider(normalProvider) , fHeapAllocated(heapAllocated) { - bool isOpaque = shader.isOpaque(); + const SkPixmap& pixmap = fDiffuseState->fPixmap; + bool isOpaque = pixmap.isOpaque(); // update fFlags uint32_t flags = 0; @@ -304,9 +366,9 @@ SkLightingShaderImpl::LightingShaderContext::LightingShaderContext( } SkLightingShaderImpl::LightingShaderContext::~LightingShaderContext() { - // The dependencies have been created outside of the context on memory that was allocated by - // the onCreateContext() method. Call the destructors and free the memory. - fDiffuseContext->~Context(); + // The bitmap proc states have been created outside of the context on memory that will be freed + // elsewhere. Call the destructors but leave the freeing of the memory to the caller. + fDiffuseState->~SkBitmapProcState(); fNormalProvider->~Provider(); sk_free(fHeapAllocated); @@ -336,23 +398,39 @@ static inline SkPMColor convert(SkColor3f color, U8CPU a) { // larger is better (fewer times we have to loop), but we shouldn't // take up too much stack-space (each one here costs 16 bytes) -#define BUFFER_MAX 16 +#define TMP_COUNT 16 +#define BUFFER_MAX ((int)(TMP_COUNT * sizeof(uint32_t))) void SkLightingShaderImpl::LightingShaderContext::shadeSpan(int x, int y, SkPMColor result[], int count) { const SkLightingShaderImpl& lightShader = static_cast(fShader); - SkPMColor diffuse[BUFFER_MAX]; + uint32_t tmpColor[TMP_COUNT]; + SkPMColor tmpColor2[2*TMP_COUNT]; + + SkBitmapProcState::MatrixProc diffMProc = fDiffuseState->getMatrixProc(); + SkBitmapProcState::SampleProc32 diffSProc = fDiffuseState->getSampleProc32(); + + int max = fDiffuseState->maxCountForBufferSize(BUFFER_MAX); + + SkASSERT(fDiffuseState->fPixmap.addr()); + + SkASSERT(max <= BUFFER_MAX); SkPoint3 normals[BUFFER_MAX]; do { - int n = SkTMin(count, BUFFER_MAX); + int n = count; + if (n > max) { + n = max; + } + + diffMProc(*fDiffuseState, tmpColor, n, x, y); + diffSProc(*fDiffuseState, tmpColor, n, tmpColor2); - fDiffuseContext->shadeSpan(x, y, diffuse, n); fNormalProvider->fillScanLine(x, y, normals, n); for (int i = 0; i < n; ++i) { - SkColor diffColor = SkUnPreMultiply::PMColorToColor(diffuse[i]); + SkColor diffColor = SkUnPreMultiply::PMColorToColor(tmpColor2[i]); SkColor3f accum = SkColor3f::Make(0.0f, 0.0f, 0.0f); // This is all done in linear unpremul color space (each component 0..255.0f though) @@ -391,10 +469,19 @@ void SkLightingShaderImpl::toString(SkString* str) const { #endif sk_sp SkLightingShaderImpl::CreateProc(SkReadBuffer& buf) { + SkMatrix diffLocalM; + bool hasDiffLocalM = buf.readBool(); + if (hasDiffLocalM) { + buf.readMatrix(&diffLocalM); + } else { + diffLocalM.reset(); + } - // Discarding SkShader flattenable params - bool hasLocalMatrix = buf.readBool(); - SkAssertResult(!hasLocalMatrix); + SkBitmap diffuse; + if (!buf.readBitmap(&diffuse)) { + return nullptr; + } + diffuse.setImmutable(); int numLights = buf.readInt(); @@ -422,15 +509,16 @@ sk_sp SkLightingShaderImpl::CreateProc(SkReadBuffer& buf) { sk_sp lights(builder.finish()); sk_sp normalSource(buf.readFlattenable()); - sk_sp diffuseShader(buf.readFlattenable()); - return sk_make_sp(std::move(diffuseShader), std::move(normalSource), - std::move(lights)); + return sk_make_sp(diffuse, std::move(lights), &diffLocalM, + std::move(normalSource)); } void SkLightingShaderImpl::flatten(SkWriteBuffer& buf) const { this->INHERITED::flatten(buf); + buf.writeBitmap(fDiffuseMap); + buf.writeInt(fLights->numLights()); for (int l = 0; l < fLights->numLights(); ++l) { const SkLights::Light& light = fLights->light(l); @@ -445,7 +533,6 @@ void SkLightingShaderImpl::flatten(SkWriteBuffer& buf) const { } buf.writeFlattenable(fNormalSource.get()); - buf.writeFlattenable(fDiffuseShader.get()); } size_t SkLightingShaderImpl::onContextSize(const ContextRec& rec) const { @@ -454,27 +541,35 @@ size_t SkLightingShaderImpl::onContextSize(const ContextRec& rec) const { SkShader::Context* SkLightingShaderImpl::onCreateContext(const ContextRec& rec, void* storage) const { - size_t heapRequired = fDiffuseShader->contextSize(rec) + - fNormalSource->providerSize(rec); + + SkMatrix diffTotalInv; + // computeTotalInverse was called in SkShader::createContext so we know it will succeed + SkAssertResult(this->computeTotalInverse(rec, &diffTotalInv)); + + size_t heapRequired = sizeof(SkBitmapProcState) + fNormalSource->providerSize(rec); void* heapAllocated = sk_malloc_throw(heapRequired); - void* diffuseContextStorage = heapAllocated; - SkShader::Context* diffuseContext = fDiffuseShader->createContext(rec, diffuseContextStorage); - if (!diffuseContext) { + void* diffuseStateStorage = heapAllocated; + SkBitmapProcState* diffuseState = new (diffuseStateStorage) SkBitmapProcState(fDiffuseMap, + SkShader::kClamp_TileMode, SkShader::kClamp_TileMode, + SkMipMap::DeduceTreatment(rec)); + SkASSERT(diffuseState); + if (!diffuseState->setup(diffTotalInv, *rec.fPaint)) { + diffuseState->~SkBitmapProcState(); sk_free(heapAllocated); return nullptr; } + void* normalProviderStorage = (char*)heapAllocated + sizeof(SkBitmapProcState); - void* normalProviderStorage = (char*)heapAllocated + fDiffuseShader->contextSize(rec); SkNormalSource::Provider* normalProvider = fNormalSource->asProvider(rec, normalProviderStorage); if (!normalProvider) { - diffuseContext->~Context(); + diffuseState->~SkBitmapProcState(); sk_free(heapAllocated); return nullptr; } - return new (storage) LightingShaderContext(*this, rec, diffuseContext, normalProvider, + return new (storage) LightingShaderContext(*this, rec, diffuseState, normalProvider, heapAllocated); } @@ -493,12 +588,8 @@ sk_sp SkLightingShader::Make(const SkBitmap& diffuse, return nullptr; } - // TODO: support other tile modes - sk_sp diffuseShader = SkBitmapProcShader::MakeBitmapShader(diffuse, - SkShader::kClamp_TileMode, SkShader::kClamp_TileMode, diffLocalM); - - return sk_make_sp(std::move(diffuseShader), std::move(normalSource), - std::move(lights)); + return sk_make_sp(diffuse, std::move(lights), diffLocalM, + std::move(normalSource)); } /////////////////////////////////////////////////////////////////////////////// diff --git a/src/core/SkNormalSource.cpp b/src/core/SkNormalSource.cpp index 19c97f9..52bb4ad 100644 --- a/src/core/SkNormalSource.cpp +++ b/src/core/SkNormalSource.cpp @@ -181,6 +181,7 @@ sk_sp NormalMapSourceImpl::asFragmentProcessor( const SkMatrix *localMatrix, SkFilterQuality filterQuality, SkSourceGammaTreatment gammaTreatment) const { + sk_sp mapFP = fMapShader->asFragmentProcessor(context, viewM, localMatrix, filterQuality, gammaTreatment); -- 2.7.4