Adding repeat mode to texture domain

Adding repeat mode to texture domain

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/5ae5fc59b27a48711e514b3ede548b228e393e9b

[joshua.litt, 2014-07-28 21:18:58]

also includes wiring up of this functionality in GrMatrixConvolution

[bsalomon, 2014-07-28 21:27:45]

Can you add a test case to gm/texturedomaineffect.cpp? (It's one of several
magic GMs that directly exercise effects without going through the public Skia
API).

https://codereview.chromium.org/422123003/diff/40001/src/gpu/effects/GrMatrix...
File src/gpu/effects/GrMatrixConvolutionEffect.cpp (right):

https://codereview.chromium.org/422123003/diff/40001/src/gpu/effects/GrMatrix...
src/gpu/effects/GrMatrixConvolutionEffect.cpp:121: uint32_t key =
m.kernelSize().width() << 16 | m.kernelSize().height();
How about an assertion here that the width and height will fit?

https://codereview.chromium.org/422123003/diff/40001/src/gpu/effects/GrTextur...
File src/gpu/effects/GrTextureDomain.cpp (right):

https://codereview.chromium.org/422123003/diff/40001/src/gpu/effects/GrTextur...
src/gpu/effects/GrTextureDomain.cpp:133: default:
I think we've moved to not having a default if all the values for the enum have
been listed in cases. At least one of the compilers, clang I think, will give us
a warning if we have omitted an enum value.

[joshua.litt, 2014-07-28 22:25:23]

feedback incorporated.  It looks like texturedomaineffect was already setup to
automatically generate the new testcase but it did not adjust the bounds.

On 2014/07/28 21:27:45, bsalomon wrote:
> Can you add a test case to gm/texturedomaineffect.cpp? (It's one of several
> magic GMs that directly exercise effects without going through the public Skia
> API).
> 
>
https://codereview.chromium.org/422123003/diff/40001/src/gpu/effects/GrMatrix...
> File src/gpu/effects/GrMatrixConvolutionEffect.cpp (right):
> 
>
https://codereview.chromium.org/422123003/diff/40001/src/gpu/effects/GrMatrix...
> src/gpu/effects/GrMatrixConvolutionEffect.cpp:121: uint32_t key =
> m.kernelSize().width() << 16 | m.kernelSize().height();
> How about an assertion here that the width and height will fit?
> 
>
https://codereview.chromium.org/422123003/diff/40001/src/gpu/effects/GrTextur...
> File src/gpu/effects/GrTextureDomain.cpp (right):
> 
>
https://codereview.chromium.org/422123003/diff/40001/src/gpu/effects/GrTextur...
> src/gpu/effects/GrTextureDomain.cpp:133: default:
> I think we've moved to not having a default if all the values for the enum
have
> been listed in cases. At least one of the compilers, clang I think, will give
us
> a warning if we have omitted an enum value.

[bsalomon, 2014-07-29 13:03:44]

lgtm

[bsalomon, 2014-07-29 13:04:23]

On 2014/07/29 13:03:44, bsalomon wrote:
> lgtm

Probably want to put texture_domain_effect in expectations/gm/ignored-tests.txt
when you land this.

[Stephen White, 2014-07-29 14:51:16]

LGTM with nits

https://codereview.chromium.org/422123003/diff/140001/src/gpu/effects/GrMatri...
File src/gpu/effects/GrMatrixConvolutionEffect.cpp (right):

https://codereview.chromium.org/422123003/diff/140001/src/gpu/effects/GrMatri...
src/gpu/effects/GrMatrixConvolutionEffect.cpp:96: builder->fsCodeAppendf("\t\tk
= %s[%d * %d + %d];\n", kernel, y, kWidth, x);
Nit: I think you could declare "float k" inline here to narrow scope.

https://codereview.chromium.org/422123003/diff/140001/src/gpu/effects/GrTextu...
File src/gpu/effects/GrTextureDomain.h (right):

https://codereview.chromium.org/422123003/diff/140001/src/gpu/effects/GrTextu...
src/gpu/effects/GrTextureDomain.h:29: kRepeat_Mode,  // Wrap texture
coordinates.  NOTE: filtering may not work as expected
This comment is a little cryptic. You mean due to lack of NPOT support? If so,
maybe we should call that out explicitly.

[Stephen White, 2014-07-29 14:52:14]

+junov, just in case he wants to take a look.

[bsalomon, 2014-07-29 14:55:27]

https://codereview.chromium.org/422123003/diff/140001/src/gpu/effects/GrTextu...
File src/gpu/effects/GrTextureDomain.h (right):

https://codereview.chromium.org/422123003/diff/140001/src/gpu/effects/GrTextu...
src/gpu/effects/GrTextureDomain.h:29: kRepeat_Mode,  // Wrap texture
coordinates.  NOTE: filtering may not work as expected
On 2014/07/29 14:51:16, Stephen White wrote:
> This comment is a little cryptic. You mean due to lack of NPOT support? If so,
> maybe we should call that out explicitly.

It's not related to NPOT. Using MIP or Bilerp filtering will read texels that
surround the domain. The only way I know of around that would be to use nearest
filtering and perform the filtering in the shader using multiple texture reads.

[Justin Novosad, 2014-07-29 15:08:59]

https://codereview.chromium.org/422123003/diff/140001/src/gpu/effects/GrTextu...
File src/gpu/effects/GrTextureDomain.cpp (right):

https://codereview.chromium.org/422123003/diff/140001/src/gpu/effects/GrTextu...
src/gpu/effects/GrTextureDomain.cpp:250: filterMode ==
GrTextureParams::kNone_FilterMode);
I suppose this is because the shader does not lerp over wrap boundaries? A way
to solve this is to copy the tile into a new texture that has border pixels that
reflect the wrapping.  That strategy would need to be managed at a higher level,
but should not be blocked here.

[joshua.litt, 2014-07-29 15:17:20]

https://codereview.chromium.org/422123003/diff/140001/src/gpu/effects/GrTextu...
File src/gpu/effects/GrTextureDomain.cpp (right):

https://codereview.chromium.org/422123003/diff/140001/src/gpu/effects/GrTextu...
src/gpu/effects/GrTextureDomain.cpp:250: filterMode ==
GrTextureParams::kNone_FilterMode);
Exactly.  We could either copy, or do texture filtering manually in the shader. 
Its something we can look at if someone actually wants to use filtering with
repeat texture domains.

On 2014/07/29 15:08:59, junov wrote:
> I suppose this is because the shader does not lerp over wrap boundaries? A way
> to solve this is to copy the tile into a new texture that has border pixels
that
> reflect the wrapping.  That strategy would need to be managed at a higher
level,
> but should not be blocked here.

[Justin Novosad, 2014-07-29 19:12:26]

On 2014/07/29 15:17:20, joshua.litt wrote:
>
https://codereview.chromium.org/422123003/diff/140001/src/gpu/effects/GrTextu...
> File src/gpu/effects/GrTextureDomain.cpp (right):
> 
>
https://codereview.chromium.org/422123003/diff/140001/src/gpu/effects/GrTextu...
> src/gpu/effects/GrTextureDomain.cpp:250: filterMode ==
> GrTextureParams::kNone_FilterMode);
> Exactly.  We could either copy, or do texture filtering manually in the
shader. 
> Its something we can look at if someone actually wants to use filtering with
> repeat texture domains.
> 
> On 2014/07/29 15:08:59, junov wrote:
> > I suppose this is because the shader does not lerp over wrap boundaries? A
way
> > to solve this is to copy the tile into a new texture that has border pixels
> that
> > reflect the wrapping.  That strategy would need to be managed at a higher
> level,
> > but should not be blocked here.

OK, that can be a future improvement though. lgtm for this.

[joshua.litt, 2014-07-29 19:46:54]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-07-29 19:47:48]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/joshualitt@chromium.org/422123003/160001

[commit-bot: I haz the power, 2014-07-29 19:59:33]

Change committed as 5ae5fc59b27a48711e514b3ede548b228e393e9b