Make GL bindings conditional

Make GL bindings conditional

In addition to the extensions also parse GL/GLES version
information from the headers (hence add a few fresh headers).

Instead of brute-forcing binding in somewhat unclear order
make all dynamic bindings very specifically conditional to
the GL version or extension, given the precise name.
If a function is not available, then leave function pointer
set to NULL so it will generate a crash (report).
(Previously it might have ended up as 0
or with whatever valid or not valid pointer GetProcAddr()
returned - i.e. GetProcAddr returning something does not
imply the functionality is there.)

The version also includes whether it is GLES or not which
means we won't look up functions on GLES anymore that are
specific to desktop or vice versa.

Update gpu_unittests so that they are very specific to what
extensions they need (otherwise they might end up calling
the _NotBound fallback).

Also make WGL/GLX/EGL/MESA bindings all happen rightaway since
they don't depend on a context.

BUG=438742, 325668
Committed: https://crrev.com/e3b17fd533b81a9855a28d04894021479cd6d89d
Cr-Commit-Position: refs/heads/master@{#313663}

[no sievers, 2015-01-23 00:45:30]

sievers@chromium.org changed reviewers:
+ kbr@chromium.org, piman@chromium.org, vmiura@chromium.org, zmo@chromium.org

[no sievers, 2015-01-23 01:05:09]

Patchset #2 (id:10001) has been deleted

[no sievers, 2015-01-23 01:08:02]

ptal

[piman, 2015-01-23 01:38:16]

https://codereview.chromium.org/871763002/diff/30001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc (right):

https://codereview.chromium.org/871763002/diff/30001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:432: if
(version->IsAtLeastGL(3, 3) || version->IsAtLeastGLES(3, 3)) {
IsAtLeastGLES(3, 0) ?

There isn't such a thing as ES 3.3 :)

https://codereview.chromium.org/871763002/diff/30001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:438: if
(version->IsAtLeastGL(4, 4) || version->IsAtLeastGLES(3, 3)) {
glGenTransformFeedbacks is present in GL 4.0 and ES 3.0

https://codereview.chromium.org/871763002/diff/30001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer_unittest.cc (right):

https://codereview.chromium.org/871763002/diff/30001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer_unittest.cc:229: // is available when
timer_query or disjoint_timer exists.
GL_ARB_timer_query requires GL 3.2

GL_EXT_disjoint_timer_query mentions glGetInteger64v, present in ES 3.0, but the
extension only requires ES 2.0, so I guess it has a spec bug :)

[no sievers, 2015-01-23 01:41:01]

https://codereview.chromium.org/871763002/diff/30001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc (right):

https://codereview.chromium.org/871763002/diff/30001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:438: if
(version->IsAtLeastGL(4, 4) || version->IsAtLeastGLES(3, 3)) {
On 2015/01/23 01:38:16, piman (Very slow to review) wrote:
> glGenTransformFeedbacks is present in GL 4.0 and ES 3.0

Hmm and it's right in the autogenerated bindings. Not sure how I got it so
totally wrong here :)

[no sievers, 2015-01-23 01:42:47]

On 2015/01/23 01:38:16, piman (Very slow to review) wrote:
>
https://codereview.chromium.org/871763002/diff/30001/gpu/command_buffer/servi...
> File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc (right):
> 
>
https://codereview.chromium.org/871763002/diff/30001/gpu/command_buffer/servi...
> gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:432: if
> (version->IsAtLeastGL(3, 3) || version->IsAtLeastGLES(3, 3)) {
> IsAtLeastGLES(3, 0) ?
> 
> There isn't such a thing as ES 3.3 :)
> 
>
https://codereview.chromium.org/871763002/diff/30001/gpu/command_buffer/servi...
> gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:438: if
> (version->IsAtLeastGL(4, 4) || version->IsAtLeastGLES(3, 3)) {
> glGenTransformFeedbacks is present in GL 4.0 and ES 3.0
> 
>
https://codereview.chromium.org/871763002/diff/30001/gpu/command_buffer/servi...
> File gpu/command_buffer/service/gpu_tracer_unittest.cc (right):
> 
>
https://codereview.chromium.org/871763002/diff/30001/gpu/command_buffer/servi...
> gpu/command_buffer/service/gpu_tracer_unittest.cc:229: // is available when
> timer_query or disjoint_timer exists.
> GL_ARB_timer_query requires GL 3.2
> 
> GL_EXT_disjoint_timer_query mentions glGetInteger64v, present in ES 3.0, but
the
> extension only requires ES 2.0, so I guess it has a spec bug :)

Yea, David is just making that tracer code conditional on ES3:
https://codereview.chromium.org/866593002/

You can still theoretically make use of the timer queries with the ES2 core and
functions from the spec. But it's more painful with the queries.

[Zhenyao Mo, 2015-01-23 01:48:20]

This is really awesome.  Mostly looks good to me with a few questions.

https://codereview.chromium.org/871763002/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc (right):

https://codereview.chromium.org/871763002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:432: if
(version->IsAtLeastGL(3, 3) || version->IsAtLeastGLES(3, 3)) {
you mean 3.0? here and below.

https://codereview.chromium.org/871763002/diff/20001/third_party/khronos/READ...
File third_party/khronos/README.chromium (right):

https://codereview.chromium.org/871763002/diff/20001/third_party/khronos/READ...
third_party/khronos/README.chromium:25: - Added GL extension headers from
Khronos
You only list files here if you modify them from the original version.

https://codereview.chromium.org/871763002/diff/20001/ui/gl/gl_bindings_autoge...
File ui/gl/gl_bindings_autogen_gl.cc (right):

https://codereview.chromium.org/871763002/diff/20001/ui/gl/gl_bindings_autoge...
ui/gl/gl_bindings_autogen_gl.cc:750: glBeginTransformFeedbackNotBound);
Can't these just be NULL?  Then you don't have to generate all these NOTREACHED
functions.

Call them will end up crashing either way.

[no sievers, 2015-01-24 01:50:46]

updated

https://codereview.chromium.org/871763002/diff/20001/ui/gl/gl_bindings_autoge...
File ui/gl/gl_bindings_autogen_gl.cc (right):

https://codereview.chromium.org/871763002/diff/20001/ui/gl/gl_bindings_autoge...
ui/gl/gl_bindings_autogen_gl.cc:750: glBeginTransformFeedbackNotBound);
On 2015/01/23 01:48:20, Zhenyao Mo wrote:
> Can't these just be NULL?  Then you don't have to generate all these
NOTREACHED
> functions.
> 
> Call them will end up crashing either way.

Except for in a release build we would handle it more gracefully.
But I don't feel strongly either way. What do others think?

https://codereview.chromium.org/871763002/diff/30001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc (right):

https://codereview.chromium.org/871763002/diff/30001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:432: if
(version->IsAtLeastGL(3, 3) || version->IsAtLeastGLES(3, 3)) {
On 2015/01/23 01:38:16, piman (Very slow to review) wrote:
> IsAtLeastGLES(3, 0) ?
> 
> There isn't such a thing as ES 3.3 :)

Done.

https://codereview.chromium.org/871763002/diff/30001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:438: if
(version->IsAtLeastGL(4, 4) || version->IsAtLeastGLES(3, 3)) {
On 2015/01/23 01:41:01, sievers wrote:
> On 2015/01/23 01:38:16, piman (Very slow to review) wrote:
> > glGenTransformFeedbacks is present in GL 4.0 and ES 3.0
> 
> Hmm and it's right in the autogenerated bindings. Not sure how I got it so
> totally wrong here :)

Done.

https://codereview.chromium.org/871763002/diff/40001/third_party/khronos/GL/g...
File third_party/khronos/GL/glext.h (right):

https://codereview.chromium.org/871763002/diff/40001/third_party/khronos/GL/g...
third_party/khronos/GL/glext.h:471: typedef khronos_ssize_t  GLsizeiptr;
I've made this change (and below) and documented it in README.chromium.
Otherwise a few builders were failing with long vs. int mismatches.

https://codereview.chromium.org/871763002/diff/40001/ui/gl/generate_bindings.py
File ui/gl/generate_bindings.py (right):

https://codereview.chromium.org/871763002/diff/40001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:1906: return 'ver->IsAtLeastGLES(%du, %du)' % (
simplified the generated code to use the helper function. makes it more
readable.

[no sievers, 2015-01-24 01:52:38]

https://codereview.chromium.org/871763002/diff/20001/ui/gl/gl_bindings_autoge...
File ui/gl/gl_bindings_autogen_gl.cc (right):

https://codereview.chromium.org/871763002/diff/20001/ui/gl/gl_bindings_autoge...
ui/gl/gl_bindings_autogen_gl.cc:750: glBeginTransformFeedbackNotBound);
On 2015/01/24 01:50:46, sievers wrote:
> On 2015/01/23 01:48:20, Zhenyao Mo wrote:
> > Can't these just be NULL?  Then you don't have to generate all these
> NOTREACHED
> > functions.
> > 
> > Call them will end up crashing either way.
> 
> Except for in a release build we would handle it more gracefully.
> But I don't feel strongly either way. What do others think?

On second thought: it would be nice to get crash reports if there is some
problem (like we are missing a binding because an extension in the header didn't
list the prototypes or so). How about I do it in a follow-up?

[Ken Russell (switch to Gerrit), 2015-01-24 05:08:05]

First, this is really awesome. It makes the bindings much more correct and their
behavior easier to understand. Thank you for doing this! I can't wait to see
this land.

Regarding whether to have the NotBound functions: I would remove them and
initialize the dynamically looked up function pointers to NULL, and do that in
this CL. The rationale is that this is a big change, and you don't want two
dependent CLs back-to-back that both might need to be reverted if it turns out
crashes are discovered when the NotBound functions are removed.

In order to unblock your work, this LGTM overall once the following comments and
questions are addressed. Will be on business travel next Tue-Fri.

https://codereview.chromium.org/871763002/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc (right):

https://codereview.chromium.org/871763002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:446: if
(init.extensions.find("GL_ARB_sync ") != std::string::npos ||
What if this one is the last in the extension string?

https://codereview.chromium.org/871763002/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer_unittest.cc (right):

https://codereview.chromium.org/871763002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer_unittest.cc:228: // TODO: The tracer code
is wrong and assumes that getInteger64v
Add owner for TODO.

https://codereview.chromium.org/871763002/diff/40001/third_party/khronos/GL/g...
File third_party/khronos/GL/glext.h (right):

https://codereview.chromium.org/871763002/diff/40001/third_party/khronos/GL/g...
third_party/khronos/GL/glext.h:471: typedef khronos_ssize_t  GLsizeiptr;
On 2015/01/24 01:50:46, sievers wrote:
> I've made this change (and below) and documented it in README.chromium.
> Otherwise a few builders were failing with long vs. int mismatches.

Would it be possible -- or wise -- to put the desktop GL headers in a
subdirectory like khronos/noinclude/GL that won't be found during Chromium's
compilation, but could be found by the bindings generator?

Would that work at all?

I'm assuming that we don't actually need the function pointer prototypes from
these headers at compilation time. This particular build issue is one I vaguely
remember from a while back, and at this point I thought we'd eliminated all
include paths to desktop GL headers during Chromium's build process.

https://codereview.chromium.org/871763002/diff/40001/ui/gl/generate_bindings.py
File ui/gl/generate_bindings.py (right):

https://codereview.chromium.org/871763002/diff/40001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:37: headers.
For future reference, does the binding generator give any notification when this
happens?

https://codereview.chromium.org/871763002/diff/40001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:55: 'extensions': ['GL_EXT_occlusion_query_boolean']
}],
So glBeginQueryARB is properly attributed to GL_ARB_occlusion_query from the
headers?

https://codereview.chromium.org/871763002/diff/40001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:1906: return 'ver->IsAtLeastGLES(%du, %du)' % (
On 2015/01/24 01:50:46, sievers wrote:
> simplified the generated code to use the helper function. makes it more
> readable.

Great.

https://codereview.chromium.org/871763002/diff/40001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:2255: Core GL 1.x, 2.x and GLES2 functions are not
returned.
Is this comment accurate? Core GLES2 functions aren't returned from this?

https://codereview.chromium.org/871763002/diff/40001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:2291: if not is_es and major_version == '1':
Could you parenthesize this expression to make it clearer?

[no sievers, 2015-01-26 21:13:03]

Addressed kbr's comments.
Unbound functions are also set to null now.

https://codereview.chromium.org/871763002/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc (right):

https://codereview.chromium.org/871763002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:446: if
(init.extensions.find("GL_ARB_sync ") != std::string::npos ||
On 2015/01/24 05:08:05, Ken Russell wrote:
> What if this one is the last in the extension string?

See NormalizeInitState():
 if (!init->extensions.empty())
   init->extensions += " ";

https://codereview.chromium.org/871763002/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer_unittest.cc (right):

https://codereview.chromium.org/871763002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer_unittest.cc:228: // TODO: The tracer code
is wrong and assumes that getInteger64v
On 2015/01/24 05:08:04, Ken Russell wrote:
> Add owner for TODO.

Done.

https://codereview.chromium.org/871763002/diff/40001/third_party/khronos/GL/g...
File third_party/khronos/GL/glext.h (right):

https://codereview.chromium.org/871763002/diff/40001/third_party/khronos/GL/g...
third_party/khronos/GL/glext.h:471: typedef khronos_ssize_t  GLsizeiptr;
On 2015/01/24 05:08:05, Ken Russell wrote:
> On 2015/01/24 01:50:46, sievers wrote:
> > I've made this change (and below) and documented it in README.chromium.
> > Otherwise a few builders were failing with long vs. int mismatches.
> 
> Would it be possible -- or wise -- to put the desktop GL headers in a
> subdirectory like khronos/noinclude/GL that won't be found during Chromium's
> compilation, but could be found by the bindings generator?
> 
> Would that work at all?
> 
> I'm assuming that we don't actually need the function pointer prototypes from
> these headers at compilation time. This particular build issue is one I
vaguely
> remember from a while back, and at this point I thought we'd eliminated all
> include paths to desktop GL headers during Chromium's build process.

Done.

https://codereview.chromium.org/871763002/diff/40001/ui/gl/generate_bindings.py
File ui/gl/generate_bindings.py (right):

https://codereview.chromium.org/871763002/diff/40001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:37: headers.
On 2015/01/24 05:08:04, Ken Russell wrote:
> For future reference, does the binding generator give any notification when
this
> happens?

Yes, it prints out both below:
"Specified redundant extensions for binding" or "Specified extra extensions for
binding". So we can follow up and try simplify things.

https://codereview.chromium.org/871763002/diff/40001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:55: 'extensions': ['GL_EXT_occlusion_query_boolean']
}],
On 2015/01/24 05:08:04, Ken Russell wrote:
> So glBeginQueryARB is properly attributed to GL_ARB_occlusion_query from the
> headers?

Yes, glext.h:GL_ARB_occlusion_query has the prototypes (glBeginQueryARB(  etc.)
declared.
Somehow gl2ext.h:GL_EXT_occlusion_query_boolean does not (glBeginQueryEXT etc.).

We could follow up and try remove all 'extra extensions' listed here (some are
already redundant) and update the headers.

I think there are a few cases though where there is an overlap (same name in
multiple extensions or also in the core) and the headers seem to generally avoid
declaring the prototypes more than once.

https://codereview.chromium.org/871763002/diff/40001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:2255: Core GL 1.x, 2.x and GLES2 functions are not
returned.
On 2015/01/24 05:08:04, Ken Russell wrote:
> Is this comment accurate? Core GLES2 functions aren't returned from this?

Oops, not anymore. Done.

https://codereview.chromium.org/871763002/diff/40001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:2291: if not is_es and major_version == '1':
On 2015/01/24 05:08:05, Ken Russell wrote:
> Could you parenthesize this expression to make it clearer?

Done.

[Zhenyao Mo, 2015-01-26 21:24:06]

LGTM

This is really great refractoring!

[Ken Russell (switch to Gerrit), 2015-01-26 23:10:16]

Excellent. Thanks for the update. LGTM again. One minor comment.

https://codereview.chromium.org/871763002/diff/80001/third_party/khronos/READ...
File third_party/khronos/README.chromium (right):

https://codereview.chromium.org/871763002/diff/80001/third_party/khronos/READ...
third_party/khronos/README.chromium:25: to avoid build errors (long vs. int) on
some 32bit systems
I think this change shouldn't be needed any more.

[no sievers, 2015-01-26 23:29:28]

On 2015/01/26 23:10:16, Ken Russell wrote:
> Excellent. Thanks for the update. LGTM again. One minor comment.
> 
>
https://codereview.chromium.org/871763002/diff/80001/third_party/khronos/READ...
> File third_party/khronos/README.chromium (right):
> 
>
https://codereview.chromium.org/871763002/diff/80001/third_party/khronos/READ...
> third_party/khronos/README.chromium:25: to avoid build errors (long vs. int)
on
> some 32bit systems
> I think this change shouldn't be needed any more.

Done.

[piman, 2015-01-27 21:38:02]

LGTM. I support binding NULL, to avoid unnecessary bloat, and get crashes.

[no sievers, 2015-01-29 02:36:18]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2015-01-29 02:37:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/871763002/120001

[commit-bot: I haz the power, 2015-01-29 04:01:27]

Committed patchset #12 (id:120001)

[commit-bot: I haz the power, 2015-01-29 04:02:26]

Patchset 12 (id:??) landed as
https://crrev.com/e3b17fd533b81a9855a28d04894021479cd6d89d
Cr-Commit-Position: refs/heads/master@{#313663}

[Daniel Kurtz, 2015-02-03 16:07:52]

On 2015/01/29 04:02:26, I haz the power (commit-bot) wrote:
> Patchset 12 (id:??) landed as
> https://crrev.com/e3b17fd533b81a9855a28d04894021479cd6d89d
> Cr-Commit-Position: refs/heads/master@{#313663}

I think this patch killed WebGL on all ARM chromebooks.

[Daniel Kurtz, 2015-02-03 16:10:14]

On 2015/02/03 16:07:52, Daniel Kurtz wrote:
> On 2015/01/29 04:02:26, I haz the power (commit-bot) wrote:
> > Patchset 12 (id:??) landed as
> > https://crrev.com/e3b17fd533b81a9855a28d04894021479cd6d89d
> > Cr-Commit-Position: refs/heads/master@{#313663}
> 
> I think this patch killed WebGL on all ARM chromebooks.

https://code.google.com/p/chromium/issues/detail?id=454816

[Daniel Kurtz, 2015-02-03 16:13:38]

A revert of this CL (patchset #12 id:120001) has been created in
https://codereview.chromium.org/893213002/ by djkurtz@chromium.org.

The reason for reverting is: Suspected of breaking WebGL on ARM (Mali) Chrome
OS.

BUG=454816.