Add extension to create software device.

Add extension to create software device.

Add an extension so that users can receive a device that renders in software. Currently this is done by loading a swiftshader dll.


BUG=
TEST=

Committed: http://code.google.com/p/angleproject/source/detail?r=705

[John Bauman, 2011-07-06 21:51:55]

Hi, 

To me, this seems like the most straightforward way to specify that a software
display will be used. Any suggestions?

[dgkoch, 2011-07-08 20:46:58]

On 2011/07/06 21:51:55, John Bauman wrote:
> Hi, 
> 
> To me, this seems like the most straightforward way to specify that a software
> display will be used. Any suggestions?

In general I'd rather you use the integrated version of ANGLE+swift, instead of
using the d3d9 interface for it (see email from Gav yesterday).

However if you really need to do this for the time being, I have the following
comments.

To me this seems like more complicated than necessary.  There doesn't really
seem to be much value added by the eglGetSoftwareDisplay function (since we
don't actually do anything with the HDC).  I presume Chrome uses
EGL_DEFAULT_DISPLAY?

I'd suggest the following:
1) define EGL_SOFTWARE_DISPLAY_ANGLE to be some value that will never be invalid
for an HDC.
2) augment eglGetDisplay as follows:
 if (display_id == EGL_DEFAULT_DISPLAY)
     return new egl::Display((HDC)NULL, false);
  else if (display_id == EGL_SOFTWARE_DISPLAY)
     return new egl::Display((HDC)NULL, true);
  else 
     return new egl::Display((HDC)display_id, true);
3) fix eglGetDisplay, so that it doesn't keep creating displays :-).  Spec says
"Multiple calls made to eglGetDisplay with the same display - id will all return
the same EGLDisplay handle."  We should to keep a list of existing displays that
we check before creating a new one.

Daniel

http://codereview.appspot.com/4631093/diff/1/extensions/EGL_ANGLE_get_softwar...
File extensions/EGL_ANGLE_get_software_display.txt (right):

http://codereview.appspot.com/4631093/diff/1/extensions/EGL_ANGLE_get_softwar...
extensions/EGL_ANGLE_get_software_display.txt:15: John Bauman, Google Inc.
(jbauman 'at' chromium.org
missing ')'  (my internal lisp parser through off warning bells :-))

http://codereview.appspot.com/4631093/diff/1/include/EGL/eglext.h
File include/EGL/eglext.h (right):

http://codereview.appspot.com/4631093/diff/1/include/EGL/eglext.h#newcode236
include/EGL/eglext.h:236: typedef EGLBoolean (EGLAPIENTRYP
PFNEGLGETSOFTWAREDISPLAYANGLEPROC) (EGLNativeDisplayType display_id);
This returns EGLDisplay not EGLBoolean

[John Bauman, 2011-07-08 22:50:35]

On Fri, Jul 8, 2011 at 1:46 PM, <daniel@transgaming.com> wrote:

> On 2011/07/06 21:51:55, John Bauman wrote:
>
>> Hi,
>>
>
>  To me, this seems like the most straightforward way to specify that a
>>
> software
>
>> display will be used. Any suggestions?
>>
>
> In general I'd rather you use the integrated version of ANGLE+swift,
> instead of using the d3d9 interface for it (see email from Gav
> yesterday).
>

Yeah, understood. I'll try to get the chromium code into a state where
that's possible - there are currently issues with loading two different egl
libraries at the same time - but if that's not convenient or if using the
integrated version increases the download size too much I may have to use
the separated version anyway for now.

However, once there is a real benefit to using the integrated version we're
likely going to switch to that.

>
> However if you really need to do this for the time being, I have the
> following comments.
>
> To me this seems like more complicated than necessary.  There doesn't
> really seem to be much value added by the eglGetSoftwareDisplay function
> (since we don't actually do anything with the HDC).  I presume Chrome
> uses EGL_DEFAULT_DISPLAY?
>
> I'd suggest the following:
> 1) define EGL_SOFTWARE_DISPLAY_ANGLE to be some value that will never be
> invalid for an HDC.
> 2) augment eglGetDisplay as follows:
>  if (display_id == EGL_DEFAULT_DISPLAY)
>     return new egl::Display((HDC)NULL, false);
>  else if (display_id == EGL_SOFTWARE_DISPLAY)
>     return new egl::Display((HDC)NULL, true);
>  else
>     return new egl::Display((HDC)display_id, true);
> 3) fix eglGetDisplay, so that it doesn't keep creating displays :-).
> Spec says "Multiple calls made to eglGetDisplay with the same display -
> id will all return the same EGLDisplay handle."  We should to keep a
> list of existing displays that we check before creating a new one.
>
> Daniel
>
>
> http://codereview.appspot.com/**4631093/diff/1/extensions/EGL_**
>
ANGLE_get_software_display.txt<http://codereview.appspot.com/4631093/diff/1/extensions/EGL_ANGLE_get_software_display.txt>
> File extensions/EGL_ANGLE_get_**software_display.txt (right):
>
> http://codereview.appspot.com/**4631093/diff/1/extensions/EGL_**
>
ANGLE_get_software_display.**txt#newcode15<http://codereview.appspot.com/4631093/diff/1/extensions/EGL_ANGLE_get_software_display.txt#newcode15>
> extensions/EGL_ANGLE_get_**software_display.txt:15: John Bauman, Google
> Inc. (jbauman 'at' chromium.org
> missing ')'  (my internal lisp parser through off warning bells :-))
>
>
http://codereview.appspot.com/**4631093/diff/1/include/EGL/**eglext.h<http://...
> File include/EGL/eglext.h (right):
>
> http://codereview.appspot.com/**4631093/diff/1/include/EGL/**
>
eglext.h#newcode236<http://codereview.appspot.com/4631093/diff/1/include/EGL/eglext.h#newcode236>
> include/EGL/eglext.h:236: typedef EGLBoolean (EGLAPIENTRYP
> PFNEGLGETSOFTWAREDISPLAYANGLEP**ROC) (EGLNativeDisplayType display_id);
> This returns EGLDisplay not EGLBoolean
>
>
>
http://codereview.appspot.com/**4631093/<http://codereview.appspot.com/4631093/>
>

[John Bauman, 2011-07-13 01:25:16]

Ok, switching to using the new enum (set to -1 for now, as I'm pretty sure
that's invalid for an HDC). Also, made sure that we always return the same
EGLDisplay for a given display_id.

I want to push forward with this patch for now, as a bunch of the chromium code
makes assumptions about our egl devices that it will take a while to get rid of.

[dgkoch, 2011-07-13 18:07:58]

Largely looks ok.  Although I've got a question about the module loading below.

I notice that we aren't advertising the extension string.  Should we/can we? 
Ideally we'd only advertise the extension if swiftshader_d3d9.dll is available.

It might also be a good idea if validateDisplay(egl::Display *display) would
check if display exists in the DisplayMap.

http://codereview.appspot.com/4631093/diff/10001/src/libEGL/Display.cpp
File src/libEGL/Display.cpp (right):

http://codereview.appspot.com/4631093/diff/10001/src/libEGL/Display.cpp#newco...
src/libEGL/Display.cpp:114: mD3d9Module =
GetModuleHandle(TEXT("swiftshader_d3d9.dll"));
what is responsible for actually loading the swiftshader_d3d9.dll?
GetModuleHandle assumes it's already loaded.

I guess egl initialization will just fail if the module is not found?  (probably
acceptable, just want that to be clear).

[John Bauman, 2011-07-13 20:27:10]

On Wed, Jul 13, 2011 at 11:07 AM, <daniel@transgaming.com> wrote:

> Largely looks ok.  Although I've got a question about the module loading
> below.
>
> I notice that we aren't advertising the extension string.  Should we/can
> we?  Ideally we'd only advertise the extension if swiftshader_d3d9.dll
> is available.
>

I guess I advertising the extension string can't really hurt, although if
you want to take advantage of it you'd have to get a display, check the
extension string for that, get a different display, and check the extension
string on that for the real list of extensions.

>
> It might also be a good idea if validateDisplay(egl::Display *display)
> would check if display exists in the DisplayMap.
>
>
> http://codereview.appspot.com/**4631093/diff/10001/src/libEGL/**
>
Display.cpp<http://codereview.appspot.com/4631093/diff/10001/src/libEGL/Display.cpp>
> File src/libEGL/Display.cpp (right):
>
> http://codereview.appspot.com/**4631093/diff/10001/src/libEGL/**
>
Display.cpp#newcode114<http://codereview.appspot.com/4631093/diff/10001/src/libEGL/Display.cpp#newcode114>
> src/libEGL/Display.cpp:114: mD3d9Module =
> GetModuleHandle(TEXT("**swiftshader_d3d9.dll"));
> what is responsible for actually loading the swiftshader_d3d9.dll?
> GetModuleHandle assumes it's already loaded.
>
> I guess egl initialization will just fail if the module is not found?
> (probably acceptable, just want that to be clear).


Right, for chromium's purposes we'll have to load the dll in advance anyway.
Other people can think of their own solutions.

>
>
>
http://codereview.appspot.com/**4631093/<http://codereview.appspot.com/4631093/>
>

[John Bauman, 2011-07-13 22:43:02]

Ok, I've added a patch to advertise the extension string.

On 2011/07/13 18:07:58, dgkoch wrote:
> Largely looks ok.  Although I've got a question about the module loading
below.
> 
> I notice that we aren't advertising the extension string.  Should we/can we? 
> Ideally we'd only advertise the extension if swiftshader_d3d9.dll is
available.
> 
> It might also be a good idea if validateDisplay(egl::Display *display) would
> check if display exists in the DisplayMap.
The DisplayMap isn't really the best datastructure to determine whether the
display exists (it maps the wrong way) and we've survived without that for this
long, so I didn't really feel the need to add a new datastructure for it.
> 
> http://codereview.appspot.com/4631093/diff/10001/src/libEGL/Display.cpp
> File src/libEGL/Display.cpp (right):
> 
>
http://codereview.appspot.com/4631093/diff/10001/src/libEGL/Display.cpp#newco...
> src/libEGL/Display.cpp:114: mD3d9Module =
> GetModuleHandle(TEXT("swiftshader_d3d9.dll"));
> what is responsible for actually loading the swiftshader_d3d9.dll?
> GetModuleHandle assumes it's already loaded.
> 
> I guess egl initialization will just fail if the module is not found? 
(probably
> acceptable, just want that to be clear).

[dgkoch, 2011-07-14 00:22:40]

On 2011/07/13 20:27:10, John Bauman wrote:

> > I notice that we aren't advertising the extension string.  Should we/can
> > we?  Ideally we'd only advertise the extension if swiftshader_d3d9.dll
> > is available.
> >
> 
> I guess I advertising the extension string can't really hurt, although if
> you want to take advantage of it you'd have to get a display, check the
> extension string for that, get a different display, and check the extension
> string on that for the real list of extensions.

Ah right -- ye old chicken and egg problem.  Had forgotten that you needed to
pass a display into that function.  Ah well you have to do similar things with
WGL, etc in some cases.

> > src/libEGL/Display.cpp:114: mD3d9Module =
> > GetModuleHandle(TEXT("**swiftshader_d3d9.dll"));
> > what is responsible for actually loading the swiftshader_d3d9.dll?
> > GetModuleHandle assumes it's already loaded.
> >
> > I guess egl initialization will just fail if the module is not found?
> > (probably acceptable, just want that to be clear).
> 
> Right, for chromium's purposes we'll have to load the dll in advance anyway.
> Other people can think of their own solutions.

Ok. That sounds good then.

[dgkoch, 2011-07-14 00:26:42]

On 2011/07/13 22:43:02, John Bauman wrote:
> Ok, I've added a patch to advertise the extension string.

> > It might also be a good idea if validateDisplay(egl::Display *display) would
> > check if display exists in the DisplayMap.
> The DisplayMap isn't really the best datastructure to determine whether the
> display exists (it maps the wrong way) and we've survived without that for
this
> long, so I didn't really feel the need to add a new datastructure for it.

fair enough.

LGTM

[dgkoch, 2011-07-14 00:27:05]

(don't forget to bump the version on commit)