Texture Draw Calls Coalescing

Texture Draw Calls Coalescing

This patch batches multiple calls to DrawQuad inside GlRenderer when the calls draw textured quads that have the same texture.  These quads differ only in transform for both position and UV.  This patch extends the vertex shader used for drawing textures to use one of 8 matrices and UV transforms.  By batching up to 8 DrawQuads into a single draw quad, we reduce the number of times many different OpenGL ES state calls are made.

The implementation maintains a cache of up to 8 quads that contain the same texture and drawing parameters (e.g. opacity).  If a 9th textured quad with the same parameters or a different kind of quad is drawn or any GL state (e.g. scissor rect) is changed or the new finishDrawingQuadList() function is called then the the quads are drawn in a single call and the cache is flushed.

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

[whunt, 2012-11-27 23:43:28]

First post of textured draw call coalescing.

[danakj, 2012-11-27 23:47:47]

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

https://codereview.chromium.org/11415161/diff/1/cc/gl_renderer.cc#newcode71
cc/gl_renderer.cc:71: struct Matrix {
Is there some reason to not just use gfx::Transform?

[jamesr, 2012-11-28 00:00:37]

As you suspect I think it'd be worth moving some of these classes into separate
files or using functionality that already exists.  gl_renderer.cc is already a
bit over 1600 lines long which is pretty unwieldy.

https://codereview.chromium.org/11415161/diff/1/cc/geometry_binding.cc
File cc/geometry_binding.cc (right):

https://codereview.chromium.org/11415161/diff/1/cc/geometry_binding.cc#newcode32
cc/geometry_binding.cc:32: -0.5f,  0.5f, 0.0f, 0.0f, 1.0f, 1.0f,
Can we generate this data with a loop instead of lots of copy-pasted lines? 
That's a bit easier to verify and will be easier to extend

https://codereview.chromium.org/11415161/diff/1/cc/geometry_binding.cc#newcode66
cc/geometry_binding.cc:66: 4,  5,  6,
Same here - each 6-tuple is just the previous one incremented by 4, so should be
easy to programatically generate

https://codereview.chromium.org/11415161/diff/1/cc/geometry_binding.h
File cc/geometry_binding.h (right):

https://codereview.chromium.org/11415161/diff/1/cc/geometry_binding.h#newcode5
cc/geometry_binding.h:5: #ifndef GeometryBinding_h
I think this change is unintentional - we want CC_GEOMETRY_BINDING_H_ here since
that matches chromium style

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

https://codereview.chromium.org/11415161/diff/1/cc/gl_renderer.cc#newcode63
cc/gl_renderer.cc:63: struct Vector {
cc/math_util.h has a HomogeneousCoordinate that's a 4-vector.  Shawn - do you
think we should extend this?

If we don't use the w component we could just use gfx::Vector3dF which defines
addition and scalar multiplication already

https://codereview.chromium.org/11415161/diff/1/cc/gl_renderer.cc#newcode89
cc/gl_renderer.cc:89: struct TexturedQuadDrawCache
Could we put this in its own file?  Adding a new file is fairly simple - just
add the file and add it to the appropriate part of cc/cc.gyp - you don't have to
muck about with individual build systems or anything like that

https://codereview.chromium.org/11415161/diff/1/cc/gl_renderer.cc#newcode109
cc/gl_renderer.cc:109: , resourceID(0)
could you initialize all scalars and move the constructor to the top of the
struct declaration?

https://codereview.chromium.org/11415161/diff/1/cc/gl_renderer.cc#newcode134
cc/gl_renderer.cc:134: , m_drawCache(new TexturedQuadDrawCache())
if we had TextureQuadDrawCache in its own header, we could just have a member of
type TexturedQuadDrawCache instead of a pointer and cut down on a bit of
boilerplate and a heap allocation

[whunt, 2012-11-28 00:29:07]

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

https://codereview.chromium.org/11415161/diff/1/cc/gl_renderer.cc#newcode63
cc/gl_renderer.cc:63: struct Vector {
In truth we no longer need any math because I'm not skinning on the host any
more because VBO upload is slow, I just need a container that stores 4 floats
(which gfx::Vector3dF is not).  When we were skinning on the host we did need
the w component.

On 2012/11/28 00:00:37, jamesr wrote:
> cc/math_util.h has a HomogeneousCoordinate that's a 4-vector.  Shawn - do you
> think we should extend this?
> 
> If we don't use the w component we could just use gfx::Vector3dF which defines
> addition and scalar multiplication already

https://codereview.chromium.org/11415161/diff/1/cc/gl_renderer.cc#newcode71
cc/gl_renderer.cc:71: struct Matrix {
This only works if gfx::Transform is of float type (it is packed and organized
in the correct format).  It cannot be guaranteed that gfx::Transform is not in
double format and thus will not upload. 

On 2012/11/27 23:47:47, danakj wrote:
> Is there some reason to not just use gfx::Transform?

https://codereview.chromium.org/11415161/diff/1/cc/gl_renderer.cc#newcode89
cc/gl_renderer.cc:89: struct TexturedQuadDrawCache
Sure, no problem.

On 2012/11/28 00:00:37, jamesr wrote:
> Could we put this in its own file?  Adding a new file is fairly simple - just
> add the file and add it to the appropriate part of cc/cc.gyp - you don't have
to
> muck about with individual build systems or anything like that

https://codereview.chromium.org/11415161/diff/1/cc/gl_renderer.cc#newcode109
cc/gl_renderer.cc:109: , resourceID(0)
Constructor moved.

About initialization.  I *can* initialize these values.  However the intended
use of this struct is to assume that if programID is 0 then all fields are
undefined.  The "appropriate" way to implement this would be to have a get
method for each field that throws an exception if programID is 0 and have a
setter that sets everything at the same time.

As it stands nothing is ever read to or written from unless programID is
non-zero.  I can initialize them as a matter of course but it's not really the
intended behavior.

On 2012/11/28 00:00:37, jamesr wrote:
> could you initialize all scalars and move the constructor to the top of the
> struct declaration?

https://codereview.chromium.org/11415161/diff/1/cc/gl_renderer.cc#newcode134
cc/gl_renderer.cc:134: , m_drawCache(new TexturedQuadDrawCache())
This is true, but that would make everything that depended on gl_renderer.h
*also* depend on the new file (we would have to include the new file in
gl_renderer.h rather than in gl_renderer.cc.  This both increases build times
and breaks encapsulation.

I can see it both ways.

On 2012/11/28 00:00:37, jamesr wrote:
> if we had TextureQuadDrawCache in its own header, we could just have a member
of
> type TexturedQuadDrawCache instead of a pointer and cut down on a bit of
> boilerplate and a heap allocation

[danakj, 2012-11-28 00:43:58]

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

https://codereview.chromium.org/11415161/diff/1/cc/gl_renderer.cc#newcode71
cc/gl_renderer.cc:71: struct Matrix {
On 2012/11/28 00:29:08, whunt wrote:
> This only works if gfx::Transform is of float type (it is packed and organized
> in the correct format).  It cannot be guaranteed that gfx::Transform is not in
> double format and thus will not upload. 

Oh okay, thanks for the explanation. Let's call it FloatMatrix..or..something
that makes its purpose/difference clear.

[shawnsingh, 2012-11-28 19:44:16]

This is pretty awesome warren =)
Here are comments I have so far....  I might have some more comments later.

https://codereview.chromium.org/11415161/diff/1/cc/direct_renderer.cc
File cc/direct_renderer.cc (right):

https://codereview.chromium.org/11415161/diff/1/cc/direct_renderer.cc#newcode210
cc/direct_renderer.cc:210: void DirectRenderer::finishDrawingQuadList()
I know I was the one who suggested doing it this way, but now I changed my
mind...  since the rest of the drawing-related API is only pure virtual, perhaps
we should also make this pure virtual to be consistent.

https://codereview.chromium.org/11415161/diff/1/cc/geometry_binding.cc
File cc/geometry_binding.cc (right):

https://codereview.chromium.org/11415161/diff/1/cc/geometry_binding.cc#newcode26
cc/geometry_binding.cc:26: // Vertex positions, texture coordinates and skinning
values for the 4 corners of 8 1x1 quads.
I think the word skinning might confuse/complicate things unnecessarily.  Would
it be just as correct to say "triangle index" here, like you did in other
places?

https://codereview.chromium.org/11415161/diff/1/cc/geometry_binding.cc#newcode27
cc/geometry_binding.cc:27: float vertexListData[] = {
If possible, we should make this an array of structs, where the struct is:

struct vertexInfo {
  float vertex[3];
  float texCoords[2];
  int triangleIndex;
}

And then on the vertexAttribPointer for the index below, we should give GL_INT
(or whatever it is correctly) instead of GL_FLOAT.

https://codereview.chromium.org/11415161/diff/1/cc/geometry_binding.cc#newcod...
cc/geometry_binding.cc:105: GLC(m_context,
m_context->vertexAttribPointer(texCoordAttribLocation(), 2, GL_FLOAT, false, 6 *
sizeof(float), 12));
This "12" should probably remain 3 * sizeof(float)

https://codereview.chromium.org/11415161/diff/1/cc/geometry_binding.cc#newcod...
cc/geometry_binding.cc:106: GLC(m_context,
m_context->vertexAttribPointer(triangleIndexAttribLocation(), 1, GL_FLOAT,
false, 6 * sizeof(float), 20));
And this "20" should probably remain 5 * sizeof(float)

if we can use array-of-structs, then this should also be GL_INT instead of
GL_FLOAT

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

https://codereview.chromium.org/11415161/diff/1/cc/gl_renderer.cc#newcode63
cc/gl_renderer.cc:63: struct Vector {
I think we should do the following:
(1) merge this Vector class with HomogeneousCoordinate.

(2) move all this math to MathUtil.*

(3) add one trivial unit test to make sure that the Matrix * Vector multiply
doesn't accidentally regress / transpose if someone else modifies it

https://codereview.chromium.org/11415161/diff/1/cc/gl_renderer.cc#newcode1085
cc/gl_renderer.cc:1085: void GLRenderer::drawTextureQuadPush(const DrawingFrame&
frame, const TextureDrawQuad* quad)
The name at a glance doesn't make me think about the implications...  could we
rename it to something like  "enqueueTextureQuad"

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

https://codereview.chromium.org/11415161/diff/1/cc/shader.cc#newcode162
cc/shader.cc:162: attribute float a_index;
If my prior comment about using the array of structs would work, then we could
make this an int.

[whunt, 2012-11-28 22:01:15]

https://codereview.chromium.org/11415161/diff/1/cc/direct_renderer.cc
File cc/direct_renderer.cc (right):

https://codereview.chromium.org/11415161/diff/1/cc/direct_renderer.cc#newcode210
cc/direct_renderer.cc:210: void DirectRenderer::finishDrawingQuadList()
DirectRenderer isn't a virtual interface, it's already got tons of
functionality.  I don't see any real logic as to which parts are pure virtual
and which aren't.  This function can certainly be made pure virtual but I don't
see a strong reason to change it.  Is there some convention?

On 2012/11/28 19:44:16, shawnsingh wrote:
> I know I was the one who suggested doing it this way, but now I changed my
> mind...  since the rest of the drawing-related API is only pure virtual,
perhaps
> we should also make this pure virtual to be consistent.

https://codereview.chromium.org/11415161/diff/1/cc/geometry_binding.cc
File cc/geometry_binding.cc (right):

https://codereview.chromium.org/11415161/diff/1/cc/geometry_binding.cc#newcode26
cc/geometry_binding.cc:26: // Vertex positions, texture coordinates and skinning
values for the 4 corners of 8 1x1 quads.
Done, I made the language more uniform and avoided the term skinning.

On 2012/11/28 19:44:16, shawnsingh wrote:
> I think the word skinning might confuse/complicate things unnecessarily. 
Would
> it be just as correct to say "triangle index" here, like you did in other
> places?

https://codereview.chromium.org/11415161/diff/1/cc/geometry_binding.cc#newcode27
cc/geometry_binding.cc:27: float vertexListData[] = {
Done.

On 2012/11/28 19:44:16, shawnsingh wrote:
> If possible, we should make this an array of structs, where the struct is:
> 
> struct vertexInfo {
>   float vertex[3];
>   float texCoords[2];
>   int triangleIndex;
> }
> 
> And then on the vertexAttribPointer for the index below, we should give GL_INT
> (or whatever it is correctly) instead of GL_FLOAT.

https://codereview.chromium.org/11415161/diff/1/cc/geometry_binding.cc#newcode32
cc/geometry_binding.cc:32: -0.5f,  0.5f, 0.0f, 0.0f, 1.0f, 1.0f,
Sure, Done.

On 2012/11/28 00:00:37, jamesr wrote:
> Can we generate this data with a loop instead of lots of copy-pasted lines? 
> That's a bit easier to verify and will be easier to extend

https://codereview.chromium.org/11415161/diff/1/cc/geometry_binding.cc#newcode66
cc/geometry_binding.cc:66: 4,  5,  6,
Yes, but this one is slightly more subtle.  You probably didn't notice that the
first set of quads is slightly different than the rest.  All sets can be made
the same.

The reason for the difference is that the first pair of quads (0, 1, 2) (3, 0,
2) also doubles as the first quad of lines when drawing a line loop (0, 1, 2,
3).  That can be maintained during automatic generation.

On 2012/11/28 00:00:37, jamesr wrote:
> Same here - each 6-tuple is just the previous one incremented by 4, so should
be
> easy to programatically generate

https://codereview.chromium.org/11415161/diff/1/cc/geometry_binding.h
File cc/geometry_binding.h (right):

https://codereview.chromium.org/11415161/diff/1/cc/geometry_binding.h#newcode5
cc/geometry_binding.h:5: #ifndef GeometryBinding_h
I'll change that.  I hadn't touched that line before to my knowledge, perhaps it
was part of one of the merges.

On 2012/11/28 00:00:37, jamesr wrote:
> I think this change is unintentional - we want CC_GEOMETRY_BINDING_H_ here
since
> that matches chromium style

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

https://codereview.chromium.org/11415161/diff/1/cc/gl_renderer.cc#newcode1085
cc/gl_renderer.cc:1085: void GLRenderer::drawTextureQuadPush(const DrawingFrame&
frame, const TextureDrawQuad* quad)
I like that suggestion. Done.

On 2012/11/28 19:44:16, shawnsingh wrote:
> The name at a glance doesn't make me think about the implications...  could we
> rename it to something like  "enqueueTextureQuad"

[danakj, 2012-11-28 22:03:15]

https://codereview.chromium.org/11415161/diff/1/cc/direct_renderer.cc
File cc/direct_renderer.cc (right):

https://codereview.chromium.org/11415161/diff/1/cc/direct_renderer.cc#newcode210
cc/direct_renderer.cc:210: void DirectRenderer::finishDrawingQuadList()
On 2012/11/28 22:01:15, whunt wrote:
> DirectRenderer isn't a virtual interface, it's already got tons of
> functionality.  I don't see any real logic as to which parts are pure virtual
> and which aren't.  This function can certainly be made pure virtual but I
don't
> see a strong reason to change it.  Is there some convention?

Things that would be useful for GLRenderer and SoftwareRenderer live in
DirectRenderer. Things that are GL-specific live in the subclass.

Generally the DirectRenderer has the logic to draw a frame, and abstract methods
filled in by GLRenderer or SoftwareRenderer to make it happen.

[whunt, 2012-11-28 23:18:00]

So this particular method isn't generally useful for any renderers that don't
perform any clean up after drawing quads.  If we'd like to change the interface
to always provide that hook we can make it purely virtual.  The method had to be
added to DirectRenderer rather than GLRenderer because DirectRenderer defines
the render loop and thus needs to call the cleanup code.

I'm happy to make it pure virtual but I'll mean going to all the other renderers
that inherit from DirectRenderer and adding a call that does nothing.  If left
to me, I'd leave it as is but I'm happy to defer to and owner with a stronger
opinion.

On 2012/11/28 22:03:15, danakj wrote:
> https://codereview.chromium.org/11415161/diff/1/cc/direct_renderer.cc
> File cc/direct_renderer.cc (right):
> 
>
https://codereview.chromium.org/11415161/diff/1/cc/direct_renderer.cc#newcode210
> cc/direct_renderer.cc:210: void DirectRenderer::finishDrawingQuadList()
> On 2012/11/28 22:01:15, whunt wrote:
> > DirectRenderer isn't a virtual interface, it's already got tons of
> > functionality.  I don't see any real logic as to which parts are pure
virtual
> > and which aren't.  This function can certainly be made pure virtual but I
> don't
> > see a strong reason to change it.  Is there some convention?
> 
> Things that would be useful for GLRenderer and SoftwareRenderer live in
> DirectRenderer. Things that are GL-specific live in the subclass.
> 
> Generally the DirectRenderer has the logic to draw a frame, and abstract
methods
> filled in by GLRenderer or SoftwareRenderer to make it happen.

[whunt, 2012-11-28 23:18:47]

Done.  It's called "float16" now and has no functionality other than getting
uploaded to GL.


On 2012/11/28 00:43:58, danakj wrote:
> https://codereview.chromium.org/11415161/diff/1/cc/gl_renderer.cc
> File cc/gl_renderer.cc (right):
> 
> https://codereview.chromium.org/11415161/diff/1/cc/gl_renderer.cc#newcode71
> cc/gl_renderer.cc:71: struct Matrix {
> On 2012/11/28 00:29:08, whunt wrote:
> > This only works if gfx::Transform is of float type (it is packed and
organized
> > in the correct format).  It cannot be guaranteed that gfx::Transform is not
in
> > double format and thus will not upload. 
> 
> Oh okay, thanks for the explanation. Let's call it FloatMatrix..or..something
> that makes its purpose/difference clear.

[jamesr, 2012-11-29 11:17:11]

This is looking really good.  I've left a flotilla of style / nitpick type
comments - they are a bit annoying, but it's really valuable on a codebase with
this many active contributors for everyone to stick to one uniform style and the
fastest way I know of to learn a style is to get all the feedback.  If you'd
like I can sit down with you and go through stuff in person - that'll be a lot
faster than doing nitpicks asynchronously.

It looks like there are a few layout test failures on the trybots - do you know
if those failures are actually due to this patch or flake and if they are due to
this patch do you know what the diffs look like?

https://codereview.chromium.org/11415161/diff/13001/cc/direct_renderer.cc
File cc/direct_renderer.cc (right):

https://codereview.chromium.org/11415161/diff/13001/cc/direct_renderer.cc#new...
cc/direct_renderer.cc:210: void DirectRenderer::finishDrawingQuadList()
I think having this as a no-op default implementation is fine - if a
DirectRenderer implementation isn't doing batching then it quite reasonably can
have nothing to do here (and that's exactly what the software one does).

https://codereview.chromium.org/11415161/diff/13001/cc/geometry_binding.cc
File cc/geometry_binding.cc (right):

https://codereview.chromium.org/11415161/diff/13001/cc/geometry_binding.cc#ne...
cc/geometry_binding.cc:33: DCHECK(sizeof(Quad) == 24 * sizeof(float));
use COMPILE_ASSERT from base/basictypes.h (which is already included here)

https://codereview.chromium.org/11415161/diff/13001/cc/geometry_binding.h
File cc/geometry_binding.h (right):

https://codereview.chromium.org/11415161/diff/13001/cc/geometry_binding.h#new...
cc/geometry_binding.h:5: #ifndef CC_GEOMETRY_BINDING_H
nit: still want the trailing underscore here

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer.cc#newcode246
cc/gl_renderer.cc:246: m_blendShadow = true;
nit: could you move this right next to the enable(GL_BLEND) call (or move that
call down a few lines) just to be extra clear about what state this is
shadowing?

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer.cc#newcode984
cc/gl_renderer.cc:984: // Check to see if we have anything to draw
nit: end comments with a period as if they were a complete sentence, even if
it's not

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer.cc#newcode995
cc/gl_renderer.cc:995: //GLC(context(), context()->activeTexture(GL_TEXTURE0));
I would prefer deleting this line to commenting it out.  It's fine to document
our assumptions in comments since we do enforce that the active texture unit is
0 between draw calls

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer.cc#newcode...
cc/gl_renderer.cc:1015: DCHECK(sizeof(float4) == 4 * sizeof(float));
You can do this with COMPILE_ASSERT() from base/basictypes.h (which is already
included in this file)

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer.cc#newcode...
cc/gl_renderer.cc:1046: if (m_drawCache.programID             !=
binding.programId              ||
I think this is a little more spaced out than we normally do - chromium style
would be to format this with one space around the "!=" operator and before the
||.  The "!="s won't line up, but it's still pretty readable

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer.cc#newcode...
cc/gl_renderer.cc:1203: if (enabled != m_blendShadow)
we generally prefer to early return so the indentation level isn't quite as
deep.  so this would be:

if (enabled == m_blendShadow)
   return;

... rest of function

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer.cc#newcode...
cc/gl_renderer.cc:1215: if (program != m_programShadow)
same here

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer.h
File cc/gl_renderer.h (right):

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer.h#newcode12
cc/gl_renderer.h:12: #include "cc/gl_renderer_drawcache.h"
naming nit: i think this should be "gl_renderer_draw_cache.h", since "drawcache"
isn't a single word

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer_drawcache.cc
File cc/gl_renderer_drawcache.cc (right):

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer_drawcache....
cc/gl_renderer_drawcache.cc:1: // Copyright 2010 The Chromium Authors. All
rights reserved.
2012

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer_drawcache.h
File cc/gl_renderer_drawcache.h (right):

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer_drawcache....
cc/gl_renderer_drawcache.h:1: // Copyright 2010 The Chromium Authors. All rights
reserved.
it's 2012

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer_drawcache....
cc/gl_renderer_drawcache.h:11: // collects 4 floats at a time for easy upload to
GL
Start comments with a uppercase letter, end with a period

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer_drawcache....
cc/gl_renderer_drawcache.h:12: struct float4 { float data[4]; };
chromium style for naming would have this be Float4

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer_drawcache....
cc/gl_renderer_drawcache.h:15: struct float16 { float data[16]; };
similarly Float16

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer_drawcache....
cc/gl_renderer_drawcache.h:21: {
the cc/ directory is currently a mix of WebKit style and chromium style, but for
new classes we're trying to stick to chromium style (since we'll convert
everything to that eventually anyway).  that means putting this curly brace on
the previous line and using chromium indentation (2-space)

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer_drawcache....
cc/gl_renderer_drawcache.h:23: virtual ~TexturedQuadDrawCache();
do you need a virtual destructor? there aren't any methods on this

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer_drawcache....
cc/gl_renderer_drawcache.h:26: int programID;
chromium style for these is to use hacker_style instead of camelCaseStyle for
members

[whunt, 2012-11-29 23:55:06]

https://codereview.chromium.org/11415161/diff/13001/cc/geometry_binding.cc
File cc/geometry_binding.cc (right):

https://codereview.chromium.org/11415161/diff/13001/cc/geometry_binding.cc#ne...
cc/geometry_binding.cc:33: DCHECK(sizeof(Quad) == 24 * sizeof(float));
thanks, was looking for that functionality.  They added static_assert in c++11
but it's not legal here yet.

On 2012/11/29 11:17:11, jamesr wrote:
> use COMPILE_ASSERT from base/basictypes.h (which is already included here)

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer.cc#newcode246
cc/gl_renderer.cc:246: m_blendShadow = true;
Done.

On 2012/11/29 11:17:11, jamesr wrote:
> nit: could you move this right next to the enable(GL_BLEND) call (or move that
> call down a few lines) just to be extra clear about what state this is
> shadowing?

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer.cc#newcode984
cc/gl_renderer.cc:984: // Check to see if we have anything to draw
Done, but Why?? They're not complete sentences...

On 2012/11/29 11:17:11, jamesr wrote:
> nit: end comments with a period as if they were a complete sentence, even if
> it's not

https://codereview.chromium.org/11415161/diff/13001/cc/gl_renderer.cc#newcode...
cc/gl_renderer.cc:1046: if (m_drawCache.programID             !=
binding.programId              ||
More importantly, it wouldn't be block-editable for those of us who edit
multiple lines at a time.  I can collapse it.
On 2012/11/29 11:17:11, jamesr wrote:
> I think this is a little more spaced out than we normally do - chromium style
> would be to format this with one space around the "!=" operator and before the
> ||.  The "!="s won't line up, but it's still pretty readable

[jamesr, 2012-11-30 00:31:21]

https://codereview.chromium.org/11415161/diff/5004/cc/geometry_binding.cc
File cc/geometry_binding.cc (right):

https://codereview.chromium.org/11415161/diff/5004/cc/geometry_binding.cc#new...
cc/geometry_binding.cc:39: {
{ belongs on previous line

https://codereview.chromium.org/11415161/diff/5004/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/11415161/diff/5004/cc/gl_renderer.cc#newcode260
cc/gl_renderer.cc:260: {
curly brace goes on previous line

WebKit's a bit nutty - opening curly braces are on their own line pretty much
only for functions and nowhere else.
http://www.webkit.org/coding/coding-style.html#braces-function if you're curious

https://codereview.chromium.org/11415161/diff/5004/cc/gl_renderer.cc#newcode1014
cc/gl_renderer.cc:1014: COMPILE_ASSERT(sizeof(float4) == 4 * sizeof(float));
I think this has to be Float4, right?

https://codereview.chromium.org/11415161/diff/5004/cc/gl_renderer.cc#newcode1015
cc/gl_renderer.cc:1015: COMPILE_ASSERT(sizeof(float16) == 16 * sizeof(float));
float16 -> Float16

https://codereview.chromium.org/11415161/diff/5004/cc/gl_renderer.cc#newcode1051
cc/gl_renderer.cc:1051: {
curly brace should go on previous line

https://codereview.chromium.org/11415161/diff/5004/cc/gl_renderer.cc#newcode1066
cc/gl_renderer.cc:1066: float4 uv = {uvRect.x(), uvRect.y(), uvRect.width(),
uvRect.height()};
float4 -> Float4

https://codereview.chromium.org/11415161/diff/5004/cc/gl_renderer.cc#newcode1074
cc/gl_renderer.cc:1074: float16 m;
float16 -> Float16

[jamesr, 2012-11-30 02:01:42]

lgtm

[commit-bot: I haz the power, 2012-11-30 02:01:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/whunt@chromium.org/11415161/10003

[commit-bot: I haz the power, 2012-11-30 05:42:04]

Change committed as 170409