cc: Allow TEXTURE_RECTANGLE_ARB to be used for tile textures.

cc: Allow TEXTURE_RECTANGLE_ARB to be used for tile textures.

Refactor shader framework to support different texture targets and
use TEXTURE_RECTANGLE_ARB by default when initializing tile textures
using ImageRasterWorkerPool.

BUG=322571
TEST=cc_unittests --gtest_filter=GLRendererShaderPixelTest.AllShadersCompile

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236981

[reveman, 2013-11-22 18:27:16]

This makes our shader framework more flexible in terms of texture targets. The
result is fewer fragment shader implementations and the ability to use
TEXTURE_RECTANGLE_ARB with tile textures.

This patch is also making the zero-copy path use TEXTURE_RECTANGLE_ARB by
default as we'll need that to use IOSurfaces on MacOSX. We can always make that
optional but I figured we could try to use this target everywhere first and only
make it optional if we have to.

[epennerAtGoogle, 2013-11-22 19:27:35]

Some high level questions and comments:

- Can you describe why rect textures are needed on OSX? Is this for unnormalized
coordinates or is it something to do with NPOT textures? Or is it just that the
API forces this on us?

- I like the EXTERNAL_OES support, but are we sure these textures will never
need to be passed to other systems that might expect texture_2d?

- Does Gralloc support rect samplers? IIRC it only supports EXTERNAL_OES and
TEXTURE_2D, and then only EXTERNAL_OES for SurfaceTextures. So I don't think we
can use rect textures for all zero-copy cases?

- I like the refactoring to use arrays, but the extra array dimension for
texture type is definitely pushing us into 'permutation explosion' territory. My
thinking on this was that, once we reach a certain threshold of permutations, it
might be better to refactor this into an uber-shader approach (effectively all
one big shader with macros to enable bits of code like masking etc.). Those can
more easily collapse permutations into slightly less efficient shaders, if need
be... Do you know how many total shaders this generates at the moment?

[epennerAtGoogle, 2013-11-22 19:27:55]

https://codereview.chromium.org/83883002/diff/1/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/83883002/diff/1/cc/output/gl_renderer.h#newco...
cc/output/gl_renderer.h:361: TileProgram
tile_program_[NumTexCoordPrecisions][NumSamplerTypes];
Hmm, could the program also become an enum? Then it would just be one big array.

Optional nit: I like to use a function to access such arrays, as it enforces
type safety on the enum. However probably only worth it if this becomes one
really big array.

[epennerAtGoogle, 2013-11-22 19:31:17]

https://codereview.chromium.org/83883002/diff/1/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/83883002/diff/1/cc/output/gl_renderer.h#newco...
cc/output/gl_renderer.h:361: TileProgram
tile_program_[NumTexCoordPrecisions][NumSamplerTypes];
> Optional nit: I like to use a function to access such arrays, as it enforces
> type safety on the enum. However probably only worth it if this becomes one
> really big array.

Never mind regarding the functions. I see you already did this above!

[reveman, 2013-11-22 19:52:11]

On 2013/11/22 19:27:35, epennerAtGoogle wrote:
> Some high level questions and comments:
> 
> - Can you describe why rect textures are needed on OSX? Is this for
unnormalized
> coordinates or is it something to do with NPOT textures? Or is it just that
the
> API forces this on us?
> 
> - I like the EXTERNAL_OES support, but are we sure these textures will never
> need to be passed to other systems that might expect texture_2d?

It can only be used for code paths where TEXTURE_2D is not expected. This is the
case for zero-copy but not for async uploads which assume TEXTURE_2D.

> 
> - Does Gralloc support rect samplers? IIRC it only supports EXTERNAL_OES and
> TEXTURE_2D, and then only EXTERNAL_OES for SurfaceTextures. So I don't think
we
> can use rect textures for all zero-copy cases?

Ok, in that case we have to make this optional before landing. Maybe we can just
use EXTERNAL_OES if supported and otherwise TEXTURE_RECTANGLE_ARB. We can always
add a best_texture_from_image_target capability.

> 
> - I like the refactoring to use arrays, but the extra array dimension for
> texture type is definitely pushing us into 'permutation explosion' territory.
My
> thinking on this was that, once we reach a certain threshold of permutations,
it
> might be better to refactor this into an uber-shader approach (effectively all
> one big shader with macros to enable bits of code like masking etc.). Those
can
> more easily collapse permutations into slightly less efficient shaders, if
need
> be... Do you know how many total shaders this generates at the moment?

This change results in less shader code to maintain, much like a uber-shader
approach would.
The number of possible shaders that we can generate increase by 12 but I don't
see a problem with that as they are only created when needed.

[piman, 2013-11-22 20:48:13]

LGTM modulo figuring out the correct target everywhere. Also double-checking if
there's perf concerns in the choice (probably not, but worth checking).

https://codereview.chromium.org/83883002/diff/1/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/83883002/diff/1/cc/output/shader.cc#newcode59
cc/output/shader.cc:59: default:
nit: can we make this explicit on NumTexCoordPrecision so that we still get the
benefits from compile-time warnings?

https://codereview.chromium.org/83883002/diff/1/cc/output/shader.cc#newcode128
cc/output/shader.cc:128: default:
nit: same here (NumSamplerType)

[reveman, 2013-11-22 20:55:24]

On 2013/11/22 19:27:35, epennerAtGoogle wrote:
> Some high level questions and comments:
> 
> - Can you describe why rect textures are needed on OSX? Is this for
unnormalized
> coordinates or is it something to do with NPOT textures? Or is it just that
the
> API forces this on us?
> 

API, IOSurfaces can only be bound to TEXTURE_RECTANGLE_ARB target.

[epennerAtGoogle, 2013-11-22 22:36:59]

> API, IOSurfaces can only be bound to TEXTURE_RECTANGLE_ARB target.

Grr, these things are frustrating ;)

Okay sounds like shader perms aren't too bad. And indeed it's more like an
uber-shader. Since this is ready to go, no problem with having the extra
flexibility. Took a more thorough look and lgtm.

Bigger picture though, I'm still a bit worried about limiting certain textures
to have special treatment because of a target limitation in one platform
(Android ST would the same thing). For example, this caused difficulty for
Android video textures when used in other places (they needed to copy it etc.).
I guess I'm just wondering if we want to go down this path instead of just GPU
copying when there is a target limitation. It's looking like we may need to do
that on Android anyway.

Btw this is primarily for tile textures right? Or is there other IOSurface
sources?

[kaanb, 2013-11-23 02:02:20]

lgtm with some questions and suggestions, I'm out next week so you can go ahead
with CQ if the other folks are OK

https://codereview.chromium.org/83883002/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/83883002/diff/1/cc/output/gl_renderer.cc#newc...
cc/output/gl_renderer.cc:1384: fragment_tex_translate_x /= texture_size.width();
consider checking if width() and/or height() could be zero

https://codereview.chromium.org/83883002/diff/1/cc/output/gl_renderer.cc#newc...
cc/output/gl_renderer.cc:1395: &uniforms);
fits previous line? also check other sites below

https://codereview.chromium.org/83883002/diff/1/cc/output/gl_renderer.cc#newc...
cc/output/gl_renderer.cc:2837: TexCoordPrecision precision) {
I'm somewhat new to this file but wouldn't it help to check if precision is
within the bounds of the array?

https://codereview.chromium.org/83883002/diff/1/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/83883002/diff/1/cc/output/shader.cc#newcode113
cc/output/shader.cc:113: "#define SamplerType sampler2DRect\n"
nit: It looks a bit weird to not have the '+' operator here and have it in the
following line. Why not use it after every line?

https://codereview.chromium.org/83883002/diff/1/cc/resources/image_raster_wor...
File cc/resources/image_raster_worker_pool.cc (right):

https://codereview.chromium.org/83883002/diff/1/cc/resources/image_raster_wor...
cc/resources/image_raster_worker_pool.cc:161: return GL_TEXTURE_RECTANGLE_ARB;
I guess this may break webview if we land it in the current form. Do we need a
crbug to track this?

https://codereview.chromium.org/83883002/diff/1/cc/resources/resource_provider.h
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/83883002/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.h:236: GLenum unit_;
Does the order of the fields need to change?

[reveman, 2013-11-23 10:29:11]

https://codereview.chromium.org/83883002/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/83883002/diff/1/cc/output/gl_renderer.cc#newc...
cc/output/gl_renderer.cc:1384: fragment_tex_translate_x /= texture_size.width();
On 2013/11/23 02:02:20, kaanb wrote:
> consider checking if width() and/or height() could be zero

Done.

https://codereview.chromium.org/83883002/diff/1/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/83883002/diff/1/cc/output/shader.cc#newcode59
cc/output/shader.cc:59: default:
On 2013/11/22 20:48:14, piman wrote:
> nit: can we make this explicit on NumTexCoordPrecision so that we still get
the
> benefits from compile-time warnings?

it's a bit annoying to lose these warnings but having a "default" case with
NOTREACHED is currently more consistent with the rest of cc/. ie. RasterMode and
WhichTree. I can change it if you prefer but would prefer do that across the
board in a separate patch.

https://codereview.chromium.org/83883002/diff/1/cc/output/shader.cc#newcode113
cc/output/shader.cc:113: "#define SamplerType sampler2DRect\n"
On 2013/11/23 02:02:20, kaanb wrote:
> nit: It looks a bit weird to not have the '+' operator here and have it in the
> following line. Why not use it after every line?

Only need '+' for runtime concatenation of strings.

https://codereview.chromium.org/83883002/diff/1/cc/resources/image_raster_wor...
File cc/resources/image_raster_worker_pool.cc (right):

https://codereview.chromium.org/83883002/diff/1/cc/resources/image_raster_wor...
cc/resources/image_raster_worker_pool.cc:161: return GL_TEXTURE_RECTANGLE_ARB;
On 2013/11/23 02:02:20, kaanb wrote:
> I guess this may break webview if we land it in the current form. Do we need a
> crbug to track this?

Done.

https://codereview.chromium.org/83883002/diff/1/cc/resources/resource_provider.h
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/83883002/diff/1/cc/resources/resource_provide...
cc/resources/resource_provider.h:236: GLenum unit_;
On 2013/11/23 02:02:20, kaanb wrote:
> Does the order of the fields need to change?

Makes initialization a bit cleaner as I can use unit_ when initializing target_.

[commit-bot: I haz the power, 2013-11-23 10:31:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/83883002/200001

[commit-bot: I haz the power, 2013-11-23 21:19:14]

Change committed as 236981