Take GL version and extensions correctly into account when binding functions

Take GL version and extensions correctly into account when binding functions

Platform libraries may return non-null function pointers from dlsym or
getProcAddress for unsupported GL functions. Previously the function
binding logic tried to address this with a separate extension function
binding step, but since some of the bindings were still done without
consulting the extension string, this failed to account for some
conflicts between core and extension functions such as
glDiscardFramebufferEXT vs. glInvalidateFramebuffer.

Fix this by binding all functions that have multiple versions only after
the context has been made current, and consulting the extension and
version strings. The logic for binding each function is now only in one
place in the generated code and easy to follow.

The patch still does not guarantee that the function pointers would be
set to null if the function is unsupported. It only attempts to guarantee
that an incorrect version of a function is not bound in case another
version is supported.

GetGLCoreProcAddress and GetGLProcAddress are combined into one function,
which always first looks for the function pointer with dlsym and then
with GetProcAddress. The new conditions for binding make sure that this
does not result in errors.

The patched bindings do not query for incorrect OES or ARB extension
strings without the GL_ prefix. Including the incorrect extension strings
without the prefix is assumed to have been a mistake. This applies to
OES_get_program_binary, OES_vertex_array_object, ARB_get_program_binary,
ARB_vertex_array_object, and APPLE_vertex_array_object.

BUG=322489

[oetuaho, 2013-12-03 13:44:50]

gpu_unittests, content_unittests and cc_unittests now passed on debug build on
Android and Linux, in addition to some manual testing with WebGL. Test runs on
other platforms are needed, and other feedback on the patch would be great.

[Ken Russell (switch to Gerrit), 2013-12-04 02:30:51]

Thanks for working on this. I definitely think this change should be made, as
you've found serious correctness issues in Chromium's GL bindings which are
causing real issues on current platforms.

Apologies, my review is incomplete, but CC'ing a couple of others who should
really review this. Some initial comments below. I'll be glad to help test on
other platforms; after reviewing the feedback below, please let me know whether
you think it's ready for that now, or you think another iteration is needed.

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

https://codereview.chromium.org/94963003/diff/60001/ui/gl/generate_bindings.p...
ui/gl/generate_bindings.py:506: # cause weirdness in the mock bindings and add
unnecessary overhead. Fix this.
Please keep in mind that it's recently been found that some devices perform very
poorly with the ES3 style multisampling provided by
glRenderbufferStorageMultisample and related functions, while
GL_EXT_multisampled_render_to_texture performs better. This informed a
distinction between the two in the GL bindings since the APIs are different.

https://codereview.chromium.org/94963003/diff/60001/ui/gl/generate_bindings.p...
ui/gl/generate_bindings.py:765: 'extension': 'APPLE_vertex_array_object' }],
One of these two extensions looks wrong; one has the GL_ prefix and the other
doesn't. Which is correct?

https://codereview.chromium.org/94963003/diff/60001/ui/gl/generate_bindings.p...
ui/gl/generate_bindings.py:803: 'gl_versions': ['es1', 'es2'] }],
Should this not be predicated on the extension GL_EXT_discard_framebuffer?

https://codereview.chromium.org/94963003/diff/60001/ui/gl/generate_bindings.p...
ui/gl/generate_bindings.py:1514: # TODO(oetuaho@nvidia.com): Get rid the
fallback.
This sounds like a big enough project that it warrants a formal follow-on bug
filed, and a reference in the code.

https://codereview.chromium.org/94963003/diff/60001/ui/gl/gl_implementation_m...
File ui/gl/gl_implementation_mac.cc (right):

https://codereview.chromium.org/94963003/diff/60001/ui/gl/gl_implementation_m...
ui/gl/gl_implementation_mac.cc:127: new GLContextMockVersion("3.0"));
Not sure 3.0 is a good default, given that Chromium currently relies on using
OpenGL 2.1 + extensions on desktop configurations. Work will need to be done to
upgrade to the 3.x core profile.

https://codereview.chromium.org/94963003/diff/60001/ui/gl/gl_implementation_o...
File ui/gl/gl_implementation_ozone.cc (right):

https://codereview.chromium.org/94963003/diff/60001/ui/gl/gl_implementation_o...
ui/gl/gl_implementation_ozone.cc:90: new GLContextMockVersion("4.0"));
Similar comment here and in later files.

[oetuaho, 2013-12-04 13:05:35]

Answers to review feedback inline, I'll be uploading a new version shortly.

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

https://codereview.chromium.org/94963003/diff/60001/ui/gl/generate_bindings.p...
ui/gl/generate_bindings.py:506: # cause weirdness in the mock bindings and add
unnecessary overhead. Fix this.
On 2013/12/04 02:30:51, Ken Russell wrote:
> Please keep in mind that it's recently been found that some devices perform
very
> poorly with the ES3 style multisampling provided by
> glRenderbufferStorageMultisample and related functions, while
> GL_EXT_multisampled_render_to_texture performs better. This informed a
> distinction between the two in the GL bindings since the APIs are different.

Okay, I was not aware of that. That does complicate combining the functions,
some might still be possible to combine but we'll have to be careful. I'll edit
the comment.

https://codereview.chromium.org/94963003/diff/60001/ui/gl/generate_bindings.p...
ui/gl/generate_bindings.py:765: 'extension': 'APPLE_vertex_array_object' }],
On 2013/12/04 02:30:51, Ken Russell wrote:
> One of these two extensions looks wrong; one has the GL_ prefix and the other
> doesn't. Which is correct?

You are right. It should be GL_APPLE_vertex_array_object, according to the spec.

https://codereview.chromium.org/94963003/diff/60001/ui/gl/generate_bindings.p...
ui/gl/generate_bindings.py:803: 'gl_versions': ['es1', 'es2'] }],
On 2013/12/04 02:30:51, Ken Russell wrote:
> Should this not be predicated on the extension GL_EXT_discard_framebuffer?

The extension is determined automatically based on the headers, as documented at
the top of the file.

https://codereview.chromium.org/94963003/diff/60001/ui/gl/generate_bindings.p...
ui/gl/generate_bindings.py:1514: # TODO(oetuaho@nvidia.com): Get rid the
fallback.
On 2013/12/04 02:30:51, Ken Russell wrote:
> This sounds like a big enough project that it warrants a formal follow-on bug
> filed, and a reference in the code.

Filed: http://code.google.com/p/chromium/issues/detail?id=325668

https://codereview.chromium.org/94963003/diff/60001/ui/gl/gl_implementation_m...
File ui/gl/gl_implementation_mac.cc (right):

https://codereview.chromium.org/94963003/diff/60001/ui/gl/gl_implementation_m...
ui/gl/gl_implementation_mac.cc:127: new GLContextMockVersion("3.0"));
On 2013/12/04 02:30:51, Ken Russell wrote:
> Not sure 3.0 is a good default, given that Chromium currently relies on using
> OpenGL 2.1 + extensions on desktop configurations. Work will need to be done
to
> upgrade to the 3.x core profile.

Since the unit tests don't specify an extension string for the bindings but some
bindings do require either an extension or GL 3.0+, putting anything less than
3.0 here could break some of the unit tests, or require lots of changes to the
unit tests. I do think that the unit tests could be set up better, but I'd say
that can go to a separate patch. I didn't have other very strong reasons to set
the 4.0/3.0 versions the way I did, though, I could make it 3.0 across the
board.

If Chrome requires OpenGL 2.1+, that could be explicitly specified when calling
glXCreateContextAttribsARB and wglCreateContextAttribsARB, by the way. It would
make it clearer what feature level is required.

[oetuaho, 2013-12-04 14:54:10]

I think the new patch set I uploaded is ready for testing on other platforms,
looking good on Android and Linux.

[piman, 2013-12-04 22:45:11]

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

https://codereview.chromium.org/94963003/diff/80001/ui/gl/generate_bindings.p...
ui/gl/generate_bindings.py:1520: file.write('  if (!fn.%sFn && (%s))\n  ' %
(known_as, cond))
How about '  else if (%s)\n  ' % cond ?
Presumably the previous line is an if, and is what may have set fn.*Fn

https://codereview.chromium.org/94963003/diff/80001/ui/gl/generate_bindings.p...
ui/gl/generate_bindings.py:1697: argRe = r'(const
)?[a-zA-Z0-9]+((\s*const\s*)?\*)* ([a-zA-Z0-9]+)'
nit: style (arg_re)

https://codereview.chromium.org/94963003/diff/80001/ui/gl/gl_context_mock_ver...
File ui/gl/gl_context_mock_version.cc (right):

https://codereview.chromium.org/94963003/diff/80001/ui/gl/gl_context_mock_ver...
ui/gl/gl_context_mock_version.cc:12: , version_str_(version_str)
nit: ',' on previous line.

https://codereview.chromium.org/94963003/diff/80001/ui/gl/gl_context_mock_ver...
File ui/gl/gl_context_mock_version.h (right):

https://codereview.chromium.org/94963003/diff/80001/ui/gl/gl_context_mock_ver...
ui/gl/gl_context_mock_version.h:20: virtual ~GLContextMockVersion();
nit: blank line before 'private:'

https://codereview.chromium.org/94963003/diff/80001/ui/gl/gl_version_info.cc
File ui/gl/gl_version_info.cc (right):

https://codereview.chromium.org/94963003/diff/80001/ui/gl/gl_version_info.cc#...
ui/gl/gl_version_info.cc:13: is_es1(false),
nit: align all fields:
GLVersionInfo::GLVersionInfo(const char* version_str)
    : is_es(false),
      is_es1(false),

etc.

https://codereview.chromium.org/94963003/diff/80001/ui/gl/gl_version_info.cc#...
ui/gl/gl_version_info.cc:20: {
nit: { on previous line

https://codereview.chromium.org/94963003/diff/80001/ui/gl/gl_version_info.h
File ui/gl/gl_version_info.h (right):

https://codereview.chromium.org/94963003/diff/80001/ui/gl/gl_version_info.h#n...
ui/gl/gl_version_info.h:14: GLVersionInfo(const char* version_str);
nit: explicit.

[oetuaho, 2013-12-05 09:47:14]

Responded to comments. I'll upload a patch set addressing the nits.

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

https://codereview.chromium.org/94963003/diff/80001/ui/gl/generate_bindings.p...
ui/gl/generate_bindings.py:1520: file.write('  if (!fn.%sFn && (%s))\n  ' %
(known_as, cond))
On 2013/12/04 22:45:12, piman wrote:
> How about '  else if (%s)\n  ' % cond ?
> Presumably the previous line is an if, and is what may have set fn.*Fn

That should be correct, and would result in cleaner generated code. Same goes
for the if under "elif not first_version". However, changing these does risk
breaking compatibility with subtly bugged drivers, for example if a driver
advertises GL_ARB_vertex_buffer_object but actually exports only glDeleteBuffers
and glGenBuffers without the ARB postfix, or something similar. I don't have a
very good idea if bugs like this appear in the wild, but we should be careful
here. I'm leaning on leaving this as is, but we can change it if you have more
data on this.

https://codereview.chromium.org/94963003/diff/80001/ui/gl/generate_bindings.p...
ui/gl/generate_bindings.py:1697: argRe = r'(const
)?[a-zA-Z0-9]+((\s*const\s*)?\*)* ([a-zA-Z0-9]+)'
On 2013/12/04 22:45:12, piman wrote:
> nit: style (arg_re)

Done.

https://codereview.chromium.org/94963003/diff/80001/ui/gl/gl_context_mock_ver...
File ui/gl/gl_context_mock_version.cc (right):

https://codereview.chromium.org/94963003/diff/80001/ui/gl/gl_context_mock_ver...
ui/gl/gl_context_mock_version.cc:12: , version_str_(version_str)
On 2013/12/04 22:45:12, piman wrote:
> nit: ',' on previous line.

Done.

https://codereview.chromium.org/94963003/diff/80001/ui/gl/gl_context_mock_ver...
File ui/gl/gl_context_mock_version.h (right):

https://codereview.chromium.org/94963003/diff/80001/ui/gl/gl_context_mock_ver...
ui/gl/gl_context_mock_version.h:20: virtual ~GLContextMockVersion();
On 2013/12/04 22:45:12, piman wrote:
> nit: blank line before 'private:'

Done.

https://codereview.chromium.org/94963003/diff/80001/ui/gl/gl_version_info.cc
File ui/gl/gl_version_info.cc (right):

https://codereview.chromium.org/94963003/diff/80001/ui/gl/gl_version_info.cc#...
ui/gl/gl_version_info.cc:13: is_es1(false),
On 2013/12/04 22:45:12, piman wrote:
> nit: align all fields:
> GLVersionInfo::GLVersionInfo(const char* version_str)
>     : is_es(false),
>       is_es1(false),
> 
> etc.

Done.

https://codereview.chromium.org/94963003/diff/80001/ui/gl/gl_version_info.cc#...
ui/gl/gl_version_info.cc:20: {
On 2013/12/04 22:45:12, piman wrote:
> nit: { on previous line

Done.

https://codereview.chromium.org/94963003/diff/80001/ui/gl/gl_version_info.h
File ui/gl/gl_version_info.h (right):

https://codereview.chromium.org/94963003/diff/80001/ui/gl/gl_version_info.h#n...
ui/gl/gl_version_info.h:14: GLVersionInfo(const char* version_str);
On 2013/12/04 22:45:12, piman wrote:
> nit: explicit.

Done.

[piman, 2013-12-05 19:47:03]

LGTM, thanks.

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

https://codereview.chromium.org/94963003/diff/80001/ui/gl/generate_bindings.p...
ui/gl/generate_bindings.py:1520: file.write('  if (!fn.%sFn && (%s))\n  ' %
(known_as, cond))
On 2013/12/05 09:47:14, oetuaho wrote:
> On 2013/12/04 22:45:12, piman wrote:
> > How about '  else if (%s)\n  ' % cond ?
> > Presumably the previous line is an if, and is what may have set fn.*Fn
> 
> That should be correct, and would result in cleaner generated code. Same goes
> for the if under "elif not first_version". However, changing these does risk
> breaking compatibility with subtly bugged drivers, for example if a driver
> advertises GL_ARB_vertex_buffer_object but actually exports only
glDeleteBuffers
> and glGenBuffers without the ARB postfix, or something similar. I don't have a
> very good idea if bugs like this appear in the wild, but we should be careful
> here. I'm leaning on leaving this as is, but we can change it if you have more
> data on this.

I see, you want to protect against buggy drivers that expose an extension, but
don't provide the entrypoints, givving them fallback options. I buy that.

[no sievers, 2013-12-05 20:51:56]

This is great, thanks for doing this.

https://codereview.chromium.org/94963003/diff/90001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right):

https://codereview.chromium.org/94963003/diff/90001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:4875:
RenderbufferStorageMultisample(GL_RENDERBUFFER,
Here we'd still want RenderbufferStorageMultisampleEXT for the specific mobile
render_to_texture extension.

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

https://codereview.chromium.org/94963003/diff/90001/ui/gl/generate_bindings.p...
ui/gl/generate_bindings.py:20: 
Can you explain |known_as| here also somewhere?

https://codereview.chromium.org/94963003/diff/90001/ui/gl/generate_bindings.p...
ui/gl/generate_bindings.py:509: { 'return_type': 'void',
I think you can define two entry points only:

One that is known as 'glRenderbufferStorageMultisample' and has versions in the
blit_framebuffer desktop extensions, the angle extension, and es3 (and desktop
gl4 i think). 

Then one that is glRenderbufferStorageMultisampleSOMETHING and has two versions
and only on es: GL_EXT/IMG_render_to_texture_multisampled

Then simplify the code in gles2_cmd_decoder.cc and remove the TODO(sievers) :-)

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_bindings.h
File ui/gl/gl_bindings.h (right):

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_bindings.h#newco...
ui/gl/gl_bindings.h:229: void InitializeDynamic(GLContext* context);
nit: InitializeDynamicBindings()

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_context.h
File ui/gl/gl_context.h (right):

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_context.h#newcod...
ui/gl/gl_context.h:121: bool InitializeDynamicBindings();
Is a call to this missing in GLContextNSView::MakeCurrent()?

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_implementation.h
File ui/gl/gl_implementation.h (right):

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_implementation.h...
ui/gl/gl_implementation.h:85: // querying functions from the EGL library
supplied by Android, which contains
Well, it's not really garbage it returns on Android but the correct entry point,
since we always bind again libGLESv2.so (and not the driver). If the driver does
not have the entry point and you call into the platform library it will log a
warning that the function is unsupported.

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_implementation_w...
File ui/gl/gl_implementation_win.cc (right):

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_implementation_w...
ui/gl/gl_implementation_win.cc:249: // Windows GetProcAddress.
This comment doesn't seem right. Fore both core and extension bindings, it seems
to do the opposite before and after your patch. It's also a comment about the
internals of another function (InitializeStaticGLBindingsGL()).

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_surface_wgl.cc
File ui/gl/gl_surface_wgl.cc (right):

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_surface_wgl.cc#n...
ui/gl/gl_surface_wgl.cc:171: HGLRC gl_context =
wglCreateContext(wgl_display->device_context());
Should this move into gl_implementation_win.cc:InitializeStaticGLBindings()?

InitializeStaticGLBindings() seems flaky otherwise (well it was before your
patch, so feel free to put a TODO and DCHECK() for a current context).

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_version_info.cc
File ui/gl/gl_version_info.cc (right):

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_version_info.cc#...
ui/gl/gl_version_info.cc:27: 
nit: if (!is_es) { ...

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_version_info.h
File ui/gl/gl_version_info.h (right):

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_version_info.h#n...
ui/gl/gl_version_info.h:5: #ifndef UI_GL_VERSION_INFO_H_
UI_GL_GL_VERSION_INFO_H_

[no sievers, 2013-12-05 21:07:16]

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_context.h
File ui/gl/gl_context.h (right):

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_context.h#newcod...
ui/gl/gl_context.h:121: bool InitializeDynamicBindings();
On 2013/12/05 20:51:57, sievers wrote:
> Is a call to this missing in GLContextNSView::MakeCurrent()?

I also suggest that we initialize the dynamic bindings more explicitly since
doing it this way in GLContext is just flaky. In gpu_main.cc and
gpu_child_thread.cc we already make a context current for most cases (see
gpu_main.cc and CollectContextGraphicsInfo(), and for Windows we actually need
the context current already for binding core functions, it seems). We could
maybe also do it in GpuChildThread::OnInitialize().

It has nothing to do with your patch directly though and we can do it as a
follow-up.

[no sievers, 2013-12-05 21:11:03]

On 2013/12/05 21:07:16, sievers wrote:
> https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_context.h
> File ui/gl/gl_context.h (right):
> 
>
https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_context.h#newcod...
> ui/gl/gl_context.h:121: bool InitializeDynamicBindings();
> On 2013/12/05 20:51:57, sievers wrote:
> > Is a call to this missing in GLContextNSView::MakeCurrent()?
> 
> I also suggest that we initialize the dynamic bindings more explicitly since
> doing it this way in GLContext is just flaky. In gpu_main.cc and
> gpu_child_thread.cc we already make a context current for most cases (see
> gpu_main.cc and CollectContextGraphicsInfo(), and for Windows we actually need
> the context current already for binding core functions, it seems). We could
> maybe also do it in GpuChildThread::OnInitialize().
> 
> It has nothing to do with your patch directly though and we can do it as a
> follow-up.

Actually, it *might* be better to make it more deterministic now, since your
patches changes the behavior in that bindings with multiple versions could be
NULL now until we make a context current, right? This could cause problems
somewhere we we end up calling into one of those too early.

[oetuaho, 2013-12-09 17:17:06]

I don't think that we should cram overhauling the whole context creation into
this patch set, though I'd like to see it done. If gl functions would be called
without a current context, I would expect that clear symptoms would have been
visible also before this patch set. But as a risk-reducing measure, maybe we
could initially fill the bindings with an error log function, if just having
null function pointers before they're properly initialized worries you too much.

I've fixed the minor issues you pointed out locally now, taking care of the
Windows initialization and possibly the unit tests is going to take some time.

https://codereview.chromium.org/94963003/diff/90001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right):

https://codereview.chromium.org/94963003/diff/90001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:4875:
RenderbufferStorageMultisample(GL_RENDERBUFFER,
On 2013/12/05 20:51:57, sievers wrote:
> Here we'd still want RenderbufferStorageMultisampleEXT for the specific mobile
> render_to_texture extension.

The root problem here is really that the unit tests don't initialize the
bindings with a proper extension string. If that would be fixed,
RenderbufferStorageMultisampleEXT would be called here. I think I'll try to see
if re-initializing the bindings in GLES2DecoderTestWithExtensions::SetUp could
work.

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

https://codereview.chromium.org/94963003/diff/90001/ui/gl/generate_bindings.p...
ui/gl/generate_bindings.py:20: 
On 2013/12/05 20:51:57, sievers wrote:
> Can you explain |known_as| here also somewhere?

Done.

https://codereview.chromium.org/94963003/diff/90001/ui/gl/generate_bindings.p...
ui/gl/generate_bindings.py:509: { 'return_type': 'void',
On 2013/12/05 20:51:57, sievers wrote:
> I think you can define two entry points only:
> 
> One that is known as 'glRenderbufferStorageMultisample' and has versions in
the
> blit_framebuffer desktop extensions, the angle extension, and es3 (and desktop
> gl4 i think). 
> 
> Then one that is glRenderbufferStorageMultisampleSOMETHING and has two
versions
> and only on es: GL_EXT/IMG_render_to_texture_multisampled
> 
> Then simplify the code in gles2_cmd_decoder.cc and remove the TODO(sievers)
:-)

Is it okay with you if we leave this up to a follow-up patch?

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_bindings.h
File ui/gl/gl_bindings.h (right):

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_bindings.h#newco...
ui/gl/gl_bindings.h:229: void InitializeDynamic(GLContext* context);
On 2013/12/05 20:51:57, sievers wrote:
> nit: InitializeDynamicBindings()

This is intentional - the InitializeDynamicBindings is private here, like
InitializeExtensionBindings was before, since the initialization needs to do
some additional work on top of the auto-generated code. But InitializeDynamic is
a bit weird as a name, so I think I'll change it to
InitializeCustomDynamicBindings which communicates what it does a bit better.

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_context.h
File ui/gl/gl_context.h (right):

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_context.h#newcod...
ui/gl/gl_context.h:121: bool InitializeDynamicBindings();
On 2013/12/05 21:07:17, sievers wrote:
> On 2013/12/05 20:51:57, sievers wrote:
> > Is a call to this missing in GLContextNSView::MakeCurrent()?
> 
> I also suggest that we initialize the dynamic bindings more explicitly since
> doing it this way in GLContext is just flaky. In gpu_main.cc and
> gpu_child_thread.cc we already make a context current for most cases (see
> gpu_main.cc and CollectContextGraphicsInfo(), and for Windows we actually need
> the context current already for binding core functions, it seems). We could
> maybe also do it in GpuChildThread::OnInitialize().
> 
> It has nothing to do with your patch directly though and we can do it as a
> follow-up.

Yeah, after seeing the gpu_main.cc code it looks like the context could just
always be initialized from there after minimal bindings needed for that are
done, so some of this complexity would be avoided.

You're right about the GLContextNSView, it looks like a call needs to be added.

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_implementation.h
File ui/gl/gl_implementation.h (right):

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_implementation.h...
ui/gl/gl_implementation.h:85: // querying functions from the EGL library
supplied by Android, which contains
On 2013/12/05 20:51:57, sievers wrote:
> Well, it's not really garbage it returns on Android but the correct entry
point,
> since we always bind again libGLESv2.so (and not the driver). If the driver
does
> not have the entry point and you call into the platform library it will log a
> warning that the function is unsupported.

Yeah, I'll re-word the comment to make this clear.

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_implementation_w...
File ui/gl/gl_implementation_win.cc (right):

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_implementation_w...
ui/gl/gl_implementation_win.cc:249: // Windows GetProcAddress.
On 2013/12/05 20:51:57, sievers wrote:
> This comment doesn't seem right. Fore both core and extension bindings, it
seems
> to do the opposite before and after your patch. It's also a comment about the
> internals of another function (InitializeStaticGLBindingsGL()).
> 

Yep, I'll just remove the comment, it has no reason to be here.

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_surface_wgl.cc
File ui/gl/gl_surface_wgl.cc (right):

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_surface_wgl.cc#n...
ui/gl/gl_surface_wgl.cc:171: HGLRC gl_context =
wglCreateContext(wgl_display->device_context());
On 2013/12/05 20:51:57, sievers wrote:
> Should this move into gl_implementation_win.cc:InitializeStaticGLBindings()?
> 
> InitializeStaticGLBindings() seems flaky otherwise (well it was before your
> patch, so feel free to put a TODO and DCHECK() for a current context).

Yeah, this needs a fix. GLSurface::InitializeOneOff calls first into
InitializeStaticGLBindings without a current context. It should be relatively
safe according to MSDN documentation, which says that wglGetProcAddress should
return NULL if there's no current context, so only OpenGL 1.1 entry points
exported by the dll should be set, and the remaining entry points would be set
here. But http://www.opengl.org/wiki/Load_OpenGL_Functions warns that actual
implementations sometimes return numbers from -1 to 3 on errors.

After finding out this, I think there's no real alternative to moving the code
over to gl_implementation_win.cc::InitializeStaticGLBindings, so I'll do that.

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_version_info.cc
File ui/gl/gl_version_info.cc (right):

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_version_info.cc#...
ui/gl/gl_version_info.cc:27: 
On 2013/12/05 20:51:57, sievers wrote:
> nit: if (!is_es) { ...

I'll restructure this a bit.

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_version_info.h
File ui/gl/gl_version_info.h (right):

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_version_info.h#n...
ui/gl/gl_version_info.h:5: #ifndef UI_GL_VERSION_INFO_H_
On 2013/12/05 20:51:57, sievers wrote:
> UI_GL_GL_VERSION_INFO_H_

Done. There seem to be a lot of missing GL_'s in existing code too, like in all
the gl_*_api_implementation.h files. Should I fix those too?

[no sievers, 2013-12-09 20:05:23]

> I don't think that we should cram overhauling the whole context creation into
this patch set, 

Yea, we can follow up on that later. I'd expect there to be enough test coverage
on the waterfalls to detect if we are missing to init the dynamic bindings
anywhere. The idea of initializing function pointers with a stub (and maybe
NOTREACHED()) sounds very neat though.

https://codereview.chromium.org/94963003/diff/90001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right):

https://codereview.chromium.org/94963003/diff/90001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:4875:
RenderbufferStorageMultisample(GL_RENDERBUFFER,
On 2013/12/09 17:17:07, oetuaho wrote:
> On 2013/12/05 20:51:57, sievers wrote:
> > Here we'd still want RenderbufferStorageMultisampleEXT for the specific
mobile
> > render_to_texture extension.
> 
> The root problem here is really that the unit tests don't initialize the
> bindings with a proper extension string. If that would be fixed,
> RenderbufferStorageMultisampleEXT would be called here. I think I'll try to
see
> if re-initializing the bindings in GLES2DecoderTestWithExtensions::SetUp could
> work.

Or maybe to make it simpler just initialize it with all extensions in
gpu/command_buffer/common/unittest_main.cc. That would be compatible with what
happens now, and I think good enough since at this level we don't really test
anything about the bindings but rather mock all of them. That means that we also
fail tests if we get unexpected mock calls.

You could use the mock version/extension trick you do in the other places. Maybe
the python script could generate a constant string with all known extensions for
this purpose?

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

https://codereview.chromium.org/94963003/diff/90001/ui/gl/generate_bindings.p...
ui/gl/generate_bindings.py:509: { 'return_type': 'void',
On 2013/12/09 17:17:07, oetuaho wrote:
> On 2013/12/05 20:51:57, sievers wrote:
> > I think you can define two entry points only:
> > 
> > One that is known as 'glRenderbufferStorageMultisample' and has versions in
> the
> > blit_framebuffer desktop extensions, the angle extension, and es3 (and
desktop
> > gl4 i think). 
> > 
> > Then one that is glRenderbufferStorageMultisampleSOMETHING and has two
> versions
> > and only on es: GL_EXT/IMG_render_to_texture_multisampled
> > 
> > Then simplify the code in gles2_cmd_decoder.cc and remove the TODO(sievers)
> :-)
> 
> Is it okay with you if we leave this up to a follow-up patch?

Yes, absolutely, I can do this later. Was only mentioning in case it helps with
the binding issue in the decoder unittest.

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_version_info.h
File ui/gl/gl_version_info.h (right):

https://codereview.chromium.org/94963003/diff/90001/ui/gl/gl_version_info.h#n...
ui/gl/gl_version_info.h:5: #ifndef UI_GL_VERSION_INFO_H_
On 2013/12/09 17:17:07, oetuaho wrote:
> On 2013/12/05 20:51:57, sievers wrote:
> > UI_GL_GL_VERSION_INFO_H_
> 
> Done. There seem to be a lot of missing GL_'s in existing code too, like in
all
> the gl_*_api_implementation.h files. Should I fix those too?

Ooops, let's do the other files in a separate patch then.

[Ken Russell (switch to Gerrit), 2013-12-10 00:07:06]

Daniel, thanks for your careful review of this patch.

This looks good. Only one issue I feel needs to be addressed, below.

One caution about introducing stub functions which either log or contain
NOTREACHED: in early Chrome on Android builds I saw problems where the stack
would be corrupted on ARM because the caller and callee had inconsistent notions
about how many arguments the function had, and the OpenGL implementation itself
was returning stub functions (taking no arguments) for each of the unimplemented
entry points. It would be better to crash on a null pointer dereference than to
corrupt the stack in weird ways.

https://codereview.chromium.org/94963003/diff/90001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right):

https://codereview.chromium.org/94963003/diff/90001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:4801:
RenderbufferStorageMultisample(GL_RENDERBUFFER,
This change is problematic. This test explicitly tries to initialize with the
GL_EXT_framebuffer_multisample extension. A fix is needed for this issue.

https://codereview.chromium.org/94963003/diff/90001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:4875:
RenderbufferStorageMultisample(GL_RENDERBUFFER,
On 2013/12/09 20:05:24, sievers wrote:
> On 2013/12/09 17:17:07, oetuaho wrote:
> > On 2013/12/05 20:51:57, sievers wrote:
> > > Here we'd still want RenderbufferStorageMultisampleEXT for the specific
> mobile
> > > render_to_texture extension.
> > 
> > The root problem here is really that the unit tests don't initialize the
> > bindings with a proper extension string. If that would be fixed,
> > RenderbufferStorageMultisampleEXT would be called here. I think I'll try to
> see
> > if re-initializing the bindings in GLES2DecoderTestWithExtensions::SetUp
could
> > work.
> 
> Or maybe to make it simpler just initialize it with all extensions in
> gpu/command_buffer/common/unittest_main.cc. That would be compatible with what
> happens now, and I think good enough since at this level we don't really test
> anything about the bindings but rather mock all of them. That means that we
also
> fail tests if we get unexpected mock calls.
> 
> You could use the mock version/extension trick you do in the other places.
Maybe
> the python script could generate a constant string with all known extensions
for
> this purpose?

There are attempts in other tests, like
GLES2DecoderManualInitTest::RenderbufferStorageMultisampleCHROMIUM, above, to
initialize the decoder with an appropriate extension string. Why isn't that
working any more?

The change of which function is called here and above -- from
RenderbufferStorageMultisampleEXT to RenderbufferStorageMultisample -- is really
problematic and needs to be resolved before this patch can land. These
multisampling APIs have very different semantics.

[oetuaho, 2013-12-10 10:37:56]

On 2013/12/10 00:07:06, Ken Russell wrote:
> Daniel, thanks for your careful review of this patch.
> 
> This looks good. Only one issue I feel needs to be addressed, below.
> 
> One caution about introducing stub functions which either log or contain
> NOTREACHED: in early Chrome on Android builds I saw problems where the stack
> would be corrupted on ARM because the caller and callee had inconsistent
notions
> about how many arguments the function had, and the OpenGL implementation
itself
> was returning stub functions (taking no arguments) for each of the
unimplemented
> entry points. It would be better to crash on a null pointer dereference than
to
> corrupt the stack in weird ways.
> 

Thanks for the comment, I was not aware of that. The stub functions could be
generated with the right parameters so the stack corruption would not be an
issue. I'll implement that for the next patch set.

[oetuaho, 2013-12-10 10:39:04]

https://codereview.chromium.org/94963003/diff/90001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right):

https://codereview.chromium.org/94963003/diff/90001/gpu/command_buffer/servic...
gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:4875:
RenderbufferStorageMultisample(GL_RENDERBUFFER,
On 2013/12/10 00:07:07, Ken Russell wrote:
> On 2013/12/09 20:05:24, sievers wrote:
> > On 2013/12/09 17:17:07, oetuaho wrote:
> > > On 2013/12/05 20:51:57, sievers wrote:
> > > > Here we'd still want RenderbufferStorageMultisampleEXT for the specific
> > mobile
> > > > render_to_texture extension.
> > > 
> > > The root problem here is really that the unit tests don't initialize the
> > > bindings with a proper extension string. If that would be fixed,
> > > RenderbufferStorageMultisampleEXT would be called here. I think I'll try
to
> > see
> > > if re-initializing the bindings in GLES2DecoderTestWithExtensions::SetUp
> could
> > > work.
> > 
> > Or maybe to make it simpler just initialize it with all extensions in
> > gpu/command_buffer/common/unittest_main.cc. That would be compatible with
what
> > happens now, and I think good enough since at this level we don't really
test
> > anything about the bindings but rather mock all of them. That means that we
> also
> > fail tests if we get unexpected mock calls.
> > 
> > You could use the mock version/extension trick you do in the other places.
> Maybe
> > the python script could generate a constant string with all known extensions
> for
> > this purpose?
> 
> There are attempts in other tests, like
> GLES2DecoderManualInitTest::RenderbufferStorageMultisampleCHROMIUM, above, to
> initialize the decoder with an appropriate extension string. Why isn't that
> working any more?
> 
> The change of which function is called here and above -- from
> RenderbufferStorageMultisampleEXT to RenderbufferStorageMultisample -- is
really
> problematic and needs to be resolved before this patch can land. These
> multisampling APIs have very different semantics.

Yes, the decoder is initialized with the appropriate extension string in the
tests, but the bindings are not. I do intend to fix this.

Ah, and now I think I get the semantic difference. So to clarify, this binding
is a different function depending on whether we're using ES or desktop GL, since
in desktop GL RenderbufferStorageMultisample !=
RenderbufferStorageMultisampleEXT, but in ES RenderbufferStorageMultisample ==
RenderbufferStorageMultisampleEXT == RenderbufferStorageMultisampleIMG. In ES
world this fallback actually looks correct to me (this line), but in the desktop
world it is definitely not correct (the case above Ken commented on). That the
fallback exists in the bindings in the first place is wrong from the desktop
point of view, so it could use some further cleanup, but for now it should be
enough to fix the unit tests so that the bindings are done as expected.

And I don't think that initializing the unit test bindings once with all known
extensions is good enough, since the extensions can be in conflict with each
other like this. They need to be reinitialized per test.

[oetuaho, 2013-12-10 17:29:44]

I now uploaded a patch set that takes care of all but the Windows issues, that
is going to take some time since I still haven't gotten all the necessary setup
for Windows development done. I made it so that the unit tests initialize the
bindings with the extension string in the test and made it possible to specify
the gl version used for the bindings as well. The tests are using gl version
"3.0" unless I found a reason to use something else, like if the test uses an
extension that's only for GLES.

The stub function generation has also been added to generate_bindings.py, the
stubs have the correct signature, they contain NOTREACHED() and return 0 if they
need a return value.

[no sievers, 2013-12-10 18:30:17]

On 2013/12/10 10:39:04, oetuaho wrote:
>
https://codereview.chromium.org/94963003/diff/90001/gpu/command_buffer/servic...
> File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right):
> 
>
https://codereview.chromium.org/94963003/diff/90001/gpu/command_buffer/servic...
> gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:4875:
> RenderbufferStorageMultisample(GL_RENDERBUFFER,
> On 2013/12/10 00:07:07, Ken Russell wrote:
> > On 2013/12/09 20:05:24, sievers wrote:
> > > On 2013/12/09 17:17:07, oetuaho wrote:
> > > > On 2013/12/05 20:51:57, sievers wrote:
> > > > > Here we'd still want RenderbufferStorageMultisampleEXT for the
specific
> > > mobile
> > > > > render_to_texture extension.
> > > > 
> > > > The root problem here is really that the unit tests don't initialize the
> > > > bindings with a proper extension string. If that would be fixed,
> > > > RenderbufferStorageMultisampleEXT would be called here. I think I'll try
> to
> > > see
> > > > if re-initializing the bindings in GLES2DecoderTestWithExtensions::SetUp
> > could
> > > > work.
> > > 
> > > Or maybe to make it simpler just initialize it with all extensions in
> > > gpu/command_buffer/common/unittest_main.cc. That would be compatible with
> what
> > > happens now, and I think good enough since at this level we don't really
> test
> > > anything about the bindings but rather mock all of them. That means that
we
> > also
> > > fail tests if we get unexpected mock calls.
> > > 
> > > You could use the mock version/extension trick you do in the other places.
> > Maybe
> > > the python script could generate a constant string with all known
extensions
> > for
> > > this purpose?
> > 
> > There are attempts in other tests, like
> > GLES2DecoderManualInitTest::RenderbufferStorageMultisampleCHROMIUM, above,
to
> > initialize the decoder with an appropriate extension string. Why isn't that
> > working any more?
> > 
> > The change of which function is called here and above -- from
> > RenderbufferStorageMultisampleEXT to RenderbufferStorageMultisample -- is
> really
> > problematic and needs to be resolved before this patch can land. These
> > multisampling APIs have very different semantics.
> 
> Yes, the decoder is initialized with the appropriate extension string in the
> tests, but the bindings are not. I do intend to fix this.
> 
> Ah, and now I think I get the semantic difference. So to clarify, this binding
> is a different function depending on whether we're using ES or desktop GL,
since
> in desktop GL RenderbufferStorageMultisample !=
> RenderbufferStorageMultisampleEXT, but in ES RenderbufferStorageMultisample ==
> RenderbufferStorageMultisampleEXT == RenderbufferStorageMultisampleIMG. In ES
> world this fallback actually looks correct to me (this line), but in the
desktop
> world it is definitely not correct (the case above Ken commented on). That the
> fallback exists in the bindings in the first place is wrong from the desktop
> point of view, so it could use some further cleanup, but for now it should be
> enough to fix the unit tests so that the bindings are done as expected.
> 

Also keep in mind the case of is_es3 and has GL_EXT(or
IMG)_render_to_texture_multisampled.
We want to be careful there and use the glRenderbufferStorageMultisample()
symbol for es3 msaa and glRenderbufferStorageMultisampleEXT/IMG() for the
extension, because they *might* be different functions in the driver (they are
different stubs in libGLESv2).



> And I don't think that initializing the unit test bindings once with all known
> extensions is good enough, since the extensions can be in conflict with each
> other like this. They need to be reinitialized per test.

[no sievers, 2013-12-10 18:33:40]

On 2013/12/10 10:39:04, oetuaho wrote:
> And I don't think that initializing the unit test bindings once with all known
> extensions is good enough, since the extensions can be in conflict with each
> other like this. They need to be reinitialized per test.

I think if they are in conflict with each other they should not use a common
|known_as| function name.
The stubs in our API binding should only be shared if they are compatible (they
can map to different driver entry points though).
Since the unit tests mock at the GL bindings level, do we really expect
conflicts or wouldn't that be a problem?

[oetuaho, 2013-12-10 19:12:45]

Re GL_EXT / ES3 core vs. GL_IMG, what I've understood is that for
glRenderbufferStorageMultisample, there can be a performance difference but no
semantic difference in the specs, which only differ in the sections discussing
multisampling for texture attachments. Or is there something I've missed?

On 2013/12/10 18:33:40, sievers wrote:
> On 2013/12/10 10:39:04, oetuaho wrote:
> > And I don't think that initializing the unit test bindings once with all
known
> > extensions is good enough, since the extensions can be in conflict with each
> > other like this. They need to be reinitialized per test.
> 
> I think if they are in conflict with each other they should not use a common
> |known_as| function name.
> The stubs in our API binding should only be shared if they are compatible
(they
> can map to different driver entry points though).
> Since the unit tests mock at the GL bindings level, do we really expect
> conflicts or wouldn't that be a problem?

I agree that they shouldn't share the same known_as name, but I still think that
belongs to a follow-up patch as we agreed earlier.

And even if that would be in place, feeding all the possible extensions to the
initialization of the test bindings seems hacky to me. That would just move the
mock OpenGL in the tests further away from a real OpenGL implementation. By
reinitializing the bindings for each test, we get better test coverage for the
binding conditions, and gain the ability to target new tests very specifically
for a given GL configuration. The reinitialization removes the need to consider
any possible quirks in the test bindings, it's less complex and not costly in
terms of performance either.

[no sievers, 2013-12-10 19:29:33]

On 2013/12/10 19:12:45, oetuaho wrote:
> Re GL_EXT / ES3 core vs. GL_IMG, what I've understood is that for
> glRenderbufferStorageMultisample, there can be a performance difference but no
> semantic difference in the specs, which only differ in the sections discussing
> multisampling for texture attachments. Or is there something I've missed?
> 

They are semantically different. One has an explicit resolve step (through
glBlitFramebuffer()), the other one (render_to_texture_multisampled) resolves
the texture implicitly.

The Android framework library provides separate entry points:
glRenderbufferStorageMultisample() and glRenderbufferStorageMultisampleEXT
(probably also IMG). Calling glRenderbufferStorageMultisampleEXT() on an es3
driver that does not advertise render_to_texture_multisampled will not end up
calling into the driver (but log a warning that you are calling an unimplemented
function) from what I saw.

Similarly, if the driver does have render_to_texture_multisampled (and is ES3
3.0) using the glRenderbufferStorageMultisampleEXT() when you are later
attempting to explicitly resolve the framebuffer with glBlitFramebuffer() might
be a problem if this is mapped to a different incompatible function in the
driver than core glRenderbufferStorageMultisample().


> On 2013/12/10 18:33:40, sievers wrote:
> > On 2013/12/10 10:39:04, oetuaho wrote:
> > > And I don't think that initializing the unit test bindings once with all
> known
> > > extensions is good enough, since the extensions can be in conflict with
each
> > > other like this. They need to be reinitialized per test.
> > 
> > I think if they are in conflict with each other they should not use a common
> > |known_as| function name.
> > The stubs in our API binding should only be shared if they are compatible
> (they
> > can map to different driver entry points though).
> > Since the unit tests mock at the GL bindings level, do we really expect
> > conflicts or wouldn't that be a problem?
> 
> I agree that they shouldn't share the same known_as name, but I still think
that
> belongs to a follow-up patch as we agreed earlier.
> 
> And even if that would be in place, feeding all the possible extensions to the
> initialization of the test bindings seems hacky to me. That would just move
the
> mock OpenGL in the tests further away from a real OpenGL implementation. By
> reinitializing the bindings for each test, we get better test coverage for the
> binding conditions, and gain the ability to target new tests very specifically
> for a given GL configuration. The reinitialization removes the need to
consider
> any possible quirks in the test bindings, it's less complex and not costly in
> terms of performance either.

[Ken Russell (switch to Gerrit), 2013-12-10 19:53:49]

On 2013/12/10 19:29:33, sievers wrote:
> On 2013/12/10 19:12:45, oetuaho wrote:
> > Re GL_EXT / ES3 core vs. GL_IMG, what I've understood is that for
> > glRenderbufferStorageMultisample, there can be a performance difference but
no
> > semantic difference in the specs, which only differ in the sections
discussing
> > multisampling for texture attachments. Or is there something I've missed?
> > 
> 
> They are semantically different. One has an explicit resolve step (through
> glBlitFramebuffer()), the other one (render_to_texture_multisampled) resolves
> the texture implicitly.
> 
> The Android framework library provides separate entry points:
> glRenderbufferStorageMultisample() and glRenderbufferStorageMultisampleEXT
> (probably also IMG). Calling glRenderbufferStorageMultisampleEXT() on an es3
> driver that does not advertise render_to_texture_multisampled will not end up
> calling into the driver (but log a warning that you are calling an
unimplemented
> function) from what I saw.
> 
> Similarly, if the driver does have render_to_texture_multisampled (and is ES3
> 3.0) using the glRenderbufferStorageMultisampleEXT() when you are later
> attempting to explicitly resolve the framebuffer with glBlitFramebuffer()
might
> be a problem if this is mapped to a different incompatible function in the
> driver than core glRenderbufferStorageMultisample().

To reiterate:

EXT_multisampled_render_to_texture and IMG_multisampled_render_to_texture are
different than ES3 core's multisampling.

The EXT_ and IMG_ versions do not have an explicit resolve step. ES3's
multisampling does.

glRenderbufferStorageMultisampleEXT and glRenderbufferStorageMultisampleIMG have
similar semantics, though it looks like they use different enums.

glRenderbufferStorageMultisample is only provided by ES3 core. It must be used
in conjunction with glBlitFramebuffer.

The EXT_ version, and what's in ES3 core, must be kept distinct in the bindings.
I think it may be a good idea to keep the IMG_ version separate too simply
because it uses a different set of enums.


> > On 2013/12/10 18:33:40, sievers wrote:
> > > On 2013/12/10 10:39:04, oetuaho wrote:
> > > > And I don't think that initializing the unit test bindings once with all
> > known
> > > > extensions is good enough, since the extensions can be in conflict with
> each
> > > > other like this. They need to be reinitialized per test.
> > > 
> > > I think if they are in conflict with each other they should not use a
common
> > > |known_as| function name.
> > > The stubs in our API binding should only be shared if they are compatible
> > (they
> > > can map to different driver entry points though).
> > > Since the unit tests mock at the GL bindings level, do we really expect
> > > conflicts or wouldn't that be a problem?
> > 
> > I agree that they shouldn't share the same known_as name, but I still think
> that
> > belongs to a follow-up patch as we agreed earlier.
> > 
> > And even if that would be in place, feeding all the possible extensions to
the
> > initialization of the test bindings seems hacky to me. That would just move
> the
> > mock OpenGL in the tests further away from a real OpenGL implementation. By
> > reinitializing the bindings for each test, we get better test coverage for
the
> > binding conditions, and gain the ability to target new tests very
specifically
> > for a given GL configuration. The reinitialization removes the need to
> consider
> > any possible quirks in the test bindings, it's less complex and not costly
in
> > terms of performance either.

[oetuaho, 2013-12-12 16:42:09]

On 2013/12/10 19:53:49, Ken Russell wrote:
> To reiterate:
> 
> EXT_multisampled_render_to_texture and IMG_multisampled_render_to_texture are
> different than ES3 core's multisampling.
> 
> The EXT_ and IMG_ versions do not have an explicit resolve step. ES3's
> multisampling does.
> 
> glRenderbufferStorageMultisampleEXT and glRenderbufferStorageMultisampleIMG
have
> similar semantics, though it looks like they use different enums.
> 
> glRenderbufferStorageMultisample is only provided by ES3 core. It must be used
> in conjunction with glBlitFramebuffer.
> 
> The EXT_ version, and what's in ES3 core, must be kept distinct in the
bindings.
> I think it may be a good idea to keep the IMG_ version separate too simply
> because it uses a different set of enums.
> 

Thanks for patiently clarifying this for me, I should have read the specs more
carefully or consulted my colleagues more familiar with this part of the specs.
I still don't have very wide experience with other GL versions than GLES2, so I
hope you understand.

Looks like we can't really combine any of these together then, rather we should
ideally just split the ES version of glRenderbufferStorageMultisampleEXT into
its own function. I'm improving the explanation in generate_bindings.py.

I've also worked a bit on the Windows part of the patch, but it will take a
while longer to submit that, I'm having some unfortunate hardware problems which
are delaying testing.

I also had another idea for further development. Right now, the the unit tests
have mock functions from the GL interface as specified in the function
separation in generate_bindings.py. How about changing it so that there's a
separate mock function for each GL driver entry point, so that the gpu_unittests
could cover the GL bindings even more? Or is the philosophy to keep the
gpu_unittests only about the command buffer as much as possible? Anyway this
doesn't belong to this patch, but could be done in the future.

[no sievers, 2013-12-12 19:40:31]

On 2013/12/12 16:42:09, oetuaho wrote:
> I also had another idea for further development. Right now, the the unit tests
> have mock functions from the GL interface as specified in the function
> separation in generate_bindings.py. How about changing it so that there's a
> separate mock function for each GL driver entry point, so that the
gpu_unittests
> could cover the GL bindings even more?

Not sure if we need to expose these details to all the test. If glFooANGLE() is
compatible with glFooEXT(), and both are exposed as glFoo() in GLInterface,
isn't that all the test should care about then? I thought that was the idea of
|known_as|.

In the example above where extensions are *not* compatible, looks like we'd need
at least 2 or 3 different GLInterface functions:
glRenderbufferStorageMultisample, i.e. core es3/desktop ext/angle,
glRenderbufferStorageMultisampleESEXT, glRenderbufferStorageMultisampleESIMG,
due to different enums. But I thought that would be possible with your patch
(different |known_as| function for each).

[Ken Russell (switch to Gerrit), 2013-12-12 23:43:13]

On 2013/12/12 19:40:31, sievers wrote:
> In the example above where extensions are *not* compatible, looks like we'd
need
> at least 2 or 3 different GLInterface functions:
> glRenderbufferStorageMultisample, i.e. core es3/desktop ext/angle,
> glRenderbufferStorageMultisampleESEXT, glRenderbufferStorageMultisampleESIMG,
> due to different enums. But I thought that would be possible with your patch
> (different |known_as| function for each).

I'd have hoped the "ES" qualifier wouldn't be necessary, but as I keep thinking
about it -- and re-confusing myself -- perhaps it would actually be helpful.

[oetuaho, 2013-12-13 16:15:15]

On 2013/12/12 19:40:31, sievers wrote:
> On 2013/12/12 16:42:09, oetuaho wrote:
> > I also had another idea for further development. Right now, the the unit
tests
> > have mock functions from the GL interface as specified in the function
> > separation in generate_bindings.py. How about changing it so that there's a
> > separate mock function for each GL driver entry point, so that the
> gpu_unittests
> > could cover the GL bindings even more?
> 
> Not sure if we need to expose these details to all the test. If glFooANGLE()
is
> compatible with glFooEXT(), and both are exposed as glFoo() in GLInterface,
> isn't that all the test should care about then? I thought that was the idea of
> |known_as|.
> 
> In the example above where extensions are *not* compatible, looks like we'd
need
> at least 2 or 3 different GLInterface functions:
> glRenderbufferStorageMultisample, i.e. core es3/desktop ext/angle,
> glRenderbufferStorageMultisampleESEXT, glRenderbufferStorageMultisampleESIMG,
> due to different enums. But I thought that would be possible with your patch
> (different |known_as| function for each).

Yes, the addition of known_as does make changing the GLInterface function name
possible. Changing the tests to mock the GL driver instead of the GLInterface
would just make it possible to test that a GPU command buffer call triggers the
correct entry point all the way down in the driver. However, as I think about it
more, making separate tests for the bindings makes more sense to avoid
duplication in the command buffer tests.

[oetuaho, 2013-12-17 16:20:23]

The newest patch set contains an attempt to fix some of the Windows issues, and
I've successfully built it and run it in the default ANGLE mode. However, when I
specify --use-gl=desktop, something fails in and I get a non-responsive window
with no content regardless of if my patches are applied or not. I'm a beginner
in Win32 API, so I'm not sure where to start debugging this. Is the
--use-gl=desktop option on Windows actively maintained to begin with, or could
there be something wrong with my setup?

[Ken Russell (switch to Gerrit), 2013-12-17 20:05:06]

On 2013/12/17 16:20:23, oetuaho wrote:
> The newest patch set contains an attempt to fix some of the Windows issues,
and
> I've successfully built it and run it in the default ANGLE mode. However, when
I
> specify --use-gl=desktop, something fails in and I get a non-responsive window
> with no content regardless of if my patches are applied or not. I'm a beginner
> in Win32 API, so I'm not sure where to start debugging this. Is the
> --use-gl=desktop option on Windows actively maintained to begin with, or could
> there be something wrong with my setup?

+jbauman,bajones

Unfortunately, --use-gl=desktop mode has been broken on Windows since Chromium's
switch to Aura. It unfortunately hasn't been made a priority to fix it.

We can help test on other platforms to verify the GL code path. What platforms
do you need help with? Desktop Linux, Mac OS?

[no sievers, 2013-12-17 21:49:56]

On 2013/12/17 16:20:23, oetuaho wrote:
> The newest patch set contains an attempt to fix some of the Windows issues,
and
> I've successfully built it and run it in the default ANGLE mode. However, when
I
> specify --use-gl=desktop, something fails in and I get a non-responsive window
> with no content regardless of if my patches are applied or not. I'm a beginner
> in Win32 API, so I'm not sure where to start debugging this. Is the
> --use-gl=desktop option on Windows actively maintained to begin with, or could
> there be something wrong with my setup?

There is a known deadlock between GPU thread and UI thread in that mode (GPU
thread needs to wait for a task on the UI thread to create a window; UI thread
is blocked on GPU thread to finish creating the first context).
I think it's specific to win_aura with the browser compositor and might go away
with crbug.com/270179 getting addressed.

[oetuaho, 2013-12-18 15:01:49]

On 2013/12/17 20:05:06, Ken Russell wrote:
> On 2013/12/17 16:20:23, oetuaho wrote:
> > The newest patch set contains an attempt to fix some of the Windows issues,
> and
> > I've successfully built it and run it in the default ANGLE mode. However,
when
> I
> > specify --use-gl=desktop, something fails in and I get a non-responsive
window
> > with no content regardless of if my patches are applied or not. I'm a
beginner
> > in Win32 API, so I'm not sure where to start debugging this. Is the
> > --use-gl=desktop option on Windows actively maintained to begin with, or
could
> > there be something wrong with my setup?
> 
> +jbauman,bajones
> 
> Unfortunately, --use-gl=desktop mode has been broken on Windows since
Chromium's
> switch to Aura. It unfortunately hasn't been made a priority to fix it.
> 
> We can help test on other platforms to verify the GL code path. What platforms
> do you need help with? Desktop Linux, Mac OS?

Okay, good to know that my Windows build setup wasn't to blame. It's a bit iffy
to make such changes to the Windows OpenGL initialization when there's no way to
test them, but I suppose it's still better than leaving the changes out.

So far, I've tested the patch most on desktop Linux and Android, and I ran some
Windows tests on top of ANGLE today. I don't have Mac OS available here so
that's where I need the help. Also, if you are easily able to test on some
devices with other than NVIDIA GPUs that would of course be good.

Then, one more thing: I'm still not listed in the AUTHORS file with the email
address oetuaho@gmail.com, and it's not possible to set the Google account
primary email to oetuaho@nvidia.com. Last time, one of you submitted the change
on my behalf to work around this, but I'd rather see my name on the commit log
and I don't want this to become a recurring problem. Would it be possible to add
oetuaho@gmail.com to the AUTHORS file with a note saying that it's covered by
the NVIDIA CLA? Or is it possible to delete this Google account and immediately
create a new one with the same @nvidia.com address without registering a gmail
address?

[oetuaho, 2014-01-07 15:10:27]

I closed this issue to be able to reupload with my new account with the correct
email address.

The new issue, with the code rebased on top of the latest LKGR is here:
https://codereview.chromium.org/98643013 and I'll add the same reviewers there.