Revert of Reopened: Caching the result of readPixelsSupported (https://codereview.chromium.org/364193004/)
Reason for revert:
This appears to be causing failures on Android when running tests.
Original issue's description:
> 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
R=junov@chromium.org, reed@chromium.org, bsalomon@chromium.org, mtklein@google.com, bsalomon@google.com, piotaixr@chromium.org
TBR=bsalomon@chromium.org, bsalomon@google.com, junov@chromium.org, mtklein@google.com, piotaixr@chromium.org, reed@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=skia:2681
Author: bungeman@google.com
Review URL: https://codereview.chromium.org/395203002
diff --git a/gyp/core.gypi b/gyp/core.gypi
index f321f01..f7853f9 100644
--- a/gyp/core.gypi
+++ b/gyp/core.gypi
@@ -199,7 +199,6 @@
'<(skia_src_path)/core/SkStrokerPriv.h',
'<(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',
diff --git a/gyp/tests.gypi b/gyp/tests.gypi
index 8542868..ec12461 100644
--- a/gyp/tests.gypi
+++ b/gyp/tests.gypi
@@ -174,7 +174,6 @@
'../tests/StrokeTest.cpp',
'../tests/SurfaceTest.cpp',
'../tests/TArrayTest.cpp',
- '../tests/THashCache.cpp',
'../tests/TLSTest.cpp',
'../tests/TSetTest.cpp',
'../tests/TextureCompressionTest.cpp',
diff --git a/src/core/SkTHashCache.h b/src/core/SkTHashCache.h
deleted file mode 100644
index cfee972..0000000
--- a/src/core/SkTHashCache.h
+++ /dev/null
@@ -1,77 +0,0 @@
-/*
- * 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 */
-
diff --git a/src/gpu/gl/GrGLCaps.cpp b/src/gpu/gl/GrGLCaps.cpp
index 89d873b..8f2d4c1 100644
--- a/src/gpu/gl/GrGLCaps.cpp
+++ b/src/gpu/gl/GrGLCaps.cpp
@@ -48,8 +48,6 @@
fIsCoreProfile = false;
fFullClearIsFree = false;
fDropsTileOnZeroDivide = false;
-
- fReadPixelsSupportedCache.reset();
}
GrGLCaps::GrGLCaps(const GrGLCaps& caps) : GrDrawTargetCaps() {
@@ -506,7 +504,7 @@
// First check version for support
if (kGL_GrGLStandard == standard) {
hasETC1 = hasCompressTex2D &&
- (version >= GR_GL_VER(4, 3) ||
+ (version >= GR_GL_VER(4, 3) ||
ctxInfo.hasExtension("GL_ARB_ES3_compatibility"));
} else {
hasETC1 = hasCompressTex2D &&
@@ -562,9 +560,9 @@
}
}
-bool GrGLCaps::doReadPixelsSupported(const GrGLInterface* intf,
- GrGLenum format,
- GrGLenum type) const {
+bool GrGLCaps::readPixelsSupported(const GrGLInterface* intf,
+ GrGLenum format,
+ GrGLenum type) const {
if (GR_GL_RGBA == format && GR_GL_UNSIGNED_BYTE == type) {
// ES 2 guarantees this format is supported
return true;
@@ -591,26 +589,6 @@
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;
diff --git a/src/gpu/gl/GrGLCaps.h b/src/gpu/gl/GrGLCaps.h
index 2d391b1..ccf04fd 100644
--- a/src/gpu/gl/GrGLCaps.h
+++ b/src/gpu/gl/GrGLCaps.h
@@ -11,9 +11,8 @@
#include "GrDrawTargetCaps.h"
#include "GrGLStencilBuffer.h"
-#include "SkChecksum.h"
-#include "SkTHashCache.h"
#include "SkTArray.h"
+#include "SkTDArray.h"
class GrGLContextInfo;
@@ -254,8 +253,7 @@
/// Does ReadPixels support the provided format/type combo?
bool readPixelsSupported(const GrGLInterface* intf,
GrGLenum format,
- GrGLenum type,
- GrGLenum currFboFormat) const;
+ GrGLenum type) const;
bool isCoreProfile() const { return fIsCoreProfile; }
@@ -326,10 +324,6 @@
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;
@@ -370,46 +364,6 @@
bool fFullClearIsFree : 1;
bool fDropsTileOnZeroDivide : 1;
- 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;
};
diff --git a/src/gpu/gl/GrGpuGL.cpp b/src/gpu/gl/GrGpuGL.cpp
index 3eca3d6..eddccc3 100644
--- a/src/gpu/gl/GrGpuGL.cpp
+++ b/src/gpu/gl/GrGpuGL.cpp
@@ -170,6 +170,8 @@
}
///////////////////////////////////////////////////////////////////////////////
+
+
GrPixelConfig GrGpuGL::preferredReadPixelsConfig(GrPixelConfig readConfig,
GrPixelConfig surfaceConfig) const {
if (GR_GL_RGBA_8888_PIXEL_OPS_SLOW && kRGBA_8888_GrPixelConfig == readConfig) {
@@ -180,13 +182,9 @@
// 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
- && this->glCaps().readPixelsSupported(
- this->glInterface(),
- GR_GL_BGRA,
- GR_GL_UNSIGNED_BYTE,
- surfaceConfig
- )) {
+ } else if (readConfig == kBGRA_8888_GrPixelConfig &&
+ !this->glCaps().readPixelsSupported(this->glInterface(),
+ GR_GL_BGRA, GR_GL_UNSIGNED_BYTE)) {
return kRGBA_8888_GrPixelConfig;
} else {
return readConfig;
@@ -715,7 +713,7 @@
}
// TODO: This function is using a lot of wonky semantics like, if width == -1
-// then set width = desc.fWdith ... blah. A better way to do it might be to
+// then set width = desc.fWdith ... blah. A better way to do it might be to
// create a CompressedTexData struct that takes a desc/ptr and figures out
// the proper upload semantics. Then users can construct this function how they
// see fit if they want to go against the "standard" way to do it.
diff --git a/tests/THashCache.cpp b/tests/THashCache.cpp
deleted file mode 100644
index 7797431..0000000
--- a/tests/THashCache.cpp
+++ /dev/null
@@ -1,91 +0,0 @@
-/*
- * 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));
-}
