Added EGL based GLContext....

Added EGL based GLContext.
Python script to generate code to dynamically bind to GL functions (native GL, OSMesa, EGL or mock GL for unit tests). This replaces GLEW because GLEW doesn't bind to the GLES dialect of GL.
Moved the mock GL code into app/gfx/gl.
Updated the GPU code and AcceleratedSurface to use the new GL bindings.


TEST=trybots
BUG=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=49332

[apatrick_chromium, 2010-06-02 23:45:42]

This is going to need some more work. In particular, mac will not work yet. I
would appreciate some feeback at this stage though.

scherkus, I am at a loss as to what to do with the OpenGL code in chrome/gpu.
Among other things, it is using the fixed function pipeline, which is not
supported by GLES2. I could add those functions to my GLEW replacement and they
would work if desktop GL was present. It seems moving to VBOs and shaders might
be a better bet longer term. Any thoughts?

[greggman, 2010-06-03 01:45:05]

Looking good. Please don't make me read it again :-p

A few random comments.

Would there be any point in making the various GetGLProc functions use a table
of pointers to functions and a table of pointers to function pointers and then a
loop instead of a giant set of ifs?  I don't really care either way, just
thought I'd ask.

Also, I'm a little concerned that calling GL functions (vs OpenGL ES functions)
will be hard. You've already got comments for a few of them. I guess I don't see
any other way except to add them.

One other thing, since the code generation is pretty straight forward should it
just be part of the build? Then you don't need to check in the auto-generated
files and you won't get tons of lint errors since they generate lots of > 80
char lines. A lame reason do to it I guess but I thought I'd ask.

http://codereview.chromium.org/2134006/diff/42001/43003
File app/gfx/gl/generate_bindings.py (right):

http://codereview.chromium.org/2134006/diff/42001/43003#newcode321
app/gfx/gl/generate_bindings.py:321: ['XVisualInfo*', ['glXChooseVisual'],
'Display* dpy, int screen, int* attribList'],
I don't really care but I noticed all the tables above are formatted to 80 chars
and then it appears you decided to stop doing that. I think some people probably
don't care about this table but they probably do care about the code not being
80 chars.

http://codereview.chromium.org/2134006/diff/42001/43003#newcode501
app/gfx/gl/generate_bindings.py:501: headerFile =
open('gl_bindings_autogen_%s.h' % setName, 'w')
w -> wb?  Otherwise you'll get different line endings on Windows and gcl lint
will complain when you try to check stuff in.

Same below

http://codereview.chromium.org/2134006/diff/42001/43004
File app/gfx/gl/gl_bindings.h (right):

http://codereview.chromium.org/2134006/diff/42001/43004#newcode13
app/gfx/gl/gl_bindings.h:13: #include <GL/gl.h>
These won't be available on native GLES2 implementations like ChromeOS. Is that
okay?

http://codereview.chromium.org/2134006/diff/42001/43004#newcode73
app/gfx/gl/gl_bindings.h:73: void* GL_BINDING_CALL GetMockGLProcAddress(const
char* name);
Does this work when compiling tests on windows? Won't it make them all WINAPI
calls?

http://codereview.chromium.org/2134006/diff/42001/43020
File app/gfx/gl/gl_context_linux.cc (right):

http://codereview.chromium.org/2134006/diff/42001/43020#newcode125
app/gfx/gl/gl_context_linux.cc:125: if (GetGLImplementation() ==
GL_IMPLEMENTATION_NATIVE) {
I don't quite understand this code? You just inited it to native above? How can
this if ever fail?

http://codereview.chromium.org/2134006/diff/42001/43026
File app/gfx/gl/gl_implementation.h (right):

http://codereview.chromium.org/2134006/diff/42001/43026#newcode12
app/gfx/gl/gl_implementation.h:12: GL_IMPLEMENTATION_NONE,
enums should be kCamelCase?

http://codereview.chromium.org/2134006/diff/42001/43027
File app/gfx/gl/gl_implementation_linux.cc (right):

http://codereview.chromium.org/2134006/diff/42001/43027#newcode25
app/gfx/gl/gl_implementation_linux.cc:25: if (g_gl_implementation !=
GL_IMPLEMENTATION_NONE)
Is there a DO_ONCE type macro in chromium base?

http://codereview.chromium.org/2134006/diff/42001/43061
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/2134006/diff/42001/43061#newcode1421
gpu/command_buffer/service/gles2_cmd_decoder.cc:1421: gfx::GetGLImplementation()
== gfx::GL_IMPLEMENTATION_MOCK) {
Pepper expects the translator to function TODAY as do the conformance tests. The
only place the translator must not be used is WebGL.  The unit tests don't
really care.

http://codereview.chromium.org/2134006/diff/42001/43061#newcode1537
gpu/command_buffer/service/gles2_cmd_decoder.cc:1537: if
(gfx::GetGLImplementation() != gfx::GL_IMPLEMENTATION_EGL) {
So EGL does this for us?  I guess I don't understand what the various
implementations mean. What's the difference between GL_IMPLEMENTATION_EGL and
GL_IMPLEMENTATION_LINUX if LINUX has EGL?

Shouldn't this check be GL_IMPLEMENTATION_NATIVE_GLES2? or something like that?

http://codereview.chromium.org/2134006/diff/42001/43061#newcode1795
gpu/command_buffer/service/gles2_cmd_decoder.cc:1795: pending_offscreen_size_,
GL_DEPTH_COMPONENT16)) {
Do we want 24 and if there is no 24 than fall back to 16?

http://codereview.chromium.org/2134006/diff/42001/43061#newcode3247
gpu/command_buffer/service/gles2_cmd_decoder.cc:3247: ShHandle compiler = 0;
Do you get a "compiler" unused variable warning->error with this outside of the
#if?

http://codereview.chromium.org/2134006/diff/42001/43061#newcode3271
gpu/command_buffer/service/gles2_cmd_decoder.cc:3271: }
Why are these lines missing? They're required for this code to run. Is this an
svn merge bug?

http://codereview.chromium.org/2134006/diff/42001/43067
File gpu/command_buffer/service/gpu_processor.h (right):

http://codereview.chromium.org/2134006/diff/42001/43067#newcode107
gpu/command_buffer/service/gpu_processor.h:107: scoped_ptr<AcceleratedSurface>
surface_;
I'm sure there's a good reason but why is this here and not just in
gpu_processor_mac?

[scherkus (not reviewing), 2010-06-03 22:14:45]

I only looked at the chrome/gpu/* stuff

don't worry about breaking that stuff.. brettw/myself were going to switch away
from immediate mode to VBOs one day :)

http://codereview.chromium.org/2134006/diff/42001/43038
File chrome/gpu/gpu_backing_store_glx_context.cc (right):

http://codereview.chromium.org/2134006/diff/42001/43038#newcode5
chrome/gpu/gpu_backing_store_glx_context.cc:5: #include <X11/Xutil.h>
does this have to go first?

I think all the types in the header file are appropriately forward declared

http://codereview.chromium.org/2134006/diff/42001/43038#newcode45
chrome/gpu/gpu_backing_store_glx_context.cc:45: context_);
indentation

http://codereview.chromium.org/2134006/diff/42001/43038#newcode60
chrome/gpu/gpu_backing_store_glx_context.cc:60: NULL, True);
indentation

http://codereview.chromium.org/2134006/diff/42001/43038#newcode100
chrome/gpu/gpu_backing_store_glx_context.cc:100: temp_scroll_texture_id_, 0);
indentation

http://codereview.chromium.org/2134006/diff/42001/43041
File chrome/gpu/gpu_view_x.cc (right):

http://codereview.chromium.org/2134006/diff/42001/43041#newcode5
chrome/gpu/gpu_view_x.cc:5: #include <X11/Xutil.h>
ditto for this headerfile

[apatrick_chromium, 2010-06-04 22:41:10]

Getting there...

http://codereview.chromium.org/2134006/diff/42001/43003
File app/gfx/gl/generate_bindings.py (right):

http://codereview.chromium.org/2134006/diff/42001/43003#newcode321
app/gfx/gl/generate_bindings.py:321: ['XVisualInfo*', ['glXChooseVisual'],
'Display* dpy, int screen, int* attribList'],
On 2010/06/03 01:45:06, greggman wrote:
> I don't really care but I noticed all the tables above are formatted to 80
chars
> and then it appears you decided to stop doing that. I think some people
probably
> don't care about this table but they probably do care about the code not
being
> 80 chars.

Done.

http://codereview.chromium.org/2134006/diff/42001/43003#newcode501
app/gfx/gl/generate_bindings.py:501: headerFile =
open('gl_bindings_autogen_%s.h' % setName, 'w')
On 2010/06/03 01:45:06, greggman wrote:
> w -> wb?  Otherwise you'll get different line endings on Windows and gcl lint
> will complain when you try to check stuff in.
> 
> Same below
> 

Done.

http://codereview.chromium.org/2134006/diff/42001/43004
File app/gfx/gl/gl_bindings.h (right):

http://codereview.chromium.org/2134006/diff/42001/43004#newcode13
app/gfx/gl/gl_bindings.h:13: #include <GL/gl.h>
On 2010/06/03 01:45:06, greggman wrote:
> These won't be available on native GLES2 implementations like ChromeOS. Is
that
> okay?

I added the mesa headers to the include search path so although OpenGL will not
be available, it can still use the Mesa OpenGL constants and types and the new
bindings will translate OpenGL calls to their GLES2 equivalents where
necessary.

If this code winds up in webkit, then this can still include the system OpenGL
headers. That's why it isn't:

#include "third_party/mesa/MesaLib/include/GL/gl.h"

http://codereview.chromium.org/2134006/diff/42001/43004#newcode73
app/gfx/gl/gl_bindings.h:73: void* GL_BINDING_CALL GetMockGLProcAddress(const
char* name);
On 2010/06/03 01:45:06, greggman wrote:
> Does this work when compiling tests on windows? Won't it make them all WINAPI
> calls?

The gpu_unittests project now registers a mock implementation of OpenGL that
calls into the your gmock objects. This overrides any implementation
InitializeOneOff attempts to register. Now we don't need to have a separation
compilation of the service side code to run the unit tests and UNIT_TEST will
never be defined. Now you can use GetGLImplementation() == kGLImplementationMock
to do a runtime check if really necessary.

http://codereview.chromium.org/2134006/diff/42001/43026
File app/gfx/gl/gl_implementation.h (right):

http://codereview.chromium.org/2134006/diff/42001/43026#newcode12
app/gfx/gl/gl_implementation.h:12: GL_IMPLEMENTATION_NONE,
On 2010/06/03 01:45:06, greggman wrote:
> enums should be kCamelCase?

Done.

http://codereview.chromium.org/2134006/diff/42001/43027
File app/gfx/gl/gl_implementation_linux.cc (right):

http://codereview.chromium.org/2134006/diff/42001/43027#newcode25
app/gfx/gl/gl_implementation_linux.cc:25: if (g_gl_implementation !=
GL_IMPLEMENTATION_NONE)
On 2010/06/03 01:45:06, greggman wrote:
> Is there a DO_ONCE type macro in chromium base?

I can't find one.

http://codereview.chromium.org/2134006/diff/42001/43038
File chrome/gpu/gpu_backing_store_glx_context.cc (right):

http://codereview.chromium.org/2134006/diff/42001/43038#newcode5
chrome/gpu/gpu_backing_store_glx_context.cc:5: #include <X11/Xutil.h>
On 2010/06/03 22:14:46, scherkus wrote:
> does this have to go first?
> 
> I think all the types in the header file are appropriately forward declared

Done.

http://codereview.chromium.org/2134006/diff/42001/43038#newcode45
chrome/gpu/gpu_backing_store_glx_context.cc:45: context_);
On 2010/06/03 22:14:46, scherkus wrote:
> indentation

Done.

http://codereview.chromium.org/2134006/diff/42001/43038#newcode60
chrome/gpu/gpu_backing_store_glx_context.cc:60: NULL, True);
On 2010/06/03 22:14:46, scherkus wrote:
> indentation

Done.

http://codereview.chromium.org/2134006/diff/42001/43038#newcode100
chrome/gpu/gpu_backing_store_glx_context.cc:100: temp_scroll_texture_id_, 0);
On 2010/06/03 22:14:46, scherkus wrote:
> indentation

Done.

http://codereview.chromium.org/2134006/diff/42001/43041
File chrome/gpu/gpu_view_x.cc (right):

http://codereview.chromium.org/2134006/diff/42001/43041#newcode5
chrome/gpu/gpu_view_x.cc:5: #include <X11/Xutil.h>
On 2010/06/03 22:14:46, scherkus wrote:
> ditto for this headerfile

Done.

http://codereview.chromium.org/2134006/diff/42001/43061
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/2134006/diff/42001/43061#newcode1421
gpu/command_buffer/service/gles2_cmd_decoder.cc:1421: gfx::GetGLImplementation()
== gfx::GL_IMPLEMENTATION_MOCK) {
On 2010/06/03 01:45:06, greggman wrote:
> Pepper expects the translator to function TODAY as do the conformance tests.
The
> only place the translator must not be used is WebGL.  The unit tests don't
> really care.

I think we never want to use the translator (here) with EGL/GLES2. I turned it
off for the tests because they fail when the translator is used, I think because
"" is not a valid shader.

http://codereview.chromium.org/2134006/diff/42001/43061#newcode1537
gpu/command_buffer/service/gles2_cmd_decoder.cc:1537: if
(gfx::GetGLImplementation() != gfx::GL_IMPLEMENTATION_EGL) {
On 2010/06/03 01:45:06, greggman wrote:
> So EGL does this for us?  I guess I don't understand what the various
> implementations mean. What's the difference between GL_IMPLEMENTATION_EGL and
> GL_IMPLEMENTATION_LINUX if LINUX has EGL?
> 
> Shouldn't this check be GL_IMPLEMENTATION_NATIVE_GLES2? or something like
that?

Done.

http://codereview.chromium.org/2134006/diff/42001/43061#newcode1795
gpu/command_buffer/service/gles2_cmd_decoder.cc:1795: pending_offscreen_size_,
GL_DEPTH_COMPONENT16)) {
On 2010/06/03 01:45:06, greggman wrote:
> Do we want 24 and if there is no 24 than fall back to 16?

Added comment.

http://codereview.chromium.org/2134006/diff/42001/43061#newcode3247
gpu/command_buffer/service/gles2_cmd_decoder.cc:3247: ShHandle compiler = 0;
On 2010/06/03 01:45:06, greggman wrote:
> Do you get a "compiler" unused variable warning->error with this outside of
the
> #if?

Done.

http://codereview.chromium.org/2134006/diff/42001/43061#newcode3271
gpu/command_buffer/service/gles2_cmd_decoder.cc:3271: }
On 2010/06/03 01:45:06, greggman wrote:
> Why are these lines missing? They're required for this code to run. Is this
an
> svn merge bug?

Looks like a bad merge.

http://codereview.chromium.org/2134006/diff/42001/43067
File gpu/command_buffer/service/gpu_processor.h (right):

http://codereview.chromium.org/2134006/diff/42001/43067#newcode107
gpu/command_buffer/service/gpu_processor.h:107: scoped_ptr<AcceleratedSurface>
surface_;
On 2010/06/03 01:45:06, greggman wrote:
> I'm sure there's a good reason but why is this here and not just in
> gpu_processor_mac?

It's just because we don't have a mac specific definition of this class. I don't
think there are sufficient differences to justify doind that.

[Ken Russell (switch to Gerrit), 2010-06-05 00:50:47]

Having trouble viewing a lot of the diffs in this version. Errors are of the
form

Error fetching svn://chrome-svn/chrome/trunk/src/DEPS?rev=48684:
InvalidURLError: ApplicationError: 1

Not sure whether re-syncing your workspace and re-uploading the CL might fix
this.

Since it looks like this needs to be a clean cutover from GLEW let's figure out
how to get the Mac port done ASAP so this can be checked in. I can help with
that.

A few comments. What I was able to see mostly looks good.

http://codereview.chromium.org/2134006/diff/89001/90003
File app/gfx/gl/generate_bindings.py (right):

http://codereview.chromium.org/2134006/diff/89001/90003#newcode413
app/gfx/gl/generate_bindings.py:413: # Write prototype for initilization
function.
initialization

http://codereview.chromium.org/2134006/diff/89001/90003#newcode467
app/gfx/gl/generate_bindings.py:467: file.write('void InitializeGLBindings%s()
{\n' % setName.upper())
Consider returning bool which indicates whether all function pointers were
looked up successfully, as GLEW does.

http://codereview.chromium.org/2134006/diff/89001/90016
File app/gfx/gl/gl_implementation_linux.cc (right):

http://codereview.chromium.org/2134006/diff/89001/90016#newcode23
app/gfx/gl/gl_implementation_linux.cc:23: // it might because the mock unit test
implementation was initialized and
There's something wrong with the grammar in this comment.

http://codereview.chromium.org/2134006/diff/89001/90017
File app/gfx/gl/gl_implementation_mac.cc (right):

http://codereview.chromium.org/2134006/diff/89001/90017#newcode22
app/gfx/gl/gl_implementation_mac.cc:22: // it might because the mock unit test
implementation was initialized and
Grammar

http://codereview.chromium.org/2134006/diff/89001/90017#newcode30
app/gfx/gl/gl_implementation_mac.cc:30:
"/System/Library/Frameworks/OpenGL.framework/Versions/Current/OpenGL",
My recollection is that it's nearly impossible to truly dynamically load the
OpenGL framework on Mac OS X. I think the reason is that it has tons of
dependencies which you would have to discover and manually load before loading
it. You will probably need to link against OpenGL.framework in addition to doing
this dlopen().

http://codereview.chromium.org/2134006/diff/89001/90017#newcode31
app/gfx/gl/gl_implementation_mac.cc:31: RTLD_LAZY | RTLD_GLOBAL);
Looking back at the GLEW sources, this used RTLD_LAZY | RTLD_LOCAL to avoid
namespace pollution.

[greggman, 2010-06-07 19:11:46]

LGTM

http://codereview.chromium.org/2134006/diff/42001/43004
File app/gfx/gl/gl_bindings.h (right):

http://codereview.chromium.org/2134006/diff/42001/43004#newcode73
app/gfx/gl/gl_bindings.h:73: void* GL_BINDING_CALL GetMockGLProcAddress(const
char* name);
On 2010/06/04 22:41:11, apatrick_chromium wrote:
> On 2010/06/03 01:45:06, greggman wrote:
> > Does this work when compiling tests on windows? Won't it make them all
WINAPI
> > calls?
> 
> The gpu_unittests project now registers a mock implementation of OpenGL that
> calls into the your gmock objects. This overrides any implementation
> InitializeOneOff attempts to register. Now we don't need to have a separation
> compilation of the service side code to run the unit tests and UNIT_TEST will
> never be defined. Now you can use GetGLImplementation() ==
kGLImplementationMock
> to do a runtime check if really necessary.
> 

I got that. What I was worried about is all the Mock functions are declared as
WINAPI. If that's not an issue than no problem.

[apatrick_chromium, 2010-06-07 19:34:39]

I uploaded again. I find the the "Delta from patch set" links do not work
reliably while the "Side-by-side diffs" links work okay.

[Ken Russell (switch to Gerrit), 2010-06-09 00:01:14]

LGTM

The elimination of the UNIT_TEST #define is great. I've been bitten by its
presence multiple times. Have the GPU unit tests been run on all platforms with
this change?

http://codereview.chromium.org/2134006/diff/146001/147039
File gpu/command_buffer/service/gl_utils.h (right):

http://codereview.chromium.org/2134006/diff/146001/147039#newcode16
gpu/command_buffer/service/gl_utils.h:16: #define GL_LOW_FLOAT                  
   0x8DF0
Do these #defines need to be guarded for the case where you're compiling against
a true GLES2 implementation?

[apatrick_chromium, 2010-06-09 01:06:43]

http://codereview.chromium.org/2134006/diff/89001/90003
File app/gfx/gl/generate_bindings.py (right):

http://codereview.chromium.org/2134006/diff/89001/90003#newcode413
app/gfx/gl/generate_bindings.py:413: # Write prototype for initilization
function.
On 2010/06/05 00:50:48, kbr wrote:
> initialization

Done.

http://codereview.chromium.org/2134006/diff/89001/90003#newcode467
app/gfx/gl/generate_bindings.py:467: file.write('void InitializeGLBindings%s()
{\n' % setName.upper())
I think it's expected for some functions to not be bound. I think they should be
tested for on an individual basis.

http://codereview.chromium.org/2134006/diff/89001/90016
File app/gfx/gl/gl_implementation_linux.cc (right):

http://codereview.chromium.org/2134006/diff/89001/90016#newcode23
app/gfx/gl/gl_implementation_linux.cc:23: // it might because the mock unit test
implementation was initialized and
On 2010/06/05 00:50:48, kbr wrote:
> There's something wrong with the grammar in this comment.
> 

Done.

http://codereview.chromium.org/2134006/diff/89001/90017
File app/gfx/gl/gl_implementation_mac.cc (right):

http://codereview.chromium.org/2134006/diff/89001/90017#newcode22
app/gfx/gl/gl_implementation_mac.cc:22: // it might because the mock unit test
implementation was initialized and
On 2010/06/05 00:50:48, kbr wrote:
> Grammar
> 

Done.

http://codereview.chromium.org/2134006/diff/89001/90017#newcode30
app/gfx/gl/gl_implementation_mac.cc:30:
"/System/Library/Frameworks/OpenGL.framework/Versions/Current/OpenGL",
It's still linking against OpenGL.framework. I only changed that for Windows and
Linux.

http://codereview.chromium.org/2134006/diff/89001/90017#newcode31
app/gfx/gl/gl_implementation_mac.cc:31: RTLD_LAZY | RTLD_GLOBAL);
On 2010/06/05 00:50:48, kbr wrote:
> Looking back at the GLEW sources, this used RTLD_LAZY | RTLD_LOCAL to avoid
> namespace pollution.
> 

Done.

http://codereview.chromium.org/2134006/diff/146001/147039
File gpu/command_buffer/service/gl_utils.h (right):

http://codereview.chromium.org/2134006/diff/146001/147039#newcode16
gpu/command_buffer/service/gl_utils.h:16: #define GL_LOW_FLOAT                  
   0x8DF0
The GPU service code is always compiled against the mesa GL headers, even if the
underlying GL is GLES2.

[apatrick, 2010-06-09 02:50:21]

My mac try job came so close to compiling :( I have run the gpu unit tests
on windows and linux and they work. I haven't tried them on mac. The trybots
run them on all platforms though.

On Tue, Jun 8, 2010 at 5:01 PM, <kbr@chromium.org> wrote:

> LGTM
>
> The elimination of the UNIT_TEST #define is great. I've been bitten by its
> presence multiple times. Have the GPU unit tests been run on all platforms
> with
> this change?
>
>
>
> http://codereview.chromium.org/2134006/diff/146001/147039
> File gpu/command_buffer/service/gl_utils.h (right):
>
> http://codereview.chromium.org/2134006/diff/146001/147039#newcode16
> gpu/command_buffer/service/gl_utils.h:16: #define GL_LOW_FLOAT
>           0x8DF0
> Do these #defines need to be guarded for the case where you're compiling
> against a true GLES2 implementation?
>
>
> http://codereview.chromium.org/2134006/show
>

[apatrick_chromium, 2010-06-09 23:17:39]

scherkus, this code landed with r49332 and your immediate mode GL code is now
commented out.