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. The patch is conservative like this to avoid
requiring the exact specification of binding conditions in case a function
only has one name, and to make it less likely to expose bugs elsewhere.

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
TEST=gpu_unittests, cc_unittests, WebGL conformance tests

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

[oetuaho-nv, 2014-01-07 16:11:43]

Reuploaded from https://codereview.chromium.org/94963003/ to get the patch under
the correct code review account. The patch is rebased on top of the latest LKGR,
but not yet thoroughly tested after that.

How will we move forward with this? Will we wait until the Windows OpenGL
initialization issues are resolved (crbug.com/270179 addressed?) or submit this
regardless? As far as I'm concerned the patch is good to go after I finish one
more round of testing it. Do you have any review comments after my latest
changes?

[Ken Russell (switch to Gerrit), 2014-01-08 02:46:59]

I think we should proceed with landing this, and not block it on
crbug.com/270179 . It fixes known correctness bugs.

One test that should be done is to add use_aura=1 to GYP_DEFINES on Linux, rerun
gclient runhooks, rebuild, and test the browser. That will at least ensure that
Linux Aura still works, so will provide test coverage of one Aura configuration.

What platforms can we help test on?

Can you make sure to send this to the win_gpu, linux_gpu, mac_gpu, and
mac_gpu_retina try bots?

[oetuaho-nv, 2014-01-08 17:18:07]

On 2014/01/08 02:46:59, Ken Russell wrote:
> I think we should proceed with landing this, and not block it on
> crbug.com/270179 . It fixes known correctness bugs.

Okay, I'd also prefer that we proceed.

> One test that should be done is to add use_aura=1 to GYP_DEFINES on Linux,
rerun
> gclient runhooks, rebuild, and test the browser. That will at least ensure
that
> Linux Aura still works, so will provide test coverage of one Aura
configuration.

Done. It works as far as I can see. I also re-ran some tests on Android, Windows
with ANGLE, and the normal Linux configuration, with this new rebased version.

> What platforms can we help test on?

I don't have the setup to test on Mac or non-NVIDIA hardware. So Mac at least,
and you can test with other GPU vendors as much as you think it is necessary.

> Can you make sure to send this to the win_gpu, linux_gpu, mac_gpu, and
> mac_gpu_retina try bots?

All the trybots are reporting failure: -1 whether I try to start them from the
web UI or from the command line using git cl try. I think I'm lacking trybot
access permissions. By the way, this could be explained clearer in the
"Contributing" section of
https://sites.google.com/a/chromium.org/dev/developers/how-tos/get-the-code with
a link to http://dev.chromium.org/developers/testing/try-server-usage

[oetuaho-nv, 2014-01-10 15:47:36]

On 2014/01/08 17:18:07, oetuaho-nv wrote:
> On 2014/01/08 02:46:59, Ken Russell wrote:
> > I think we should proceed with landing this, and not block it on
> > crbug.com/270179 . It fixes known correctness bugs.
> 
> Okay, I'd also prefer that we proceed.
> 
> > One test that should be done is to add use_aura=1 to GYP_DEFINES on Linux,
> rerun
> > gclient runhooks, rebuild, and test the browser. That will at least ensure
> that
> > Linux Aura still works, so will provide test coverage of one Aura
> configuration.
> 
> Done. It works as far as I can see. I also re-ran some tests on Android,
Windows
> with ANGLE, and the normal Linux configuration, with this new rebased version.
> 
> > What platforms can we help test on?
> 
> I don't have the setup to test on Mac or non-NVIDIA hardware. So Mac at least,
> and you can test with other GPU vendors as much as you think it is necessary.
> 
> > Can you make sure to send this to the win_gpu, linux_gpu, mac_gpu, and
> > mac_gpu_retina try bots?
> 
> All the trybots are reporting failure: -1 whether I try to start them from the
> web UI or from the command line using git cl try. I think I'm lacking trybot
> access permissions. By the way, this could be explained clearer in the
> "Contributing" section of
> https://sites.google.com/a/chromium.org/dev/developers/how-tos/get-the-code
with
> a link to http://dev.chromium.org/developers/testing/try-server-usage

Some of the trybots revealed a build issue caused by a missing GL_EXPORT. That
is now fixed, and try jobs have been resubmitted. There were other errors as
well, but all of those looked unrelated.

[oetuaho-nv, 2014-01-13 17:09:33]

Submitted another rebase to accommodate the patch "Allow tests to stub out GL
draw calls."

Could I please get another round of reviews?

[no sievers, 2014-01-13 23:46:40]

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

https://codereview.chromium.org/98643013/diff/620001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:26: extension/extensions: Extensions where the
function is found. If not
Do we need to support both 'extension' and 'extensions'?
It seems to just make it more slightly complicated and confusing below.

https://codereview.chromium.org/98643013/diff/620001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:1493: if len(func['names']) == 1:
Hmm... is that the right check? Aren't you trying to figure out whether this is
a conditional binding or not, which could have only one name?

https://codereview.chromium.org/98643013/diff/620001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:1531: def Wrap(cond):
nit: Maybe this function can be written slightly more readable by having
|version_condition| and |ext_condition| and simply returning
'(version_condition) && (ext_condition)'.

https://codereview.chromium.org/98643013/diff/620001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:1538: assert len(func['versions']) > 1
Why? What if there was a GLES2-only function that I wanted to bind conditionally
(only on GLES2 platforms)?

https://codereview.chromium.org/98643013/diff/620001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:1544: cond = VersionCondition(version)
I find the condition combining hard to read and it also seems to generate
different code depending on order of the 'versions' list (for example 3 versions
with names 'glFoo', 'glFooOES', 'glFoo' vs. 'glFoo', 'glFoo', 'glFooOES').

Is it really necessary or could it just be left out? It looks to me like it just
generates 'if (a || b) BindFunc' instead of two if-statements therefore avoiding
assigning the value twice in the (somewhat unexpected?) case of multiple version
conditions both resolving to true on a given platform.

https://codereview.chromium.org/98643013/diff/620001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:1567: if cond != '' and i + 1 <
len(func['versions']):
nit: Can you create a Boolean |last_version| for 'i == len(func['versions']' for
readability in this condition? (You are using it above also.)

https://codereview.chromium.org/98643013/diff/620001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:1988: func['names'].append(version['name'])
Why do we need both func['names'] and func['versions'] rather than just using,
let's say, func['versions'] from here on?

[oetuaho-nv, 2014-01-14 16:39:57]

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

https://codereview.chromium.org/98643013/diff/620001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:26: extension/extensions: Extensions where the
function is found. If not
On 2014/01/13 23:46:40, sievers wrote:
> Do we need to support both 'extension' and 'extensions'?
> It seems to just make it more slightly complicated and confusing below.

I did it this way since extension functions are mostly found only in one
extension, and leaving the list out in that case makes the function specs a bit
easier to read/write. But this isn't a very strong reason, and I'll change it so
that only 'extensions' is used to make the code simpler.

https://codereview.chromium.org/98643013/diff/620001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:1493: if len(func['names']) == 1:
On 2014/01/13 23:46:40, sievers wrote:
> Hmm... is that the right check? Aren't you trying to figure out whether this
is
> a conditional binding or not, which could have only one name?

The bindings only guarantee that the correct version of a function will be bound
if it is available. If a function only has one name, the safest way to achieve
this is to try bind it regardless of where the function should be available or
not. The higher levels of the code have the responsibility to determine which
functions to actually use. The commit message and the comment at the top of the
file try to explain this as well.

Eventually, it might be good if the higher levels could check whether a function
is usable just by checking if it's bound to null, but it takes a lot of work
with the GL specs and testing with different platforms to get there. My opinion
is that this work should be split into multiple patches, instead of trying to
roll it into this one patch that has already been in review for 5 weeks.

https://codereview.chromium.org/98643013/diff/620001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:1531: def Wrap(cond):
On 2014/01/13 23:46:40, sievers wrote:
> nit: Maybe this function can be written slightly more readable by having
> |version_condition| and |ext_condition| and simply returning
> '(version_condition) && (ext_condition)'.

Either of the conditions might be empty, so simply returning version_condition
&& ext_condition doesn't work. We might initialize the values to true, but I
don't think we want a lot of '&& true' in the generated code either.

Wrapping the conditions to parentheses could be unconditional, and I used to
have it this way at first, but that resulted in a lot of extra parentheses in
the generated code, which made it hard to follow.

I think the best way to go is to get rid of the list comprehension, which makes
the join line simpler, and use the Wrap function for version_cond and ext_cond
separately. I'll also rename the two different Wrap functions to WrapOr and
WrapAnd to eliminate some confusion.

https://codereview.chromium.org/98643013/diff/620001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:1538: assert len(func['versions']) > 1
On 2014/01/13 23:46:40, sievers wrote:
> Why? What if there was a GLES2-only function that I wanted to bind
conditionally
> (only on GLES2 platforms)?

I believe my answer above covers this - the philosophy is to bind functions
conditionally only if it is necessary for choosing the correct function.

https://codereview.chromium.org/98643013/diff/620001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:1544: cond = VersionCondition(version)
On 2014/01/13 23:46:40, sievers wrote:
> I find the condition combining hard to read and it also seems to generate
> different code depending on order of the 'versions' list (for example 3
versions
> with names 'glFoo', 'glFooOES', 'glFoo' vs. 'glFoo', 'glFoo', 'glFooOES').
> 
> Is it really necessary or could it just be left out? It looks to me like it
just
> generates 'if (a || b) BindFunc' instead of two if-statements therefore
avoiding
> assigning the value twice in the (somewhat unexpected?) case of multiple
version
> conditions both resolving to true on a given platform.

You're right about the combining depending on the order of the versions list,
but I don't think that is a problem. The order of the versions list determines
which version is preferred. You can't combine the conditions of glFoo (1) and
glFoo (2), in case binding glFooOES is preferred less than glFoo (1) but
preferred more than glFoo (2).

Combining conditions is here in the first place to make the generated code easy
to follow. Without it, there could be nonsensical generated code such as first
binding the function conditionally and then binding it unconditionally. This
way, if there's a bug concerning a specific function - such as the bug with
glDiscardFramebufferEXT that this patch fixes - it's easier to check if the
bindings are correct.

https://codereview.chromium.org/98643013/diff/620001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:1567: if cond != '' and i + 1 <
len(func['versions']):
On 2014/01/13 23:46:40, sievers wrote:
> nit: Can you create a Boolean |last_version| for 'i == len(func['versions']'
for
> readability in this condition? (You are using it above also.)

Done.

https://codereview.chromium.org/98643013/diff/620001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:1988: func['names'].append(version['name'])
On 2014/01/13 23:46:40, sievers wrote:
> Why do we need both func['names'] and func['versions'] rather than just using,
> let's say, func['versions'] from here on?

The set of unique names is needed on a few occasions, and there it was easier to
refer to func['names'] than set([version['name'] for version in
func['versions']]) which will also lose the ordering. However, it seems like
having a list of unique names with the order preserved is not necessary, and I
understand this might be a source of confusion. I'll change it so that only
'versions' is used from here on.

[oetuaho-nv, 2014-01-14 20:42:09]

I did a couple of other minor improvements to generate_bindings.py while trying
to take the review feedback into account. Tests I've ran on Linux after the
changes are passing, but since this is a slightly bigger change to the patch
I'll do more testing on other platforms as well.

[no sievers, 2014-01-14 23:16:00]

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

https://codereview.chromium.org/98643013/diff/620001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:1493: if len(func['names']) == 1:
On 2014/01/14 16:39:58, oetuaho-nv wrote:
> On 2014/01/13 23:46:40, sievers wrote:
> > Hmm... is that the right check? Aren't you trying to figure out whether this
> is
> > a conditional binding or not, which could have only one name?
> 
> The bindings only guarantee that the correct version of a function will be
bound
> if it is available. If a function only has one name, the safest way to achieve
> this is to try bind it regardless of where the function should be available or
> not. The higher levels of the code have the responsibility to determine which
> functions to actually use. The commit message and the comment at the top of
the
> file try to explain this as well.
> 
> Eventually, it might be good if the higher levels could check whether a
function
> is usable just by checking if it's bound to null, but it takes a lot of work
> with the GL specs and testing with different platforms to get there. My
opinion
> is that this work should be split into multiple patches, instead of trying to
> roll it into this one patch that has already been in review for 5 weeks.

Why not check the extension? That seems like the right way to see if a function
is available and already works.

I don't think we have to worry about the case where the extension is advertised
and the entry points are not available or broken for some reason. That sounds
like a really broken implementation.

Can't we use the 'NotBound' stub rather than dropping version conditions of the
last version?

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

https://codereview.chromium.org/98643013/diff/750001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:1983: func['known_as'] = func['versions'][0]['name']
Should we null out func['names'] here somewhere to make sure nobody starts using
it in this script with somewhat unpredictable results?

https://codereview.chromium.org/98643013/diff/750001/ui/gl/gl_implementation_...
File ui/gl/gl_implementation_win.cc (right):

https://codereview.chromium.org/98643013/diff/750001/ui/gl/gl_implementation_...
ui/gl/gl_implementation_win.cc:271: InitializeStaticGLBindingsWGL();
How about only looking up the symbol for wglCreateContext here manually?

That way we wouldn't have to init and clear all the other bindings for nothing
and it would simplify the comment/need for explanation.

https://codereview.chromium.org/98643013/diff/750001/ui/gl/gl_surface.cc
File ui/gl/gl_surface.cc (right):

https://codereview.chromium.org/98643013/diff/750001/ui/gl/gl_surface.cc#newc...
ui/gl/gl_surface.cc:57: initialized = InitializePreBindingsInternal(impl);
Adding InitializePreBindingsInternal() everywhere makes the code a bit harder to
follow (and it's already pretty confusing). Would it work to simply call
GLSurfaceWGL::InitializeOneOff() from gl_implementation_win.cc?

[oetuaho-nv, 2014-01-15 15:50:23]

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

https://codereview.chromium.org/98643013/diff/620001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:1493: if len(func['names']) == 1:
On 2014/01/14 23:16:01, sievers wrote:
> On 2014/01/14 16:39:58, oetuaho-nv wrote:
> > On 2014/01/13 23:46:40, sievers wrote:
> > > Hmm... is that the right check? Aren't you trying to figure out whether
this
> > is
> > > a conditional binding or not, which could have only one name?
> > 
> > The bindings only guarantee that the correct version of a function will be
> bound
> > if it is available. If a function only has one name, the safest way to
achieve
> > this is to try bind it regardless of where the function should be available
or
> > not. The higher levels of the code have the responsibility to determine
which
> > functions to actually use. The commit message and the comment at the top of
> the
> > file try to explain this as well.
> > 
> > Eventually, it might be good if the higher levels could check whether a
> function
> > is usable just by checking if it's bound to null, but it takes a lot of work
> > with the GL specs and testing with different platforms to get there. My
> opinion
> > is that this work should be split into multiple patches, instead of trying
to
> > roll it into this one patch that has already been in review for 5 weeks.
> 
> Why not check the extension? That seems like the right way to see if a
function
> is available and already works.
> 
> I don't think we have to worry about the case where the extension is
advertised
> and the entry points are not available or broken for some reason. That sounds
> like a really broken implementation.
> 
> Can't we use the 'NotBound' stub rather than dropping version conditions of
the
> last version?

The risk doesn't result only from broken driver implementations, but rather that
all the conditions for binding a function need to be 100% correct if they are
always enforced. The most common problematic case is that the same function is
defined in an ARB extension and also in core in a later version. In this case it
needs to be explicitly specified like this:

'versions': [{ 'name': 'glFoo', 'gl_version': '4.0'},
             { 'name': 'glFoo', 'extensions': ['GL_ARB_FOO']}]

Otherwise the code will just automatically make the ARB extension a condition
for binding the function based on the extension headers, which will probably
usually work, but there could be a spec-conformant GL implementation where this
doesn't work, and if it exists it's probably exotic enough that it doesn't come
up in testing. So I'd prefer that this work would be done in smaller, more
manageable pieces, instead of needing to go through all the GL specs ever
published at once to check for cases like this.

It's also a small additional risk if functions are moved from
InitializeStaticBindings to InitializeDynamicBindings, mostly on Windows where
the OpenGL initialization is broken and we can't properly test this, but where
we should still avoid additional regressions. You yourself were worried about
functions being called before a context is made current and the dynamic bindings
are initialized in an earlier review on 2013/12/05.

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

https://codereview.chromium.org/98643013/diff/750001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:1983: func['known_as'] = func['versions'][0]['name']
On 2014/01/14 23:16:01, sievers wrote:
> Should we null out func['names'] here somewhere to make sure nobody starts
using
> it in this script with somewhat unpredictable results?

That should typically make the build fail when running this script, but I agree,
it's clearer if the 'names' key is removed. Done.

https://codereview.chromium.org/98643013/diff/750001/ui/gl/gl_implementation_...
File ui/gl/gl_implementation_win.cc (right):

https://codereview.chromium.org/98643013/diff/750001/ui/gl/gl_implementation_...
ui/gl/gl_implementation_win.cc:271: InitializeStaticGLBindingsWGL();
On 2014/01/14 23:16:01, sievers wrote:
> How about only looking up the symbol for wglCreateContext here manually?
> 
> That way we wouldn't have to init and clear all the other bindings for nothing
> and it would simplify the comment/need for explanation.

True. Done.

https://codereview.chromium.org/98643013/diff/750001/ui/gl/gl_surface.cc
File ui/gl/gl_surface.cc (right):

https://codereview.chromium.org/98643013/diff/750001/ui/gl/gl_surface.cc#newc...
ui/gl/gl_surface.cc:57: initialized = InitializePreBindingsInternal(impl);
On 2014/01/14 23:16:01, sievers wrote:
> Adding InitializePreBindingsInternal() everywhere makes the code a bit harder
to
> follow (and it's already pretty confusing). Would it work to simply call
> GLSurfaceWGL::InitializeOneOff() from gl_implementation_win.cc?

I agree, done.

[no sievers, 2014-01-15 21:03:27]

LGTM, thanks.

Just wondering: Why do you specify desktop GL 3.0 for many of the decoder tests
and also the mock bindings?

Is that to guarantee commonly used GL functions are resolved with the mock
bindings?

I thought generally we'd only rely on 2.1 and in the decoder tests request other
needed extensions explicitly. 

For all the other unit tests, I guess I'm not sure what the expectations and
implications are.

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

https://codereview.chromium.org/98643013/diff/620001/ui/gl/generate_bindings....
ui/gl/generate_bindings.py:1493: if len(func['names']) == 1:
On 2014/01/15 15:50:24, oetuaho-nv wrote:
> On 2014/01/14 23:16:01, sievers wrote:
> > On 2014/01/14 16:39:58, oetuaho-nv wrote:
> > > On 2014/01/13 23:46:40, sievers wrote:
> > > > Hmm... is that the right check? Aren't you trying to figure out whether
> this
> > > is
> > > > a conditional binding or not, which could have only one name?
> > > 
> > > The bindings only guarantee that the correct version of a function will be
> > bound
> > > if it is available. If a function only has one name, the safest way to
> achieve
> > > this is to try bind it regardless of where the function should be
available
> or
> > > not. The higher levels of the code have the responsibility to determine
> which
> > > functions to actually use. The commit message and the comment at the top
of
> > the
> > > file try to explain this as well.
> > > 
> > > Eventually, it might be good if the higher levels could check whether a
> > function
> > > is usable just by checking if it's bound to null, but it takes a lot of
work
> > > with the GL specs and testing with different platforms to get there. My
> > opinion
> > > is that this work should be split into multiple patches, instead of trying
> to
> > > roll it into this one patch that has already been in review for 5 weeks.
> > 
> > Why not check the extension? That seems like the right way to see if a
> function
> > is available and already works.
> > 
> > I don't think we have to worry about the case where the extension is
> advertised
> > and the entry points are not available or broken for some reason. That
sounds
> > like a really broken implementation.
> > 
> > Can't we use the 'NotBound' stub rather than dropping version conditions of
> the
> > last version?
> 
> The risk doesn't result only from broken driver implementations, but rather
that
> all the conditions for binding a function need to be 100% correct if they are
> always enforced. The most common problematic case is that the same function is
> defined in an ARB extension and also in core in a later version. In this case
it
> needs to be explicitly specified like this:
> 
> 'versions': [{ 'name': 'glFoo', 'gl_version': '4.0'},
>              { 'name': 'glFoo', 'extensions': ['GL_ARB_FOO']}]
> 
> Otherwise the code will just automatically make the ARB extension a condition
> for binding the function based on the extension headers, which will probably
> usually work, but there could be a spec-conformant GL implementation where
this
> doesn't work, and if it exists it's probably exotic enough that it doesn't
come
> up in testing. So I'd prefer that this work would be done in smaller, more
> manageable pieces, instead of needing to go through all the GL specs ever
> published at once to check for cases like this.
> 
> It's also a small additional risk if functions are moved from
> InitializeStaticBindings to InitializeDynamicBindings, mostly on Windows where
> the OpenGL initialization is broken and we can't properly test this, but where
> we should still avoid additional regressions. You yourself were worried about
> functions being called before a context is made current and the dynamic
bindings
> are initialized in an earlier review on 2013/12/05.

Actually I was more worried about the case where we specify the last version
with an explicit condition (for example glDiscardFramebufferEXT()), where it
seemed weird that we just drop that condition.

But fair enough :)

[oetuaho-nv, 2014-01-16 13:09:58]

On 2014/01/15 21:03:27, sievers wrote:
> LGTM, thanks.
> 
> Just wondering: Why do you specify desktop GL 3.0 for many of the decoder
tests
> and also the mock bindings?
> 
> Is that to guarantee commonly used GL functions are resolved with the mock
> bindings?
> 
> I thought generally we'd only rely on 2.1 and in the decoder tests request
other
> needed extensions explicitly. 
> 
> For all the other unit tests, I guess I'm not sure what the expectations and
> implications are.
> 

Thanks for the approval.

For most of the tests, the choice of GL version doesn't matter much for the mock
bindings, since GetMockGLProcAddress will return the same mock function for all
the different versions of the same function. Only when the same function name is
seen in multiple function specs in generate_bindings.py the version can have an
effect on the bindings. But on the other hand, it's nice if the GL version and
extensions are specified so that the function would exist in a real GL
implementation with those characteristics. This would be required if the
fallback behavior of the bindings was removed.

For these reasons, 3.0 is the easiest default choice, but sometimes it makes
sense to specify a different version if for example the extension has been moved
to core in later versions or the functionality only exists in ES.

It would still be good to get a bit more automated test coverage for the
bindings, and I have some code written for that, but I think we can handle it in
a separate patch, if you're okay with that.

[commit-bot: I haz the power, 2014-01-16 16:47:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oetuaho@nvidia.com/98643013/900001

[commit-bot: I haz the power, 2014-01-16 17:42:30]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[oetuaho-nv, 2014-01-16 18:34:38]

Needed to rebase yet again, since new tests with incompatible InitDecoder() call
had appeared.

Actually, now I noticed that you didn't explicitly mention any manual testing on
Mac, which I haven't been doing in my end - should I still wait for that before
ticking the commit box? Or do other reviewers still need to have their say as
well?

[no sievers, 2014-01-16 21:04:42]

On 2014/01/16 18:34:38, oetuaho-nv wrote:
> Needed to rebase yet again, since new tests with incompatible InitDecoder()
call
> had appeared.
> 
> Actually, now I noticed that you didn't explicitly mention any manual testing
on
> Mac, which I haven't been doing in my end - should I still wait for that
before
> ticking the commit box? Or do other reviewers still need to have their say as
> well?

Maybe kbr@ or piman@ can at least stamp ui/gl again, since there were some
slight changes over the patch from the previous codereview issue?

Otherwise I'd leave it to the commit queue. I think we'd notice fairly quickly
if something broke :)

[piman, 2014-01-16 21:44:51]

lgtm

[Ken Russell (switch to Gerrit), 2014-01-16 21:46:33]

On 2014/01/16 21:04:42, sievers wrote:
> On 2014/01/16 18:34:38, oetuaho-nv wrote:
> > Needed to rebase yet again, since new tests with incompatible InitDecoder()
> call
> > had appeared.
> > 
> > Actually, now I noticed that you didn't explicitly mention any manual
testing
> on
> > Mac, which I haven't been doing in my end - should I still wait for that
> before
> > ticking the commit box? Or do other reviewers still need to have their say
as
> > well?
> 
> Maybe kbr@ or piman@ can at least stamp ui/gl again, since there were some
> slight changes over the patch from the previous codereview issue?
> 
> Otherwise I'd leave it to the commit queue. I think we'd notice fairly quickly
> if something broke :)

I apologize for the holdup, and was still planning to test this manually on Mac
OS, today.

If you can't wait, I'll be glad to re-review now.

[Ken Russell (switch to Gerrit), 2014-01-17 04:21:31]

On 2014/01/16 21:46:33, Ken Russell wrote:
> On 2014/01/16 21:04:42, sievers wrote:
> > On 2014/01/16 18:34:38, oetuaho-nv wrote:
> > > Needed to rebase yet again, since new tests with incompatible
InitDecoder()
> > call
> > > had appeared.
> > > 
> > > Actually, now I noticed that you didn't explicitly mention any manual
> testing
> > on
> > > Mac, which I haven't been doing in my end - should I still wait for that
> > before
> > > ticking the commit box? Or do other reviewers still need to have their say
> as
> > > well?
> > 
> > Maybe kbr@ or piman@ can at least stamp ui/gl again, since there were some
> > slight changes over the patch from the previous codereview issue?
> > 
> > Otherwise I'd leave it to the commit queue. I think we'd notice fairly
quickly
> > if something broke :)
> 
> I apologize for the holdup, and was still planning to test this manually on
Mac
> OS, today.
> 
> If you can't wait, I'll be glad to re-review now.

Apologies again for the delay.

I still didn't have time today to test this patch -- sorry again -- but I
shouldn't hold it up. Please try it again on win_gpu, linux_gpu, mac_gpu and
mac_gpu_retina explicitly, and if it passes those bots, just CQ it.

The patch still LGTM although I share sievers' concern about specifying OpenGL
3.0 as the OpenGL version in the unit tests. While we certainly intend to
upgrade to a more recent version on both Mac and Linux, right now OpenGL 2.1 +
vendor extensions is used on these platforms. The tests should probably match
how the code works.

[commit-bot: I haz the power, 2014-01-17 13:25:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oetuaho@nvidia.com/98643013/1400001

[commit-bot: I haz the power, 2014-01-17 13:44:21]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[oetuaho-nv, 2014-01-17 13:59:11]

Had to do one more rebase (hopefully the last)

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    media/tools/player_x11/gl_video_renderer.cc

Presubmit is failing on this one file, adding bunch of folks from media/OWNERS
to review the relatively trivial change.

[scherkus (not reviewing), 2014-01-17 17:46:58]

media/ lgtm

[commit-bot: I haz the power, 2014-01-17 20:10:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oetuaho@nvidia.com/98643013/1400001

[commit-bot: I haz the power, 2014-01-17 21:31:04]

Change committed as 245623