Do premul and r/b swap conversions in a custom effect

[bsalomon, 2012-08-24 21:50:23]

Sorry for the big CL :( Passes all the unit tests.

[TomH, 2012-08-24 21:59:00]

Exciting...

[robertphillips, 2012-08-27 12:19:23]

LGTM with minor questions & comment suggestions

http://codereview.appspot.com/6473060/diff/9002/include/gpu/GrSamplerState.h
File include/gpu/GrSamplerState.h (left):

http://codereview.appspot.com/6473060/diff/9002/include/gpu/GrSamplerState.h#...
include/gpu/GrSamplerState.h:103: bool operator ==(const GrSamplerState& s)
const {
Should fSwapRAndB and fPremultiply play here?

http://codereview.appspot.com/6473060/diff/9002/include/gpu/GrSamplerState.h#...
include/gpu/GrSamplerState.h:123: fMatrix = s.fMatrix;
Copy premultiply here?

http://codereview.appspot.com/6473060/diff/9002/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

http://codereview.appspot.com/6473060/diff/9002/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:1240: // This should only be called with the result of
grconfig_to_config8888
Add "// It returns a configuration with swapped R & B components"?

http://codereview.appspot.com/6473060/diff/9002/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:1291: if (unpremul && kRGBA_8888_GrPixelConfig != config
&& kBGRA_8888_GrPixelConfig != config) {
comment on why returning?

http://codereview.appspot.com/6473060/diff/9002/src/gpu/effects/GrConfigConve...
File src/gpu/effects/GrConfigConversionEffect.cpp (right):

http://codereview.appspot.com/6473060/diff/9002/src/gpu/effects/GrConfigConve...
src/gpu/effects/GrConfigConversionEffect.cpp:121: // Fill with every possible
premultiplied A, color channel value. There will be 256-y duplicate
identically

http://codereview.appspot.com/6473060/diff/9002/src/gpu/effects/GrConfigConve...
src/gpu/effects/GrConfigConversionEffect.cpp:169: // We do a PM->UPM draw from
dataTex to readTex and read the data. Then we do a UPM->PM draw
followed _by a_

http://codereview.appspot.com/6473060/diff/9002/src/gpu/effects/GrConfigConve...
src/gpu/effects/GrConfigConversionEffect.cpp:217: if (!swapRedAndBlue &&
kNone_PMConversion == pmConversion) {
This comment doesn't seem quite right?

http://codereview.appspot.com/6473060/diff/9002/src/gpu/effects/GrConfigConve...
File src/gpu/effects/GrConfigConversionEffect.h (right):

http://codereview.appspot.com/6473060/diff/9002/src/gpu/effects/GrConfigConve...
src/gpu/effects/GrConfigConversionEffect.h:17: * unpremultiplied. Also on some
systems reading/writing BGRA or RGBA is faster. In those cases we
we we -> we

http://codereview.appspot.com/6473060/diff/9002/src/gpu/effects/GrConfigConve...
src/gpu/effects/GrConfigConversionEffect.h:35: 
are -> is

[bsalomon, 2012-08-27 12:53:37]

Checked in at r5284.

https://codereview.appspot.com/6473060/diff/9002/include/gpu/GrSamplerState.h
File include/gpu/GrSamplerState.h (left):

https://codereview.appspot.com/6473060/diff/9002/include/gpu/GrSamplerState.h...
include/gpu/GrSamplerState.h:103: bool operator ==(const GrSamplerState& s)
const {
On 2012/08/27 12:19:23, robertphillips wrote:
> Should fSwapRAndB and fPremultiply play here?

They were in the "bitwiseRegion" I think... but I've removed them.

https://codereview.appspot.com/6473060/diff/9002/include/gpu/GrSamplerState.h...
include/gpu/GrSamplerState.h:123: fMatrix = s.fMatrix;
On 2012/08/27 12:19:23, robertphillips wrote:
> Copy premultiply here?

It should have been here... but it's gone now, so oh well! :)

https://codereview.appspot.com/6473060/diff/9002/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.appspot.com/6473060/diff/9002/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1240: // This should only be called with the result of
grconfig_to_config8888
On 2012/08/27 12:19:23, robertphillips wrote:
> Add "// It returns a configuration with swapped R & B components"?

Done.

https://codereview.appspot.com/6473060/diff/9002/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1291: if (unpremul && kRGBA_8888_GrPixelConfig != config
&& kBGRA_8888_GrPixelConfig != config) {
On 2012/08/27 12:19:23, robertphillips wrote:
> comment on why returning?

Done:
// The unpremul flag is only allowed for these two configs.

https://codereview.appspot.com/6473060/diff/9002/src/gpu/effects/GrConfigConv...
File src/gpu/effects/GrConfigConversionEffect.cpp (right):

https://codereview.appspot.com/6473060/diff/9002/src/gpu/effects/GrConfigConv...
src/gpu/effects/GrConfigConversionEffect.cpp:121: // Fill with every possible
premultiplied A, color channel value. There will be 256-y duplicate
On 2012/08/27 12:19:23, robertphillips wrote:
> identically

Done.

https://codereview.appspot.com/6473060/diff/9002/src/gpu/effects/GrConfigConv...
src/gpu/effects/GrConfigConversionEffect.cpp:169: // We do a PM->UPM draw from
dataTex to readTex and read the data. Then we do a UPM->PM draw
On 2012/08/27 12:19:23, robertphillips wrote:
> followed _by a_

Done.

https://codereview.appspot.com/6473060/diff/9002/src/gpu/effects/GrConfigConv...
src/gpu/effects/GrConfigConversionEffect.cpp:217: if (!swapRedAndBlue &&
kNone_PMConversion == pmConversion) {
On 2012/08/27 12:19:23, robertphillips wrote:
> This comment doesn't seem quite right?

Changed to:
// If we returned a GrConfigConversionEffect that was equivalent to a
GrSingleTextureEffect
// then we may pollute our texture cache with redundant shaders. So in the case
that no
// conversions were requested we instead return a GrSingleTextureEffect.

https://codereview.appspot.com/6473060/diff/9002/src/gpu/effects/GrConfigConv...
File src/gpu/effects/GrConfigConversionEffect.h (right):

https://codereview.appspot.com/6473060/diff/9002/src/gpu/effects/GrConfigConv...
src/gpu/effects/GrConfigConversionEffect.h:17: * unpremultiplied. Also on some
systems reading/writing BGRA or RGBA is faster. In those cases we
On 2012/08/27 12:19:23, robertphillips wrote:
> we we -> we

Done.

https://codereview.appspot.com/6473060/diff/9002/src/gpu/effects/GrConfigConv...
src/gpu/effects/GrConfigConversionEffect.h:35: 
On 2012/08/27 12:19:23, robertphillips wrote:
> are -> is

Done.

[bsalomon, 2012-08-28 15:22:35]

Closed with r5284.

[TomH, 2012-09-07 09:21:37]

https://codereview.appspot.com/6473060/diff/12002/include/gpu/GrSamplerState.h
File include/gpu/GrSamplerState.h (left):

https://codereview.appspot.com/6473060/diff/12002/include/gpu/GrSamplerState....
include/gpu/GrSamplerState.h:124: fSwapRAndB = s.fSwapRAndB;
Were we not copying fPremultiply here? And yet we saw no bugs?

[bsalomon, 2012-09-07 12:32:12]

https://codereview.appspot.com/6473060/diff/12002/include/gpu/GrSamplerState.h
File include/gpu/GrSamplerState.h (left):

https://codereview.appspot.com/6473060/diff/12002/include/gpu/GrSamplerState....
include/gpu/GrSamplerState.h:124: fSwapRAndB = s.fSwapRAndB;
On 2012/09/07 09:21:37, TomH wrote:
> Were we not copying fPremultiply here? And yet we saw no bugs?

I think it was only being used with draws that went directly to GrGpu, not to
GrIODB. So the draw state wasn't getting copied when fPremultiply was true.
Also, this code was very short-lived. It was an interim step to getting the
conversion custom effect in place.