Issue 379113004: Makes GrGLProgramDesc's key store the lengths as well as offsets of the effect keys.

Issue 379113004: Makes GrGLProgramDesc's key store the lengths as well as offsets of the effect keys. (Closed)

Created:
6 years, 5 months ago by bsalomon

Modified:
6 years, 5 months ago

Reviewers:
jvanverth1, robertphillips

CC:
reviews_skia.org

Base URL:
https://skia.googlesource.com/skia.git@key

Visibility:
Public.

More Reviews

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 Committed: https://skia.googlesource.com/skia/+/929f29a5c1bffa7f7c1b5a376351d0762b8ac561

Patch Set 4 : address comments #

Total comments: 2

Patch Set 5 : update comment at Rob's suggestion. #

Patch Set 6 : suppress warning #

Patch Set 7 : Fix sizeof(uint32_t) bug #

Created: 6 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+213 lines, -124 lines)			Patch
M	include/gpu/GrBackendEffectFactory.h	View	1 2 3 4	3 chunks	+58 lines, -29 lines	0 comments	Download
M	include/gpu/GrTBackendEffectFactory.h	View	1 2 3	4 chunks	+27 lines, -29 lines	0 comments	Download
M	src/gpu/gl/GrGLProgramDesc.h	View	1 2 3	4 chunks	+24 lines, -6 lines	0 comments	Download
M	src/gpu/gl/GrGLProgramDesc.cpp	View	1 2 3 4 5 6	3 chunks	+53 lines, -31 lines	0 comments	Download
M	src/gpu/gl/GrGLProgramEffects.h	View		3 chunks	+15 lines, -5 lines	0 comments	Download
M	src/gpu/gl/GrGLProgramEffects.cpp	View	1 2 3	1 chunk	+21 lines, -0 lines	0 comments	Download
M	tests/GLProgramsTest.cpp	View		1 chunk	+15 lines, -24 lines	0 comments	Download

Messages

Total messages: 17 (0 generated)

Expand Messages | Collapse Messages

bsalomon

I'll need the effect key lengths in a subsequent change. Requires https://codereview.chromium.org/356513003/ to land first.

6 years, 5 months ago (2014-07-11 16:49:03 UTC) #1

robertphillips

First round https://codereview.chromium.org/379113004/diff/40001/include/gpu/GrBackendEffectFactory.h File include/gpu/GrBackendEffectFactory.h (right): https://codereview.chromium.org/379113004/diff/40001/include/gpu/GrBackendEffectFactory.h#newcode57 include/gpu/GrBackendEffectFactory.h:57: * This sentence seems a bit garbled ...

6 years, 5 months ago (2014-07-11 17:24:59 UTC) #2

bsalomon

https://codereview.chromium.org/379113004/diff/40001/include/gpu/GrBackendEffectFactory.h File include/gpu/GrBackendEffectFactory.h (right): https://codereview.chromium.org/379113004/diff/40001/include/gpu/GrBackendEffectFactory.h#newcode57 include/gpu/GrBackendEffectFactory.h:57: * On 2014/07/11 17:24:58, robertphillips wrote: > This sentence ...

6 years, 5 months ago (2014-07-11 17:52:56 UTC) #3

robertphillips

lgtm + comment suggestion https://codereview.chromium.org/379113004/diff/60001/include/gpu/GrBackendEffectFactory.h File include/gpu/GrBackendEffectFactory.h (right): https://codereview.chromium.org/379113004/diff/60001/include/gpu/GrBackendEffectFactory.h#newcode52 include/gpu/GrBackendEffectFactory.h:52: * effect object. Also tracks ...

6 years, 5 months ago (2014-07-11 18:19:14 UTC) #4

bsalomon

https://codereview.chromium.org/379113004/diff/60001/include/gpu/GrBackendEffectFactory.h File include/gpu/GrBackendEffectFactory.h (right): https://codereview.chromium.org/379113004/diff/60001/include/gpu/GrBackendEffectFactory.h#newcode52 include/gpu/GrBackendEffectFactory.h:52: * effect object. Also tracks equivalence of shaders generated ...

6 years, 5 months ago (2014-07-11 20:00:39 UTC) #5

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/bsalomon@google.com/379113004/80001

6 years, 5 months ago (2014-07-12 04:55:49 UTC) #7

commit-bot: I haz the power

FYI, CQ is re-trying this CL (attempt #1). The failing builders are: Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on tryserver.skia ...

6 years, 5 months ago (2014-07-12 05:11:12 UTC) #8

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

6 years, 5 months ago (2014-07-12 05:26:12 UTC) #9

commit-bot: I haz the power

Try jobs failed on following builders: Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on tryserver.skia (http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot/builds/3236)

6 years, 5 months ago (2014-07-12 05:26:13 UTC) #10

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/bsalomon@google.com/379113004/100001

6 years, 5 months ago (2014-07-15 14:00:50 UTC) #12

commit-bot: I haz the power

Change committed as c0ea398aff8254e31152cbb94c9ab6150428e252

6 years, 5 months ago (2014-07-16 02:41:21 UTC) #13

mtklein

A revert of this CL has been created in https://codereview.chromium.org/394213002/ by mtklein@google.com. The reason for ...

6 years, 5 months ago (2014-07-16 13:16:17 UTC) #14

Message was sent while issue was closed.

A revert of this CL has been created in
https://codereview.chromium.org/394213002/ by mtklein@google.com.

The reason for reverting is: 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)
.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/bsalomon@google.com/379113004/120001

6 years, 5 months ago (2014-07-17 14:43:59 UTC) #16

commit-bot: I haz the power

6 years, 5 months ago (2014-07-17 14:55:15 UTC) #17

Message was sent while issue was closed.

Change committed as 929f29a5c1bffa7f7c1b5a376351d0762b8ac561

Expand Messages | Collapse Messages