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

2 files changed, 45 insertions, 14 deletions

diff --git a/include/gpu/GrContext.h b/include/gpu/GrContext.h
index 7f1e4109fa..64f4f6ce30 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 4e8060b187..6ae96c498b 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<GrConfigConversionEffect::PMConversion>(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<GrConfigConversionEffect::PMConversion>(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;
 }
 
 //////////////////////////////////////////////////////////////////////////////