Adding support for per vertex opacity on textured layer

Adding support for per vertex opacity on textured layer

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

[Jerome, 2012-12-14 18:16:45]

I added opacity per vertex for the texture layer.

Warren: Could you double check the batching code?

[whunt, 2012-12-14 21:13:45]

https://codereview.chromium.org/11570027/diff/2001/cc/geometry_binding.cc
File cc/geometry_binding.cc (right):

https://codereview.chromium.org/11570027/diff/2001/cc/geometry_binding.cc#new...
cc/geometry_binding.cc:38: for (int i = 0; i < 8; i++) {
Why are we adding 0.5f to these indices?  They're intended to be integers.  When
used in the shaders they're cast to integers anyway.

https://codereview.chromium.org/11570027/diff/2001/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/11570027/diff/2001/cc/gl_renderer.cc#newcode1078
cc/gl_renderer.cc:1078: void GLRenderer::drawTextureQuad(const DrawingFrame&
frame, const TextureDrawQuad* quad)
Note: This function is deprecated (it's never called) and will be deleted.  It
hasn't been deleted yet because it's presence makes it much easier to test if
batching is causing a problem, but will probably disappear if no bugs related to
batching show up in the near future.

https://codereview.chromium.org/11570027/diff/2001/cc/gl_renderer.cc#newcode1123
cc/gl_renderer.cc:1123: 
You're using uniform4f to bind to a float array (which *might* work but I
believe is technically incorrect)
this should be something like:

float opacity[4] = {quad->opacity(), quad->opacity(), quad->opacity(),
quad->opacity()};
GLC(context(), context()->uniform1fv(binding.vertexOpacityLocation, opacity);

https://codereview.chromium.org/11570027/diff/2001/cc/heads_up_display_layer_...
File cc/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/11570027/diff/2001/cc/heads_up_display_layer_...
cc/heads_up_display_layer_impl.cc:94: const float vertex_opacity[] = {1, 1, 1,
1};
Just a note about 'const'.
If you're worried you or someone else might change this value at some point,
const is a reasonable way to prevent that.  The compiler however ignores 'const'
with respect to any optimizations.  Basically the reason is that the compiler
can tell if you change it or not and const doesn't actually prevent you from
changing const values.  The result is that most of the time 'const' is extra
text that enforces a behavior that a reasonable programmer isn't going to break
without good reason anyway.

Just remember:
((float*)vertex_opacity)[1] = 0.5f; is totally legal C.

https://codereview.chromium.org/11570027/diff/2001/cc/shader.h
File cc/shader.h (right):

https://codereview.chromium.org/11570027/diff/2001/cc/shader.h#newcode66
cc/shader.h:66: VertexShaderPosTexTransform();
Inconsistent tabbing.

https://codereview.chromium.org/11570027/diff/2001/cc/shader.h#newcode167
cc/shader.h:167: class FragmentShaderRGBATexAlpha : public
FragmentTexAlphaBinding {
Is this class used anymore?

[jamesr, 2012-12-14 21:18:19]

The constants are a bit inconsistent in this patch.  For floats, "0.0f" / "1.0f"
are preferred over "0", "1".

https://codereview.chromium.org/11570027/diff/2001/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/11570027/diff/2001/cc/gl_renderer.cc#newcode1013
cc/gl_renderer.cc:1013: GLC(m_context,
m_context->uniform1fv((int)m_drawCache.vertex_opacity_location,
(int)m_drawCache.vertex_opacity_data.size(),
(float*)&m_drawCache.vertex_opacity_data.front()));
chromium style prefers C++ style casts (static_cast<> and family) to C-style
casts, although i'm noticing now that the previous lines are also doing C-style
casts :(. mind changing the new ones to all be static_cast<>() ?

https://codereview.chromium.org/11570027/diff/2001/cc/shader.cc
File cc/shader.cc (right):

https://codereview.chromium.org/11570027/diff/2001/cc/shader.cc#newcode158
cc/shader.cc:158: && m_texTransformLocation != -1
&&s on previous line

https://codereview.chromium.org/11570027/diff/2001/cc/texture_draw_quad.cc
File cc/texture_draw_quad.cc (right):

https://codereview.chromium.org/11570027/diff/2001/cc/texture_draw_quad.cc#ne...
cc/texture_draw_quad.cc:31: || vertex_opacity[1] != 1.0f
chromium style is to have the operator on the previous line and line up the !=s

https://codereview.chromium.org/11570027/diff/2001/cc/texture_draw_quad.cc#ne...
cc/texture_draw_quad.cc:116: || vertex_opacity[0] != vertex_opacity[2]
||s on previous line

[enne (OOO), 2012-12-14 21:21:05]

https://codereview.chromium.org/11570027/diff/2001/cc/heads_up_display_layer_...
File cc/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/11570027/diff/2001/cc/heads_up_display_layer_...
cc/heads_up_display_layer_impl.cc:94: const float vertex_opacity[] = {1, 1, 1,
1};
I find const to be good for communicating expectations for how something will be
used.  const on a member function says that this function is not intended to
modify an object.  Sure, somebody could const_cast it away just like you can use
pointers in your example, but it's really about communicating expectations, not
an absolute enforcement.

I think the same thing holds for locals.  It's just communication that you're
declaring something in a temporary variable.  You're saying that you're not
planning to modify it later, but this variable is just here because it's more
readable to separate it out from usage.

[whunt, 2012-12-14 21:30:44]

I agree that const is useful for communicating expectations and is appropriate
in many cases.  I think the usefulness of const is typically proportional to the
scope over which the variable/member is visible.  It's very important in
interfaces and objects.  Less so for locals but still useful in longer
functions.  If the longest def-use chain for a local variable is like 5 lines it
just says "if you need to change this value do it on the line it was defined
rather than the next one" which imposes a specific coding pattern that may or
may not be any more readable and provides little additional information. 
Anyway, this discussion is largely philosophical and can be done outside the
context of this review.  I'm not asking for the consts to be removed from the
code.
On 2012/12/14 21:21:05, enne wrote:
>
https://codereview.chromium.org/11570027/diff/2001/cc/heads_up_display_layer_...
> File cc/heads_up_display_layer_impl.cc (right):
> 
>
https://codereview.chromium.org/11570027/diff/2001/cc/heads_up_display_layer_...
> cc/heads_up_display_layer_impl.cc:94: const float vertex_opacity[] = {1, 1, 1,
> 1};
> I find const to be good for communicating expectations for how something will
be
> used.  const on a member function says that this function is not intended to
> modify an object.  Sure, somebody could const_cast it away just like you can
use
> pointers in your example, but it's really about communicating expectations,
not
> an absolute enforcement.
> 
> I think the same thing holds for locals.  It's just communication that you're
> declaring something in a temporary variable.  You're saying that you're not
> planning to modify it later, but this variable is just here because it's more
> readable to separate it out from usage.

[Jerome, 2012-12-14 21:48:47]

https://codereview.chromium.org/11570027/diff/2001/cc/geometry_binding.cc
File cc/geometry_binding.cc (right):

https://codereview.chromium.org/11570027/diff/2001/cc/geometry_binding.cc#new...
cc/geometry_binding.cc:38: for (int i = 0; i < 8; i++) {
I was worried that the precision of the math might impact the index / 4.0 adding
0.5 does not change the result but is more robust to precision issues.

https://codereview.chromium.org/11570027/diff/2001/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/11570027/diff/2001/cc/gl_renderer.cc#newcode1013
cc/gl_renderer.cc:1013: GLC(m_context,
m_context->uniform1fv((int)m_drawCache.vertex_opacity_location,
(int)m_drawCache.vertex_opacity_data.size(),
(float*)&m_drawCache.vertex_opacity_data.front()));
On 2012/12/14 21:18:20, jamesr wrote:
> chromium style prefers C++ style casts (static_cast<> and family) to C-style
> casts, although i'm noticing now that the previous lines are also doing
C-style
> casts :(. mind changing the new ones to all be static_cast<>() ?

Done.

https://codereview.chromium.org/11570027/diff/2001/cc/gl_renderer.cc#newcode1078
cc/gl_renderer.cc:1078: void GLRenderer::drawTextureQuad(const DrawingFrame&
frame, const TextureDrawQuad* quad)
Ok, I though there was some cases where you wanted to disable the batching. I'll
it there for that patch

https://codereview.chromium.org/11570027/diff/2001/cc/gl_renderer.cc#newcode1123
cc/gl_renderer.cc:1123: 
On 2012/12/14 21:13:45, whunt wrote:
> You're using uniform4f to bind to a float array (which *might* work but I
> believe is technically incorrect)
> this should be something like:
> 
> float opacity[4] = {quad->opacity(), quad->opacity(), quad->opacity(),
> quad->opacity()};
> GLC(context(), context()->uniform1fv(binding.vertexOpacityLocation, opacity);

Done.

https://codereview.chromium.org/11570027/diff/2001/cc/shader.cc
File cc/shader.cc (right):

https://codereview.chromium.org/11570027/diff/2001/cc/shader.cc#newcode158
cc/shader.cc:158: && m_texTransformLocation != -1
On 2012/12/14 21:18:20, jamesr wrote:
> &&s on previous line

Done.

https://codereview.chromium.org/11570027/diff/2001/cc/shader.h
File cc/shader.h (right):

https://codereview.chromium.org/11570027/diff/2001/cc/shader.h#newcode66
cc/shader.h:66: VertexShaderPosTexTransform();
On 2012/12/14 21:13:45, whunt wrote:
> Inconsistent tabbing.

Done.

https://codereview.chromium.org/11570027/diff/2001/cc/shader.h#newcode167
cc/shader.h:167: class FragmentShaderRGBATexAlpha : public
FragmentTexAlphaBinding {
Yes by: TileProgram and RenderPassProgram

https://codereview.chromium.org/11570027/diff/2001/cc/texture_draw_quad.cc
File cc/texture_draw_quad.cc (right):

https://codereview.chromium.org/11570027/diff/2001/cc/texture_draw_quad.cc#ne...
cc/texture_draw_quad.cc:31: || vertex_opacity[1] != 1.0f
On 2012/12/14 21:18:20, jamesr wrote:
> chromium style is to have the operator on the previous line and line up the
!=s

Done.

https://codereview.chromium.org/11570027/diff/2001/cc/texture_draw_quad.cc#ne...
cc/texture_draw_quad.cc:116: || vertex_opacity[0] != vertex_opacity[2]
On 2012/12/14 21:18:20, jamesr wrote:
> ||s on previous line

Done.

[jamesr1, 2012-12-14 21:49:57]

On Fri, Dec 14, 2012 at 1:48 PM, <jscholler@chromium.org> wrote:

>
> https://codereview.chromium.**org/11570027/diff/2001/cc/**
>
geometry_binding.cc<https://codereview.chromium.org/11570027/diff/2001/cc/geometry_binding.cc>
> File cc/geometry_binding.cc (right):
>
> https://codereview.chromium.**org/11570027/diff/2001/cc/**
>
geometry_binding.cc#newcode38<https://codereview.chromium.org/11570027/diff/2001/cc/geometry_binding.cc#newcode38>
> cc/geometry_binding.cc:38: for (int i = 0; i < 8; i++) {
> I was worried that the precision of the math might impact the index /
> 4.0 adding 0.5 does not change the result but is more robust to
> precision issues.
>
>
Floating point math for 1.0, 2.0, 3.0, 4.0 should be exact for any sort of
IEEE-like representation

[Jerome, 2012-12-14 22:26:46]

Nevermind, I changed the code to do the division in integer rather than float.
Hopefully it is going to be translated as a shift.

[whunt, 2012-12-14 22:38:43]

https://codereview.chromium.org/11570027/diff/2001/cc/geometry_binding.cc
File cc/geometry_binding.cc (right):

https://codereview.chromium.org/11570027/diff/2001/cc/geometry_binding.cc#new...
cc/geometry_binding.cc:38: for (int i = 0; i < 8; i++) {
Every floating point standard known represents whole small integers correctly. 
Floating point operations where the values and results are small integers (small
being representable in 24 bits for float, 11 for half (CPUs don't support half
so this isn't relevant here) and 53 for double) are guaranteed by IEEE to not
generate reduced precision results.  Floating point isn't *that* scary =).

please remove the 0.5s

On 2012/12/14 21:48:47, Jerome wrote:
> I was worried that the precision of the math might impact the index / 4.0
adding
> 0.5 does not change the result but is more robust to precision issues.

[jamesr, 2012-12-15 00:48:19]

style nit but, eh, whatever

lgtm

https://codereview.chromium.org/11570027/diff/9022/cc/texture_draw_quad.cc
File cc/texture_draw_quad.cc (right):

https://codereview.chromium.org/11570027/diff/9022/cc/texture_draw_quad.cc#ne...
cc/texture_draw_quad.cc:27: bool needs_blending = vertex_opacity[0] != 1.0f ||
vertex_opacity[1] != 1.0f
line's too long - need to stick to 80col

[commit-bot: I haz the power, 2012-12-15 00:53:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jscholler@chromium.org/11570027/9022

[commit-bot: I haz the power, 2012-12-15 00:54:03]

Failed to apply patch for cc/gl_renderer_draw_cache.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/gl_renderer_draw_cache.h
  Hunk #1 FAILED at 24.
  1 out of 1 hunk FAILED -- saving rejects to file
cc/gl_renderer_draw_cache.h.rej

Patch:       cc/gl_renderer_draw_cache.h
Index: cc/gl_renderer_draw_cache.h
diff --git a/cc/gl_renderer_draw_cache.h b/cc/gl_renderer_draw_cache.h
index
c0a2b6900d8b9b34846f3813baf020225002947a..161214783e79d340f4dfe6cf6ed247a4af4a5c5c
100644
--- a/cc/gl_renderer_draw_cache.h
+++ b/cc/gl_renderer_draw_cache.h
@@ -24,17 +24,17 @@ struct TexturedQuadDrawCache {
   // Values tracked to determine if textured quads may be coalesced.
   int program_id;
   int resource_id;
-  float alpha;
   bool use_premultiplied_alpha;
   bool needs_blending;
 
   // Information about the program binding that is required to draw.
-  int alpha_location;
   int uv_xform_location;
+  int vertex_opacity_location;
   int matrix_location;
 
   // A cache for the coalesced quad data.
   std::vector<Float4> uv_xform_data;
+  std::vector<float> vertex_opacity_data;
   std::vector<Float16> matrix_data;
 };

[whunt, 2012-12-15 00:55:00]

lgtm

https://codereview.chromium.org/11570027/diff/9022/cc/geometry_binding.cc
File cc/geometry_binding.cc (right):

https://codereview.chromium.org/11570027/diff/9022/cc/geometry_binding.cc#new...
cc/geometry_binding.cc:38: for (int i = 0; i < 8; i++) {
FYI: doing the math in integer land and using stati_cast<float>() would be more
efficient,  The change is unnecessary though because this is startup code and
the performance difference is small.

https://codereview.chromium.org/11570027/diff/9022/cc/shader.cc
File cc/shader.cc (right):

https://codereview.chromium.org/11570027/diff/9022/cc/shader.cc#newcode173
cc/shader.cc:173: gl_Position = matrix[int(a_index * 0.25)] * a_position;
Did you try (int(a_index) >> 2)
That should be faster if >> is defined in gl_es
(I wouldn't trust to compiler very much)

https://codereview.chromium.org/11570027/diff/9022/cc/texture_draw_quad.cc
File cc/texture_draw_quad.cc (right):

https://codereview.chromium.org/11570027/diff/9022/cc/texture_draw_quad.cc#ne...
cc/texture_draw_quad.cc:70: // Grab our scale and make sure it's positive.
We're going to get a collision at some point because I performed similar
updates. (added a static_cast<float> because these are double results and
abstracted the tests to a function in gfx::transform)  It should be easy to
resolve for whoever has to do it.

[jamesr, 2012-12-15 00:57:37]

On 2012/12/15 00:55:00, whunt wrote:
> https://codereview.chromium.org/11570027/diff/9022/cc/shader.cc#newcode173
> cc/shader.cc:173: gl_Position = matrix[int(a_index * 0.25)] * a_position;
> Did you try (int(a_index) >> 2)
> That should be faster if >> is defined in gl_es
> (I wouldn't trust to compiler very much)

We checked and opengles 2.0's shading language spec doesn't appear to promise
that pretty much any integer ops are available :(. since this is the vertex
shader and we don't push many vertices at all I think this should be OK.

[commit-bot: I haz the power, 2012-12-15 17:10:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jscholler@chromium.org/11570027/14001

[commit-bot: I haz the power, 2012-12-15 20:17:20]

Change committed as 173310