From 6c6f65885ba20ba9e8d8c36039f1c9eff4cc814b Mon Sep 17 00:00:00 2001 From: bsalomon Date: Thu, 10 Sep 2015 08:12:46 -0700 Subject: [PATCH] Add a mutex to GrContext::readSurfacePixels to protect against multiple CPU raster threads accessing the same GrContext to read back GPU input data BUG=chromium:524717 TBR=reed@google.com Committed: https://skia.googlesource.com/skia/+/eb662bc407cec0585a821946fef123102cae64db Review URL: https://codereview.chromium.org/1329313002 --- include/gpu/GrContext.h | 21 +++++++++++++++++++-- src/gpu/GrContext.cpp | 38 ++++++++++++++++++++++++++------------ 2 files changed, 45 insertions(+), 14 deletions(-) diff --git a/include/gpu/GrContext.h b/include/gpu/GrContext.h index 7f1e410..64f4f6c 100644 --- a/include/gpu/GrContext.h +++ b/include/gpu/GrContext.h @@ -15,6 +15,7 @@ #include "GrRenderTarget.h" #include "GrTextureProvider.h" #include "SkMatrix.h" +#include "SkMutex.h" #include "SkPathEffect.h" #include "SkTypes.h" @@ -380,6 +381,18 @@ private: bool fDidTestPMConversions; int fPMToUPMConversion; int fUPMToPMConversion; + // The sw backend may call GrContext::readSurfacePixels on multiple threads + // We may transfer the responsibilty for using a mutex to the sw backend + // when there are fewer code paths that lead to a readSurfacePixels call + // from the sw backend. readSurfacePixels is reentrant in one case - when performing + // the PM conversions test. To handle this we do the PM conversions test outside + // of fReadPixelsMutex and use a separate mutex to guard it. When it re-enters + // readSurfacePixels it will grab fReadPixelsMutex and release it before the outer + // readSurfacePixels proceeds to grab it. + // TODO: Stop pretending to make GrContext thread-safe for sw rasterization and provide + // a mechanism to make a SkPicture safe for multithreaded sw rasterization. + SkMutex fReadPixelsMutex; + SkMutex fTestPMConversionsMutex; struct CleanUpData { PFCleanUpFunc fFunc; @@ -446,9 +459,13 @@ private: * return NULL. */ const GrFragmentProcessor* createPMToUPMEffect(GrProcessorDataManager*, GrTexture*, - bool swapRAndB, const SkMatrix&); + bool swapRAndB, const SkMatrix&) const; const GrFragmentProcessor* createUPMToPMEffect(GrProcessorDataManager*, GrTexture*, - bool swapRAndB, const SkMatrix&); + bool swapRAndB, const SkMatrix&) const; + /** Called before either of the above two functions to determine the appropriate fragment + processors for conversions. This must be called by readSurfacePixels befor a mutex is taken, + since testingvPM conversions itself will call readSurfacePixels */ + void testPMConversionsIfNecessary(uint32_t flags); /** Returns true if we've already determined that createPMtoUPMEffect and createUPMToPMEffect will fail. In such cases fall back to SW conversion. */ bool didFailPMUPMConversionTest() const; diff --git a/src/gpu/GrContext.cpp b/src/gpu/GrContext.cpp index 4e8060b..6ae96c4 100644 --- a/src/gpu/GrContext.cpp +++ b/src/gpu/GrContext.cpp @@ -329,6 +329,10 @@ bool GrContext::writeSurfacePixels(GrSurface* surface, GrPixelConfig srcConfig, const void* buffer, size_t rowBytes, uint32_t pixelOpsFlags) { RETURN_FALSE_IF_ABANDONED + ASSERT_OWNED_RESOURCE(surface); + SkASSERT(surface); + + this->testPMConversionsIfNecessary(pixelOpsFlags); // Trim the params here so that if we wind up making a temporary surface it can be as small as // necessary and because GrGpu::getWritePixelsInfo requires it. @@ -461,6 +465,9 @@ bool GrContext::readSurfacePixels(GrSurface* src, ASSERT_OWNED_RESOURCE(src); SkASSERT(src); + this->testPMConversionsIfNecessary(flags); + SkAutoMutexAcquire ama(fReadPixelsMutex); + // Adjust the params so that if we wind up using an intermediate surface we've already done // all the trimming and the temporary can be the min size required. if (!GrSurfacePriv::AdjustReadPixelParams(src->width(), src->height(), @@ -694,14 +701,22 @@ void test_pm_conversions(GrContext* ctx, int* pmToUPMValue, int* upmToPMValue) { } } +void GrContext::testPMConversionsIfNecessary(uint32_t flags) { + if (SkToBool(kUnpremul_PixelOpsFlag & flags)) { + SkAutoMutexAcquire ama(fTestPMConversionsMutex); + if (!fDidTestPMConversions) { + test_pm_conversions(this, &fPMToUPMConversion, &fUPMToPMConversion); + fDidTestPMConversions = true; + } + } +} + const GrFragmentProcessor* GrContext::createPMToUPMEffect(GrProcessorDataManager* procDataManager, GrTexture* texture, bool swapRAndB, - const SkMatrix& matrix) { - if (!fDidTestPMConversions) { - test_pm_conversions(this, &fPMToUPMConversion, &fUPMToPMConversion); - fDidTestPMConversions = true; - } + const SkMatrix& matrix) const { + // We should have already called this->testPMConversionsIfNecessary(). + SkASSERT(fDidTestPMConversions); GrConfigConversionEffect::PMConversion pmToUPM = static_cast(fPMToUPMConversion); if (GrConfigConversionEffect::kNone_PMConversion != pmToUPM) { @@ -715,11 +730,9 @@ const GrFragmentProcessor* GrContext::createPMToUPMEffect(GrProcessorDataManager const GrFragmentProcessor* GrContext::createUPMToPMEffect(GrProcessorDataManager* procDataManager, GrTexture* texture, bool swapRAndB, - const SkMatrix& matrix) { - if (!fDidTestPMConversions) { - test_pm_conversions(this, &fPMToUPMConversion, &fUPMToPMConversion); - fDidTestPMConversions = true; - } + const SkMatrix& matrix) const { + // We should have already called this->testPMConversionsIfNecessary(). + SkASSERT(fDidTestPMConversions); GrConfigConversionEffect::PMConversion upmToPM = static_cast(fUPMToPMConversion); if (GrConfigConversionEffect::kNone_PMConversion != upmToPM) { @@ -731,9 +744,10 @@ const GrFragmentProcessor* GrContext::createUPMToPMEffect(GrProcessorDataManager } bool GrContext::didFailPMUPMConversionTest() const { + // We should have already called this->testPMConversionsIfNecessary(). + SkASSERT(fDidTestPMConversions); // The PM<->UPM tests fail or succeed together so we only need to check one. - return fDidTestPMConversions && - GrConfigConversionEffect::kNone_PMConversion == fPMToUPMConversion; + return GrConfigConversionEffect::kNone_PMConversion == fPMToUPMConversion; } ////////////////////////////////////////////////////////////////////////////// -- 2.7.4