diff options
author | mtklein <mtklein@google.com> | 2014-07-16 06:16:43 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2014-07-16 06:16:43 -0700 |
commit | 7940100faec0b758645d40c876e9c796884410f7 (patch) | |
tree | 42d23e21b906e0eba4ccddb7263045f7b8b59ae0 /src | |
parent | e9d2d09ea53d20828d2f5320fc1f19a9952f2b31 (diff) |
Revert of Makes GrGLProgramDesc's key store the lengths as well as offsets of the effect keys. (https://codereview.chromium.org/379113004/)
Reason for revert:
Most likely candidate for Valgrind failures:
[21:10:08.755668] ==3036== Use of uninitialised value of size 8
[21:10:08.755753] ==3036== at 0x734AB2: GrGpuGL::ProgramCache::getProgram(GrGLProgramDesc const&, GrEffectStage const**, GrEffectStage const**) (GrGpuGL_program.cpp:107)
[21:10:08.755788] ==3036== by 0x734ED2: GrGpuGL::flushGraphicsState(GrGpu::DrawType, GrDeviceCoordTexture const*) (GrGpuGL_program.cpp:253)
[21:10:08.755811] ==3036== by 0x6E81C2: GrGpu::setupClipAndFlushState(GrGpu::DrawType, GrDeviceCoordTexture const*, GrDrawState::AutoRestoreEffects*, SkRect const*) (GrGpu.cpp:350)
[21:10:08.755837] ==3036== by 0x6E9BE8: GrGpu::onDraw(GrDrawTarget::DrawInfo const&) (GrGpu.cpp:390)
[21:10:08.755858] ==3036== by 0x6EEECE: GrInOrderDrawBuffer::flush() (GrDrawTarget.h:506)
[21:10:08.755879] ==3036== by 0x6D0EB4: GrContext::flush(int) (GrContext.cpp:1327)
[21:10:08.755900] ==3036== by 0x6D3F8F: GrContext::writeTexturePixels(GrTexture*, int, int, int, int, GrPixelConfig, void const*, unsigned long, unsigned int) (GrContext.cpp:1349)
[21:10:08.755922] ==3036== by 0x6D39D7: GrContext::writeRenderTargetPixels(GrRenderTarget*, int, int, int, int, GrPixelConfig, void const*, unsigned long, unsigned int) (GrContext.cpp:1632)
[21:10:08.755949] ==3036== by 0x6FFDF3: GrRenderTarget::writePixels(int, int, int, int, GrPixelConfig, void const*, unsigned long, unsigned int) (GrRenderTarget.cpp:45)
[21:10:08.755978] ==3036== by 0x735563: SkGpuDevice::onWritePixels(SkImageInfo const&, void const*, unsigned long, int, int) (SkGpuDevice.cpp:280)
[21:10:08.756003] ==3036== by 0x57A048: SkBaseDevice::writePixels(SkImageInfo const&, void const*, unsigned long, int, int) (SkDevice.cpp:106)
[21:10:08.756025] ==3036== by 0x56D0AE: SkCanvas::writePixels(SkImageInfo const&, void const*, unsigned long, int, int) (SkCanvas.cpp:700)
[21:10:08.756050] ==3036== by 0x56D156: SkCanvas::writePixels(SkBitmap const&, int, int) (SkCanvas.cpp:652)
[21:10:08.756077] ==3036== by 0x5109B6: test_WritePixels(skiatest::Reporter*, GrContextFactory*) (WritePixelsTest.cpp:464)
[21:10:08.756099] ==3036== by 0x51114C: skiatest::WritePixelsClass::onRun(skiatest::Reporter*) (WritePixelsTest.cpp:361)
[21:10:08.756122] ==3036== by 0x406BE8: skiatest::Test::run() (Test.cpp:107)
[21:10:08.756145] ==3036== by 0x4064C2: SkTestRunnable::run() (skia_test.cpp:109)
[21:10:08.756167] ==3036== by 0x405D1A: tool_main(int, char**) (skia_test.cpp:221)
[21:10:08.756189] ==3036== by 0x405F75: main (skia_test.cpp:239)
[21:10:08.756211] ==3036== Uninitialised value was created by a stack allocation
[21:10:08.756233] ==3036== at 0x734CC8: GrGpuGL::flushGraphicsState(GrGpu::DrawType, GrDeviceCoordTexture const*) (GrGpuGL_program.cpp:213)
Original issue's description:
> Makes GrGLProgramDesc's key store the lengths as well as offsets of the effect keys.
>
> Makes it possible to use GrBackendEffectFactories other than GrTBEF by moving meta-key generation out of GrTBEF.
>
> Cleans up docs around GrBackendEffectFactory.
>
> Committed: https://skia.googlesource.com/skia/+/c0ea398aff8254e31152cbb94c9ab6150428e252
R=robertphillips@google.com, jvanverth@google.com, bsalomon@google.com
TBR=bsalomon@google.com, jvanverth@google.com, robertphillips@google.com
NOTREECHECKS=true
NOTRY=true
Author: mtklein@google.com
Review URL: https://codereview.chromium.org/394213002
Diffstat (limited to 'src')
-rw-r--r-- | src/gpu/gl/GrGLProgramDesc.cpp | 84 | ||||
-rw-r--r-- | src/gpu/gl/GrGLProgramDesc.h | 30 | ||||
-rw-r--r-- | src/gpu/gl/GrGLProgramEffects.cpp | 21 | ||||
-rw-r--r-- | src/gpu/gl/GrGLProgramEffects.h | 20 |
4 files changed, 42 insertions, 113 deletions
diff --git a/src/gpu/gl/GrGLProgramDesc.cpp b/src/gpu/gl/GrGLProgramDesc.cpp index 1807b84888..2c260cda52 100644 --- a/src/gpu/gl/GrGLProgramDesc.cpp +++ b/src/gpu/gl/GrGLProgramDesc.cpp @@ -14,14 +14,13 @@ #include "SkChecksum.h" -bool GrGLProgramDesc::GetEffectKeyAndUpdateStats(const GrEffectStage& stage, - const GrGLCaps& caps, - bool useExplicitLocalCoords, - GrEffectKeyBuilder* b, - uint16_t* effectKeySize, - bool* setTrueIfReadsDst, - bool* setTrueIfReadsPos, - bool* setTrueIfHasVertexCode) { +static inline bool get_key_and_update_stats(const GrEffectStage& stage, + const GrGLCaps& caps, + bool useExplicitLocalCoords, + GrEffectKeyBuilder* b, + bool* setTrueIfReadsDst, + bool* setTrueIfReadsPos, + bool* setTrueIfHasVertexCode) { const GrBackendEffectFactory& factory = stage.getEffect()->getFactory(); GrDrawEffect drawEffect(stage, useExplicitLocalCoords); if (stage.getEffect()->willReadDstColor()) { @@ -33,17 +32,7 @@ bool GrGLProgramDesc::GetEffectKeyAndUpdateStats(const GrEffectStage& stage, if (stage.getEffect()->hasVertexCode()) { *setTrueIfHasVertexCode = true; } - factory.getGLEffectKey(drawEffect, caps, b); - size_t size = b->size(); - if (size > SK_MaxU16) { - *effectKeySize = 0; // suppresses a warning. - return false; - } - *effectKeySize = SkToU16(size); - if (!GrGLProgramEffects::GenEffectMetaKey(drawEffect, caps, b)) { - return false; - } - return true; + return factory.getGLEffectKey(drawEffect, caps, b); } bool GrGLProgramDesc::Build(const GrDrawState& drawState, @@ -116,54 +105,43 @@ bool GrGLProgramDesc::Build(const GrDrawState& drawState, if (!skipCoverage) { numStages += drawState.numCoverageStages() - firstEffectiveCoverageStage; } - GR_STATIC_ASSERT(0 == kEffectKeyOffsetsAndLengthOffset % sizeof(uint32_t)); + GR_STATIC_ASSERT(0 == kEffectKeyLengthsOffset % sizeof(uint32_t)); // Make room for everything up to and including the array of offsets to effect keys. desc->fKey.reset(); - desc->fKey.push_back_n(kEffectKeyOffsetsAndLengthOffset + 2 * sizeof(uint32_t) * numStages); + desc->fKey.push_back_n(kEffectKeyLengthsOffset + sizeof(uint32_t) * numStages); - int offsetAndSizeIndex = 0; + size_t offset = desc->fKey.count(); + int offsetIndex = 0; bool effectKeySuccess = true; if (!skipColor) { for (int s = firstEffectiveColorStage; s < drawState.numColorStages(); ++s) { - uint16_t* offsetAndSize = - reinterpret_cast<uint16_t*>(desc->fKey.begin() + kEffectKeyOffsetsAndLengthOffset + - offsetAndSizeIndex * 2 * sizeof(uint16_t)); + uint32_t* offsetLocation = reinterpret_cast<uint32_t*>(desc->fKey.begin() + + kEffectKeyLengthsOffset + + offsetIndex * sizeof(uint32_t)); + *offsetLocation = offset; + ++offsetIndex; GrEffectKeyBuilder b(&desc->fKey); - uint16_t effectKeySize; - uint32_t effectOffset = desc->fKey.count(); - effectKeySuccess |= GetEffectKeyAndUpdateStats( - drawState.getColorStage(s), gpu->glCaps(), - requiresLocalCoordAttrib, &b, - &effectKeySize, &readsDst, - &readFragPosition, &hasVertexCode); - effectKeySuccess |= (effectOffset <= SK_MaxU16); - - offsetAndSize[0] = SkToU16(effectOffset); - offsetAndSize[1] = effectKeySize; - ++offsetAndSizeIndex; + effectKeySuccess |= get_key_and_update_stats(drawState.getColorStage(s), gpu->glCaps(), + requiresLocalCoordAttrib, &b, &readsDst, + &readFragPosition, &hasVertexCode); + offset += b.size(); } } if (!skipCoverage) { for (int s = firstEffectiveCoverageStage; s < drawState.numCoverageStages(); ++s) { - uint16_t* offsetAndSize = - reinterpret_cast<uint16_t*>(desc->fKey.begin() + kEffectKeyOffsetsAndLengthOffset + - offsetAndSizeIndex * 2 * sizeof(uint16_t)); - + uint32_t* offsetLocation = reinterpret_cast<uint32_t*>(desc->fKey.begin() + + kEffectKeyLengthsOffset + + offsetIndex * sizeof(uint32_t)); + *offsetLocation = offset; + ++offsetIndex; GrEffectKeyBuilder b(&desc->fKey); - uint16_t effectKeySize; - uint32_t effectOffset = desc->fKey.count(); - effectKeySuccess |= GetEffectKeyAndUpdateStats( - drawState.getCoverageStage(s), gpu->glCaps(), - requiresLocalCoordAttrib, &b, - &effectKeySize, &readsDst, - &readFragPosition, &hasVertexCode); - effectKeySuccess |= (effectOffset <= SK_MaxU16); - - offsetAndSize[0] = SkToU16(effectOffset); - offsetAndSize[1] = effectKeySize; - ++offsetAndSizeIndex; + effectKeySuccess |= get_key_and_update_stats(drawState.getCoverageStage(s), + gpu->glCaps(), requiresLocalCoordAttrib, + &b, &readsDst, &readFragPosition, + &hasVertexCode); + offset += b.size(); } } if (!effectKeySuccess) { diff --git a/src/gpu/gl/GrGLProgramDesc.h b/src/gpu/gl/GrGLProgramDesc.h index c8aae19503..d7652f473c 100644 --- a/src/gpu/gl/GrGLProgramDesc.h +++ b/src/gpu/gl/GrGLProgramDesc.h @@ -172,20 +172,14 @@ private: // 1. uint32_t for total key length. // 2. uint32_t for a checksum. // 3. Header struct defined above. - // 4. An array of offsets to effect keys and their sizes (see 5). uint16_t for each - // offset and size. + // 4. uint32_t offsets to beginning of every effects' key (see 5). // 5. per-effect keys. Each effect's key is a variable length array of uint32_t. enum { - // Part 1. kLengthOffset = 0, - // Part 2. kChecksumOffset = kLengthOffset + sizeof(uint32_t), - // Part 3. kHeaderOffset = kChecksumOffset + sizeof(uint32_t), kHeaderSize = SkAlign4(sizeof(KeyHeader)), - // Part 4. - // This is the offset in the overall key to the array of per-effect offset,length pairs. - kEffectKeyOffsetsAndLengthOffset = kHeaderOffset + kHeaderSize, + kEffectKeyLengthsOffset = kHeaderOffset + kHeaderSize, }; template<typename T, size_t OFFSET> T* atOffset() { @@ -200,16 +194,6 @@ private: KeyHeader* header() { return this->atOffset<KeyHeader, kHeaderOffset>(); } - // Shared code between setRandom() and Build(). - static bool GetEffectKeyAndUpdateStats(const GrEffectStage& stage, - const GrGLCaps& caps, - bool useExplicitLocalCoords, - GrEffectKeyBuilder* b, - uint16_t* effectKeySize, - bool* setTrueIfReadsDst, - bool* setTrueIfReadsPos, - bool* setTrueIfHasVertexCode); - void finalize(); const KeyHeader& getHeader() const { return *this->atOffset<KeyHeader, kHeaderOffset>(); } @@ -228,11 +212,9 @@ private: } EffectKey get(int index) const { - const uint16_t* offsets = reinterpret_cast<const uint16_t*>( - fDesc->fKey.begin() + kEffectKeyOffsetsAndLengthOffset); - // We store two uint16_ts per effect, one for the offset to the effect's key and one for - // its length. Here we just need the offset. - uint16_t offset = offsets[2 * (fBaseIndex + index)]; + const uint32_t* offsets = reinterpret_cast<const uint32_t*>(fDesc->fKey.begin() + + kEffectKeyLengthsOffset); + uint32_t offset = offsets[fBaseIndex + index]; return *reinterpret_cast<const EffectKey*>(fDesc->fKey.begin() + offset); } private: @@ -243,7 +225,7 @@ private: enum { kMaxPreallocEffects = 8, kIntsPerEffect = 4, // This is an overestimate of the average effect key size. - kPreAllocSize = kEffectKeyOffsetsAndLengthOffset + + kPreAllocSize = kEffectKeyLengthsOffset + kMaxPreallocEffects * sizeof(uint32_t) * kIntsPerEffect, }; diff --git a/src/gpu/gl/GrGLProgramEffects.cpp b/src/gpu/gl/GrGLProgramEffects.cpp index 9936aa54ad..65d14fde11 100644 --- a/src/gpu/gl/GrGLProgramEffects.cpp +++ b/src/gpu/gl/GrGLProgramEffects.cpp @@ -114,27 +114,6 @@ SkMatrix get_transform_matrix(const GrDrawEffect& drawEffect, int transformIdx) //////////////////////////////////////////////////////////////////////////////// -bool GrGLProgramEffects::GenEffectMetaKey(const GrDrawEffect& drawEffect, const GrGLCaps& caps, - GrEffectKeyBuilder* b) { - - EffectKey textureKey = GrGLProgramEffects::GenTextureKey(drawEffect, caps); - EffectKey transformKey = GrGLProgramEffects::GenTransformKey(drawEffect); - EffectKey attribKey = GrGLProgramEffects::GenAttribKey(drawEffect); - uint32_t classID = drawEffect.effect()->getFactory().effectClassID(); - - // Currently we allow 16 bits for each of the above portions of the meta-key. Fail if they - // don't fit. - static const uint32_t kMetaKeyInvalidMask = ~((uint32_t) SK_MaxU16); - if ((textureKey | transformKey | attribKey | classID) & kMetaKeyInvalidMask) { - return false; - } - - uint32_t* key = b->add32n(2); - key[0] = (textureKey << 16 | transformKey); - key[1] = (classID << 16 | attribKey); - return true; -} - EffectKey GrGLProgramEffects::GenAttribKey(const GrDrawEffect& drawEffect) { EffectKey key = 0; int numAttributes = drawEffect.getVertexAttribIndexCount(); diff --git a/src/gpu/gl/GrGLProgramEffects.h b/src/gpu/gl/GrGLProgramEffects.h index c4d884392a..5a2fefd7d4 100644 --- a/src/gpu/gl/GrGLProgramEffects.h +++ b/src/gpu/gl/GrGLProgramEffects.h @@ -9,9 +9,9 @@ #define GrGLProgramEffects_DEFINED #include "GrBackendEffectFactory.h" -#include "GrGLUniformManager.h" #include "GrTexture.h" #include "GrTextureAccess.h" +#include "GrGLUniformManager.h" class GrEffect; class GrEffectStage; @@ -31,14 +31,11 @@ public: typedef GrGLUniformManager::UniformHandle UniformHandle; /** - * This class emits some of the code inserted into the shaders for an effect. The code it - * creates may be dependent on properties of the effect that the effect itself doesn't use - * in its key (e.g. the pixel format of textures used). So this class inserts a meta-key for - * every effect using this function. It is also responsible for inserting the effect's class ID - * which must be different for every GrEffect subclass. It can fail if an effect uses too many - * textures, attributes, etc for the space allotted in the meta-key. + * These methods generate different portions of an effect's final key. */ - static bool GenEffectMetaKey(const GrDrawEffect&, const GrGLCaps&, GrEffectKeyBuilder*); + static EffectKey GenAttribKey(const GrDrawEffect&); + static EffectKey GenTransformKey(const GrDrawEffect&); + static EffectKey GenTextureKey(const GrDrawEffect&, const GrGLCaps&); virtual ~GrGLProgramEffects(); @@ -101,13 +98,6 @@ public: typedef SkTArray<TextureSampler> TextureSamplerArray; protected: - /** - * Helpers for GenEffectMetaKey. - */ - static EffectKey GenAttribKey(const GrDrawEffect&); - static EffectKey GenTransformKey(const GrDrawEffect&); - static EffectKey GenTextureKey(const GrDrawEffect&, const GrGLCaps&); - GrGLProgramEffects(int reserveCount) : fGLEffects(reserveCount) , fSamplers(reserveCount) { |