gpu: Upgrade to new ANGLE Platform Display init.

gpu: Upgrade to new ANGLE Platform Display init.

This allows us to select which ANGLE back-end to use more flexibly,
and controls the selection process entirely in Chrome.

The logic for when to try a backup renderer is a bit more complex
because we query the display extensions in the static binding
init code.

BUG=457325
Committed: https://crrev.com/fefb7cfcf73c07b5f2774266bda02e839a829ef1
Cr-Commit-Position: refs/heads/master@{#327315}

[Jamie Madill, 2015-04-16 18:27:09]

jmadill@chromium.org changed reviewers:
+ piman@chromium.org, zmo@chromium.org

[Jamie Madill, 2015-04-16 18:28:36]

Mo & Antoine,

Can you PTAL and see if the approach here is sensible? I was considering using a
more crafty approach by using a std::vector of init callback functions. Would be
size 1 in most cases, except D3D11-else-D3D9. It's also important to be able to
capture the flow in a UMA histogram.

[piman, 2015-04-16 21:51:05]

LGTM + nits overall. 
Worth a comment somewhere about why GetPlatformDisplay is needed (for the
bindings, and why the fallback doesn't matter in that case)

https://codereview.chromium.org/1051253008/diff/20001/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/1051253008/diff/20001/ui/gl/gl_surface_egl.cc...
ui/gl/gl_surface_egl.cc:378: if (client_extensions)
nit: need braces per style guide

https://codereview.chromium.org/1051253008/diff/20001/ui/gl/gl_surface_egl.h
File ui/gl/gl_surface_egl.h (right):

https://codereview.chromium.org/1051253008/diff/20001/ui/gl/gl_surface_egl.h#...
ui/gl/gl_surface_egl.h:54: static EGLDisplay
GetPlatformANGLEDisplay(EGLNativeDisplayType native_display,
nit: you can also have it as a free (static) function in the .cc file.

[zmo, 2015-04-16 21:52:33]

zmo@google.com changed reviewers:
+ zmo@google.com

[zmo, 2015-04-16 21:52:34]

LGTM with nits

https://codereview.chromium.org/1051253008/diff/20001/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/1051253008/diff/20001/ui/gl/gl_surface_egl.cc...
ui/gl/gl_surface_egl.cc:378: if (client_extensions)
nit: use {} if the block is more than one line.

https://codereview.chromium.org/1051253008/diff/20001/ui/gl/gl_surface_egl.h
File ui/gl/gl_surface_egl.h (right):

https://codereview.chromium.org/1051253008/diff/20001/ui/gl/gl_surface_egl.h#...
ui/gl/gl_surface_egl.h:54: static EGLDisplay
GetPlatformANGLEDisplay(EGLNativeDisplayType native_display,
This can just be a helper function in the anonymous namespace in the .cc file. 
No need to add it to the class.

[Jamie Madill, 2015-04-17 19:14:13]

Thanks for feedback, PTAL.

Antoine (and/or Mo), is it a concern that the D3D11-failing-D3D9-fallback
systems won't initialize the display extensions string properly? (When before
they would be)

https://codereview.chromium.org/1051253008/diff/20001/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/1051253008/diff/20001/ui/gl/gl_surface_egl.cc...
ui/gl/gl_surface_egl.cc:378: if (client_extensions)
On 2015/04/16 21:52:34, zmo wrote:
> nit: use {} if the block is more than one line.

Done.

https://codereview.chromium.org/1051253008/diff/20001/ui/gl/gl_surface_egl.h
File ui/gl/gl_surface_egl.h (right):

https://codereview.chromium.org/1051253008/diff/20001/ui/gl/gl_surface_egl.h#...
ui/gl/gl_surface_egl.h:54: static EGLDisplay
GetPlatformANGLEDisplay(EGLNativeDisplayType native_display,
On 2015/04/16 21:52:34, zmo wrote:
> This can just be a helper function in the anonymous namespace in the .cc file.

> No need to add it to the class.

Done.

[piman, 2015-04-17 19:24:58]

On Fri, Apr 17, 2015 at 12:14 PM, <jmadill@chromium.org> wrote:

> Thanks for feedback, PTAL.
>
> Antoine (and/or Mo), is it a concern that the D3D11-failing-D3D9-fallback
> systems won't initialize the display extensions string properly? (When
> before
> they would be)


Do they differ between D3D9 and D3D11?

Antoine


>
>
>
>
> https://codereview.chromium.org/1051253008/diff/20001/ui/gl/gl_surface_egl.cc
> File ui/gl/gl_surface_egl.cc (right):
>
>
>
https://codereview.chromium.org/1051253008/diff/20001/ui/gl/gl_surface_egl.cc...
> ui/gl/gl_surface_egl.cc:378: if (client_extensions)
> On 2015/04/16 21:52:34, zmo wrote:
>
>> nit: use {} if the block is more than one line.
>>
>
> Done.
>
>
> https://codereview.chromium.org/1051253008/diff/20001/ui/gl/gl_surface_egl.h
> File ui/gl/gl_surface_egl.h (right):
>
>
>
https://codereview.chromium.org/1051253008/diff/20001/ui/gl/gl_surface_egl.h#...
> ui/gl/gl_surface_egl.h:54: static EGLDisplay
> GetPlatformANGLEDisplay(EGLNativeDisplayType native_display,
> On 2015/04/16 21:52:34, zmo wrote:
>
>> This can just be a helper function in the anonymous namespace in the
>>
> .cc file.
>
>> No need to add it to the class.
>>
>
> Done.
>
> https://codereview.chromium.org/1051253008/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jamie Madill, 2015-04-17 19:31:12]

> Do they differ between D3D9 and D3D11?
> 
> Antoine

Yes, they can differ. For example, post_sub_buffer is only supported on D3D9
currently.
Also, if we fail to initialize D3D11 when we expect it to work, we'll end up
with
a failure to init the display in DriverEGL::GetPlatformExtensions and likely
return
an empty extensions string.

Alternately we could initialize the display before initializing the bindings,
and then
finish initializing the surface (context, config, etc) in InitializeOneOff.

[piman, 2015-04-17 19:39:27]

On Fri, Apr 17, 2015 at 12:31 PM, <jmadill@chromium.org> wrote:

> Do they differ between D3D9 and D3D11?
>>
>
>  Antoine
>>
>
> Yes, they can differ. For example, post_sub_buffer is only supported on
> D3D9
> currently.
>

That would be an important difference


> Also, if we fail to initialize D3D11 when we expect it to work, we'll end
> up
> with
> a failure to init the display in DriverEGL::GetPlatformExtensions and
> likely
> return
> an empty extensions string.
>
> Alternately we could initialize the display before initializing the
> bindings,
> and then
> finish initializing the surface (context, config, etc) in InitializeOneOff.
>

I think we should do that then.


>
> https://codereview.chromium.org/1051253008/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jamie Madill, 2015-04-20 18:33:11]

PTAL, I redesigned this code somewhat to initialize the display on Windows
before querying for extensions.

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

https://codereview.chromium.org/1051253008/diff/80001/ui/gl/gl_bindings.cc#ne...
ui/gl/gl_bindings.cc:47: return "";
I wasn't sure if I should put the NO_DISPLAY check in the main body of the
function, because it theoretically would change the behaviour of the function on
non-Windows platforms.

[Zhenyao Mo, 2015-04-20 19:51:47]

LGTM

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

https://codereview.chromium.org/1051253008/diff/80001/ui/gl/gl_bindings.cc#ne...
ui/gl/gl_bindings.cc:47: return "";
On 2015/04/20 18:33:11, Jamie Madill wrote:
> I wasn't sure if I should put the NO_DISPLAY check in the main body of the
> function, because it theoretically would change the behaviour of the function
on
> non-Windows platforms.

If display isn't valid, eglQueryString just return NULL. So probably you can
unify the WIN and non WIN path behaviors.

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

https://codereview.chromium.org/1051253008/diff/80001/ui/gl/gl_surface_egl.cc...
ui/gl/gl_surface_egl.cc:178: DCHECK(false);
nit: you can use NOTREACHED() here.

https://codereview.chromium.org/1051253008/diff/80001/ui/gl/gl_surface_egl.cc...
ui/gl/gl_surface_egl.cc:196: DCHECK(false);
nit: you can use NOTREACHED() here.

https://codereview.chromium.org/1051253008/diff/80001/ui/gl/gl_surface_egl.cc...
ui/gl/gl_surface_egl.cc:202: bool supports_angle_d3d) {
nit: In general input args should be before output args.

[Jamie Madill, 2015-04-20 20:20:10]

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

https://codereview.chromium.org/1051253008/diff/80001/ui/gl/gl_bindings.cc#ne...
ui/gl/gl_bindings.cc:47: return "";
On 2015/04/20 19:51:47, Zhenyao Mo wrote:
> On 2015/04/20 18:33:11, Jamie Madill wrote:
> > I wasn't sure if I should put the NO_DISPLAY check in the main body of the
> > function, because it theoretically would change the behaviour of the
function
> on
> > non-Windows platforms.
> 
> If display isn't valid, eglQueryString just return NULL. So probably you can
> unify the WIN and non WIN path behaviors.

One difference - if display is EGL_NO_DISPLAY and the EGL platform supports
EGL_EXT_client_extensions (or EGL 1.5), the EGL will return a list of client
extensions. (See GetClientExtensions below). I think in either case it makes
sense to unify, thanks.

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

https://codereview.chromium.org/1051253008/diff/80001/ui/gl/gl_surface_egl.cc...
ui/gl/gl_surface_egl.cc:178: DCHECK(false);
On 2015/04/20 19:51:47, Zhenyao Mo wrote:
> nit: you can use NOTREACHED() here.

Done.

https://codereview.chromium.org/1051253008/diff/80001/ui/gl/gl_surface_egl.cc...
ui/gl/gl_surface_egl.cc:196: DCHECK(false);
On 2015/04/20 19:51:47, Zhenyao Mo wrote:
> nit: you can use NOTREACHED() here.

Done.

https://codereview.chromium.org/1051253008/diff/80001/ui/gl/gl_surface_egl.cc...
ui/gl/gl_surface_egl.cc:202: bool supports_angle_d3d) {
On 2015/04/20 19:51:47, Zhenyao Mo wrote:
> nit: In general input args should be before output args.

Done.

[Jamie Madill, 2015-04-23 16:32:43]

Ping Antoine, could you re-review the diff of this patch? There are some
significant changes.

[piman, 2015-04-23 21:03:30]

https://codereview.chromium.org/1051253008/diff/100001/ui/gl/gl_bindings.cc
File ui/gl/gl_bindings.cc (right):

https://codereview.chromium.org/1051253008/diff/100001/ui/gl/gl_bindings.cc#n...
ui/gl/gl_bindings.cc:54: g_driver_egl.fn.eglInitializeFn(display, NULL, NULL);
It would be easier to follow if both OS_WIN and !OS_WIN went through the same
path here, i.e. have GLSurfaceEGL::InitializeDisplay be exposed on both - on
!OS_WIN it would simply do the eglGetDisplay and eglInitialize. That could even
be reused in GLSurfaceEGL::InitializeOneOff

https://codereview.chromium.org/1051253008/diff/100001/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/1051253008/diff/100001/ui/gl/gl_surface_egl.c...
ui/gl/gl_surface_egl.cc:233: }
I like this, it's easier to follow with the logic in one place.

[Jamie Madill, 2015-04-24 16:37:54]

Antoine PTAL,

I did as you suggested, and took it further, to use the same code paths on all
platforms. The init will only use the ANGLE extension in the display supports
it. If you'd rather I keep the code paths separate for now I can roll back to
patch set 7. I also wasn't sure if I should keep the swiftshader bit out of the
non-Windows paths.

[piman, 2015-04-27 22:56:30]

LGTM, seems perfectly reasonable.

[Jamie Madill, 2015-04-28 16:32:31]

The CQ bit was checked by jmadill@chromium.org

[Jamie Madill, 2015-04-28 16:32:31]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@google.com,
zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/1051253008/#ps160001
(title: "Fix for ChromeOS build")

[commit-bot: I haz the power, 2015-04-28 16:32:56]

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

[commit-bot: I haz the power, 2015-04-28 17:23:54]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-04-28 17:25:55]

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