Use vertex array objects on core profiles.

Use vertex array objects on core profiles.
Committed: https://code.google.com/p/skia/source/detail?r=8024

[bsalomon, 2013-03-06 15:20:59]

Use vertex array objects on core profiles. Default to core profiles on Mac and
Windows if available in our tools.

[robertphillips, 2013-03-06 16:14:20]

First pass - I need to look through it again.

https://codereview.chromium.org/12533007/diff/13001/src/gpu/gl/GrGLVertexArray.h
File src/gpu/gl/GrGLVertexArray.h (right):

https://codereview.chromium.org/12533007/diff/13001/src/gpu/gl/GrGLVertexArra...
src/gpu/gl/GrGLVertexArray.h:96: 
// comment?

https://codereview.chromium.org/12533007/diff/13001/src/gpu/gl/GrGLVertexArra...
src/gpu/gl/GrGLVertexArray.h:96: 
// comment?

https://codereview.chromium.org/12533007/diff/13001/src/gpu/gl/GrGLVertexArra...
src/gpu/gl/GrGLVertexArray.h:134: GrGLuint                fIndexBufferID;
not a bool?

https://codereview.chromium.org/12533007/diff/13001/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

https://codereview.chromium.org/12533007/diff/13001/src/gpu/gl/GrGpuGL.cpp#ne...
src/gpu/gl/GrGpuGL.cpp:2333: }
Returning a non-const fDefaultVertexArrayAttribState here seems a bit worrying.

https://codereview.chromium.org/12533007/diff/13001/src/gpu/gl/GrGpuGL.h
File src/gpu/gl/GrGpuGL.h (right):

https://codereview.chromium.org/12533007/diff/13001/src/gpu/gl/GrGpuGL.h#newc...
src/gpu/gl/GrGpuGL.h:377: 
one -> using

[bsalomon, 2013-03-06 16:23:26]

https://codereview.chromium.org/12533007/diff/13001/src/gpu/gl/GrGLVertexArray.h
File src/gpu/gl/GrGLVertexArray.h (right):

https://codereview.chromium.org/12533007/diff/13001/src/gpu/gl/GrGLVertexArra...
src/gpu/gl/GrGLVertexArray.h:96: 
On 2013/03/06 16:14:20, robertphillips wrote:
> // comment?

Done.

https://codereview.chromium.org/12533007/diff/13001/src/gpu/gl/GrGLVertexArra...
src/gpu/gl/GrGLVertexArray.h:134: GrGLuint                fIndexBufferID;
On 2013/03/06 16:14:20, robertphillips wrote:
> not a bool?

Done.

https://codereview.chromium.org/12533007/diff/13001/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

https://codereview.chromium.org/12533007/diff/13001/src/gpu/gl/GrGpuGL.cpp#ne...
src/gpu/gl/GrGpuGL.cpp:2333: }
On 2013/03/06 16:14:20, robertphillips wrote:
> Returning a non-const fDefaultVertexArrayAttribState here seems a bit
worrying.

The returned object is used to set attrib arrays (in GrGpuGL::setupGeometry) and
doing so mutates the object because it is tracking the attrib array state.

https://codereview.chromium.org/12533007/diff/13001/src/gpu/gl/GrGpuGL.h
File src/gpu/gl/GrGpuGL.h (right):

https://codereview.chromium.org/12533007/diff/13001/src/gpu/gl/GrGpuGL.h#newc...
src/gpu/gl/GrGpuGL.h:377: 
On 2013/03/06 16:14:20, robertphillips wrote:
> one -> using

Done.

[jvanverth1, 2013-03-06 17:50:12]

Just one comment so far.

https://codereview.chromium.org/12533007/diff/20003/src/gpu/gl/GrGLVertexArray.h
File src/gpu/gl/GrGLVertexArray.h (right):

https://codereview.chromium.org/12533007/diff/20003/src/gpu/gl/GrGLVertexArra...
src/gpu/gl/GrGLVertexArray.h:77: struct AttribArray {
I think this struct name is confusing -- isn't it an array element (an Attrib?)
more than an array?

[bsalomon, 2013-03-06 18:31:07]

https://codereview.chromium.org/12533007/diff/20003/src/gpu/gl/GrGLVertexArray.h
File src/gpu/gl/GrGLVertexArray.h (right):

https://codereview.chromium.org/12533007/diff/20003/src/gpu/gl/GrGLVertexArra...
src/gpu/gl/GrGLVertexArray.h:77: struct AttribArray {
On 2013/03/06 17:50:12, JimVV wrote:
> I think this struct name is confusing -- isn't it an array element (an
Attrib?)
> more than an array?

Hmm... I get what you mean. It isn't a C array but it does hold the state of a
GL attribute array set by glVertexAttribArray.  Does making it not end in Array
make it clearer? e.g. AttribArrayState.

[robertphillips, 2013-03-06 21:00:16]

lgtm modulo a bunch of questions

https://codereview.chromium.org/12533007/diff/20003/src/gpu/gl/GrGLVertexArra...
File src/gpu/gl/GrGLVertexArray.cpp (right):

https://codereview.chromium.org/12533007/diff/20003/src/gpu/gl/GrGLVertexArra...
src/gpu/gl/GrGLVertexArray.cpp:57: GR_GL_CALL(gpu->glInterface(),
DisableVertexAttribArray(i));
is this supposed to be true?

https://codereview.chromium.org/12533007/diff/20003/src/gpu/gl/GrGLVertexArra...
src/gpu/gl/GrGLVertexArray.cpp:112: if (fIndexBufferIDIsValid && bufferID ==
fIndexBufferID) {
reset fIndexBufferIDIsValid too?

https://codereview.chromium.org/12533007/diff/20003/src/gpu/gl/GrGLVertexArray.h
File src/gpu/gl/GrGLVertexArray.h (right):

https://codereview.chromium.org/12533007/diff/20003/src/gpu/gl/GrGLVertexArra...
src/gpu/gl/GrGLVertexArray.h:77: struct AttribArray {
I think that is clearer (AttribArrayState that is) - and perhaps a comment?

https://codereview.chromium.org/12533007/diff/20003/src/gpu/gl/GrGpuGL.h
File src/gpu/gl/GrGpuGL.h (right):

https://codereview.chromium.org/12533007/diff/20003/src/gpu/gl/GrGpuGL.h#newc...
src/gpu/gl/GrGpuGL.h:316: if (fBoundVertexBufferIDIsValid && id ==
fBoundVertexBufferID) {
false? We already know it is true.

https://codereview.chromium.org/12533007/diff/20003/src/gpu/gl/GrGpuGL.h#newc...
src/gpu/gl/GrGpuGL.h:328: id == fDefaultVertexArrayBoundIndexBufferID) {
false? We already know it is true.

https://codereview.chromium.org/12533007/diff/20003/src/gpu/gl/GrGpuGL.h#newc...
src/gpu/gl/GrGpuGL.h:361: * The vertex array is bound and its attrib array state
object is returned. The vertex
missing 'is'?

[bsalomon, 2013-03-06 21:32:53]

https://codereview.chromium.org/12533007/diff/20003/src/gpu/gl/GrGLVertexArra...
File src/gpu/gl/GrGLVertexArray.cpp (right):

https://codereview.chromium.org/12533007/diff/20003/src/gpu/gl/GrGLVertexArra...
src/gpu/gl/GrGLVertexArray.cpp:57: GR_GL_CALL(gpu->glInterface(),
DisableVertexAttribArray(i));
On 2013/03/06 21:00:16, robertphillips wrote:
> is this supposed to be true?

Yeah, the bool fEnabledIsValid indicates whether fEnabled is valid or not
(represents known GL state).

https://codereview.chromium.org/12533007/diff/20003/src/gpu/gl/GrGLVertexArra...
src/gpu/gl/GrGLVertexArray.cpp:112: if (fIndexBufferIDIsValid && bufferID ==
fIndexBufferID) {
On 2013/03/06 21:00:16, robertphillips wrote:
> reset fIndexBufferIDIsValid too?

We already checked that it's true on the previous line.

https://codereview.chromium.org/12533007/diff/20003/src/gpu/gl/GrGLVertexArray.h
File src/gpu/gl/GrGLVertexArray.h (right):

https://codereview.chromium.org/12533007/diff/20003/src/gpu/gl/GrGLVertexArra...
src/gpu/gl/GrGLVertexArray.h:77: struct AttribArray {
On 2013/03/06 21:00:16, robertphillips wrote:
> I think that is clearer (AttribArrayState that is) - and perhaps a comment?

Done.

https://codereview.chromium.org/12533007/diff/20003/src/gpu/gl/GrGpuGL.h
File src/gpu/gl/GrGpuGL.h (right):

https://codereview.chromium.org/12533007/diff/20003/src/gpu/gl/GrGpuGL.h#newc...
src/gpu/gl/GrGpuGL.h:316: if (fBoundVertexBufferIDIsValid && id ==
fBoundVertexBufferID) {
On 2013/03/06 21:00:16, robertphillips wrote:
> false? We already know it is true.

Removed the line (it should stay true)

https://codereview.chromium.org/12533007/diff/20003/src/gpu/gl/GrGpuGL.h#newc...
src/gpu/gl/GrGpuGL.h:328: id == fDefaultVertexArrayBoundIndexBufferID) {
On 2013/03/06 21:00:16, robertphillips wrote:
> false? We already know it is true.

Removed the line (it should stay true)

https://codereview.chromium.org/12533007/diff/20003/src/gpu/gl/GrGpuGL.h#newc...
src/gpu/gl/GrGpuGL.h:361: * The vertex array is bound and its attrib array state
object is returned. The vertex
On 2013/03/06 21:00:16, robertphillips wrote:
> missing 'is'?

Done.

[bsalomon, 2013-03-07 16:04:45]

Use vertex array objects on core profiles.

[jvanverth1, 2013-03-07 16:17:44]

LGTM + one minor nit

https://codereview.chromium.org/12533007/diff/29001/src/gpu/gl/GrGpuGL.h
File src/gpu/gl/GrGpuGL.h (right):

https://codereview.chromium.org/12533007/diff/29001/src/gpu/gl/GrGpuGL.h#newc...
src/gpu/gl/GrGpuGL.h:78: void notifyVertexArrayDelete(GrGLuint id) {
Match the order of the notify*Delete methods with the bind* methods? (i.e. move
notifyVertexArrayDelete before notifyVertexBufferDelete).

[bsalomon, 2013-03-07 16:25:07]

https://codereview.chromium.org/12533007/diff/29001/src/gpu/gl/GrGpuGL.h
File src/gpu/gl/GrGpuGL.h (right):

https://codereview.chromium.org/12533007/diff/29001/src/gpu/gl/GrGpuGL.h#newc...
src/gpu/gl/GrGpuGL.h:78: void notifyVertexArrayDelete(GrGLuint id) {
On 2013/03/07 16:17:44, JimVV wrote:
> Match the order of the notify*Delete methods with the bind* methods? (i.e.
move
> notifyVertexArrayDelete before notifyVertexBufferDelete).
will do before landing.

[bsalomon, 2013-03-07 19:09:21]

Committed patchset #7 manually as r8024 (presubmit successful).