Caching the result of readPixelsSupported

The call was calling GR_GL_GetIntegerv 2 times for each readPixels
and thus was causing a loss of performance

(resubmit of issue 344793008)

Benchmark url: http://packages.gkny.fr/tst/index.html

BUG=skia:2681

Committed: https://skia.googlesource.com/skia/+/753a2964afe5661ce9b2a8ca77ca9d0aabd3173c

Committed: https://skia.googlesource.com/skia/+/8339371f1ec3c57a0741932fd96bff32c53d4e54

Review URL: https://codereview.chromium.org/364193004

gyp/core.gypi

@@ -187,6 +187,7 @@
         '<(skia_src_path)/core/SkTextBlob.cpp',
         '<(skia_src_path)/core/SkTextFormatParams.h',
         '<(skia_src_path)/core/SkTextMapStateProc.h',
+        '<(skia_src_path)/core/SkTHashCache.h',
         '<(skia_src_path)/core/SkTileGrid.cpp',
         '<(skia_src_path)/core/SkTileGrid.h',
         '<(skia_src_path)/core/SkTLList.h',

gyp/tests.gypi

@@ -193,6 +193,7 @@
     '../tests/StrokeTest.cpp',
     '../tests/SurfaceTest.cpp',
     '../tests/TArrayTest.cpp',
+    '../tests/THashCache.cpp',
     '../tests/TLSTest.cpp',
     '../tests/TSetTest.cpp',
     '../tests/TextBlobTest.cpp',

src/core/SkTHashCache.h

@@ -0,0 +1,77 @@
+/*
+ * Copyright 2014 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#ifndef SkTHASHCACHE_DEFINED
+#define	SkTHASHCACHE_DEFINED
+
+#include "SkTypes.h"
+#include "SkTDynamicHash.h"
+
+template <typename T,
+typename Key,
+typename Traits = T,
+int kGrowPercent = 75 >
+class SkTHashCache : public SkNoncopyable {
+public:
+
+    SkTHashCache() {
+        this->reset();
+    }
+
+    ~SkTHashCache() {
+        this->clear();
+    }
+
+    T* find(const Key& key) const {
+        return fDict->find(key);
+    }
+
+    /**
+     *  If element already in cache, return immediately the cached value
+     */
+    T& add(const T& add) {
+        Key key = Traits::GetKey(add);
+        if (T* val = this->find(key)) {
+            return *val;
+        }
+
+        T* element = SkNEW_ARGS(T, (add));
+
+        fDict->add(element);
+
+        return *element;
+    }
+
+    int size() const {
+        return fDict->count();
+    }
+
+    void reset() {
+        this->clear();
+
+        fDict.reset(SkNEW(DictType));
+    }
+
+private:
+    typedef SkTDynamicHash<T, Key, Traits, kGrowPercent> DictType;
+
+    void clear() {
+        if (fDict.get()) {
+            typename DictType::Iter it(fDict.get());
+
+            while (!it.done()) {
+                SkDELETE(&(*it));
+                ++it;
+            }
+        }
+    }
+
+    SkAutoTDelete<DictType> fDict;
+};
+
+#endif	/* SkHASHCACHE_DEFINED */
+

src/gpu/gl/GrGLCaps.cpp

@@ -50,6 +50,8 @@ void GrGLCaps::reset() {
     fFBFetchSupport = false;
     fFBFetchColorName = NULL;
     fFBFetchExtensionString = NULL;
+
+    fReadPixelsSupportedCache.reset();
 }
 
 GrGLCaps::GrGLCaps(const GrGLCaps& caps) : GrDrawTargetCaps() {
@@ -601,7 +603,7 @@ void GrGLCaps::initConfigTexturableTable(const GrGLContextInfo& ctxInfo, const G
     fConfigTextureSupport[kRGBA_float_GrPixelConfig] = hasFPTextures;
 }
 
-bool GrGLCaps::readPixelsSupported(const GrGLInterface* intf,
+bool GrGLCaps::doReadPixelsSupported(const GrGLInterface* intf,
                                      GrGLenum format,
                                      GrGLenum type) const {
     if (GR_GL_RGBA == format && GR_GL_UNSIGNED_BYTE == type) {
@@ -630,6 +632,26 @@ bool GrGLCaps::readPixelsSupported(const GrGLInterface* intf,
     return (GrGLenum)otherFormat == format && (GrGLenum)otherType == type;
 }
 
+bool GrGLCaps::readPixelsSupported(const GrGLInterface* intf,
+                                   GrGLenum format,
+                                   GrGLenum type,
+                                   GrGLenum currFboFormat) const {
+
+    ReadPixelsSupportedFormats::Key key = {format, type, currFboFormat};
+
+    ReadPixelsSupportedFormats* cachedValue = fReadPixelsSupportedCache.find(key);
+
+    if (NULL == cachedValue) {
+        bool value = doReadPixelsSupported(intf, format, type);
+        ReadPixelsSupportedFormats newValue(key, value);
+        fReadPixelsSupportedCache.add(newValue);
+
+        return newValue.value();
+    }
+
+    return cachedValue->value();
+}
+
 void GrGLCaps::initFSAASupport(const GrGLContextInfo& ctxInfo, const GrGLInterface* gli) {
 
     fMSFBOType = kNone_MSFBOType;

src/gpu/gl/GrGLCaps.h

@@ -11,8 +11,9 @@
 
 #include "GrDrawTargetCaps.h"
 #include "GrGLStencilBuffer.h"
+#include "SkChecksum.h"
+#include "SkTHashCache.h"
 #include "SkTArray.h"
-#include "SkTDArray.h"
 
 class GrGLContextInfo;
 
@@ -252,7 +253,8 @@ public:
     /// Does ReadPixels support the provided format/type combo?
     bool readPixelsSupported(const GrGLInterface* intf,
                              GrGLenum format,
-                             GrGLenum type) const;
+                             GrGLenum type,
+                             GrGLenum currFboFormat) const;
 
     bool isCoreProfile() const { return fIsCoreProfile; }
 
@@ -323,6 +325,10 @@ private:
     void initConfigRenderableTable(const GrGLContextInfo&);
     void initConfigTexturableTable(const GrGLContextInfo&, const GrGLInterface*);
 
+    bool doReadPixelsSupported(const GrGLInterface* intf,
+                                   GrGLenum format,
+                                   GrGLenum type) const;
+
     // tracks configs that have been verified to pass the FBO completeness when
     // used as a color attachment
     VerifiedColorConfigs fVerifiedColorConfigs;
@@ -366,6 +372,46 @@ private:
     const char* fFBFetchColorName;
     const char* fFBFetchExtensionString;
 
+    class ReadPixelsSupportedFormats {
+    public:
+        struct Key {
+            GrGLenum fFormat;
+            GrGLenum fType;
+            GrGLenum fFboFormat;
+
+            bool operator==(const Key& rhs) const {
+                return fFormat == rhs.fFormat
+                        && fType == rhs.fType
+                        && fFboFormat == rhs.fFboFormat;
+            }
+
+            uint32_t getHash() const {
+                return SkChecksum::Murmur3(reinterpret_cast<const uint32_t*>(this), sizeof(*this));
+            }
+        };
+
+        ReadPixelsSupportedFormats(Key key, bool value) : fKey(key), fValue(value) {
+        }
+
+        static const Key& GetKey(const ReadPixelsSupportedFormats& element) {
+            return element.fKey;
+        }
+
+        static uint32_t Hash(const Key& key) {
+            return key.getHash();
+        }
+
+        bool value() const {
+            return fValue;
+        }
+    private:
+        Key fKey;
+        bool fValue;
+    };
+
+    mutable SkTHashCache<ReadPixelsSupportedFormats,
+                         ReadPixelsSupportedFormats::Key> fReadPixelsSupportedCache;
+
     typedef GrDrawTargetCaps INHERITED;
 };
 

src/gpu/gl/GrGpuGL.cpp

@@ -178,8 +178,6 @@ void GrGpuGL::contextAbandoned() {
 }
 
 ///////////////////////////////////////////////////////////////////////////////
-
-
 GrPixelConfig GrGpuGL::preferredReadPixelsConfig(GrPixelConfig readConfig,
                                                  GrPixelConfig surfaceConfig) const {
     if (GR_GL_RGBA_8888_PIXEL_OPS_SLOW && kRGBA_8888_GrPixelConfig == readConfig) {
@@ -190,9 +188,13 @@ GrPixelConfig GrGpuGL::preferredReadPixelsConfig(GrPixelConfig readConfig,
         // Mesa 3D takes a slow path on when reading back  BGRA from an RGBA surface and vice-versa.
         // Perhaps this should be guarded by some compiletime or runtime check.
         return surfaceConfig;
-    } else if (readConfig == kBGRA_8888_GrPixelConfig &&
+    } else if (readConfig == kBGRA_8888_GrPixelConfig
-               !this->glCaps().readPixelsSupported(this->glInterface(),
+            && !this->glCaps().readPixelsSupported(
-                                                   GR_GL_BGRA, GR_GL_UNSIGNED_BYTE)) {
+                this->glInterface(),
+                GR_GL_BGRA,
+                GR_GL_UNSIGNED_BYTE,
+                surfaceConfig
+            )) {
         return kRGBA_8888_GrPixelConfig;
     } else {
         return readConfig;

tests/THashCache.cpp

@@ -0,0 +1,91 @@
+/*
+ * 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 "SkTHashCache.h"
+
+
+// Tests the SkTHashCache<T> class template.
+
+struct Tint {
+    uint32_t value;
+
+    bool operator==(const Tint& rhs) const {
+        return value == rhs.value;
+    }
+};
+
+class Element {
+public:
+
+    bool operator==(const Element& rhs) const {
+        return value == rhs.value && key == rhs.key;
+    }
+
+    static const Tint& GetKey(const Element& element) {
+        return element.key;
+    }
+
+    static uint32_t Hash(const Tint& key) {
+        return key.value;
+    }
+
+    Element(Tint key, int value) : key(key), value(value) {
+    }
+
+    Tint key;
+    int value;
+};
+
+typedef SkTHashCache<Element, Tint> CacheType;
+
+DEF_TEST(THashCache, reporter) {
+    Tint k11 = {11};
+    Element e11(k11, 22);
+
+    e11.value = e11.value;
+    Element e11Collision(k11, 0);
+    //    Element e42(4, 2);
+
+    //Some tests for the class Element
+    REPORTER_ASSERT(reporter, Element::GetKey(e11) == k11);
+    REPORTER_ASSERT(reporter, Element::Hash(k11) == 11);
+
+    CacheType cache;
+
+    // Is the cahce correctly initialized ?
+    REPORTER_ASSERT(reporter, 0 == cache.size());
+    REPORTER_ASSERT(reporter, NULL == cache.find(k11));
+
+    Element& e11_c = cache.add(e11);
+    e11_c.value = e11_c.value;
+
+    // Tests for simple insertion, verifying that the returned element
+    // has the same values as the original one
+    REPORTER_ASSERT(reporter, 1 == cache.size());
+    REPORTER_ASSERT(reporter, NULL != cache.find(k11));
+    REPORTER_ASSERT(reporter, e11_c == e11);
+
+    Element& e11Collision_c = cache.add(e11Collision);
+    // Verifying that, in case of collision, the element alerady in the cache is not removed
+    REPORTER_ASSERT(reporter, e11Collision_c == e11);
+    REPORTER_ASSERT(reporter, 1 == cache.size());
+
+    Tint k42 = {42};
+    Element e42(k42, 2);
+    cache.add(e42);
+    // Can we add more than one element?
+    REPORTER_ASSERT(reporter, NULL != cache.find(k11));
+    REPORTER_ASSERT(reporter, NULL != cache.find(k42));
+    REPORTER_ASSERT(reporter, 2 == cache.size());
+
+    cache.reset();
+    // Does clear do its job?
+    REPORTER_ASSERT(reporter, 0 == cache.size());
+    REPORTER_ASSERT(reporter, NULL == cache.find(k11));
+    REPORTER_ASSERT(reporter, NULL == cache.find(k42));
+}