First version of ANGLE GL interface

First version of ANGLE GL interface

For this delivery I propose not delivering the .gyp files - since they need a lot of work (more later). I have added a new ANGLE GL interface in src/gpu/gl/angle and modified the interface validation test, bench, gm and SampleApp to use it. Everything is guarded behind a SK_ANGLE #define so w/o the .gyps no part of my changes will be compiled. Note that I have set it up so we can switch between normal native OpenGL and ANGLE merely by specifying "--angle" to gm and SampleApp (provided ANGLE is compiled in).

Elliot - here all all the problems I know of with the .gyps

1) although the SK_MESA flag is only added in common.gypi I found this wasn't sufficient and also needed to add a 'defines' block to common_conditions.gypi.
2) I don't know how to attach ANGLE to Skia correctly so currently have hard paths to the ANGLE libs I (manually) copied to third_party/externals/angle.
3) For some reason the 'libraries' and 'include_dirs' settings in gpu.gyp weren't propagating to the executables so I replicated these blocks in bench.gypi, tests.gyp, gm.gyp and SampleApp.gyp.

[robertphillips, 2012-03-27 20:47:59]

http://codereview.appspot.com/5940046/diff/1/samplecode/SampleDegenerateTwoPt...
File samplecode/SampleDegenerateTwoPtRadials.cpp (left):

http://codereview.appspot.com/5940046/diff/1/samplecode/SampleDegenerateTwoPt...
samplecode/SampleDegenerateTwoPtRadials.cpp:69: SkRect rect;
This just fixes a Windows compiler complaint - unused variable

http://codereview.appspot.com/5940046/diff/1/third_party/glu/libtess/sweep.c
File third_party/glu/libtess/sweep.c (left):

http://codereview.appspot.com/5940046/diff/1/third_party/glu/libtess/sweep.c#...
third_party/glu/libtess/sweep.c:469: weights[1] = 0.5 * t1 / (t1 + t2);
These two changes silence a compiler complaint on Windows - conversion from
double to float

[robertphillips, 2012-03-28 13:05:34]

Brian - I have upload a fix for the ANGLE constant issue.

[bsalomon, 2012-03-28 14:06:11]

Overall LGTM. I'll leave the gyp / dependency issues to you and epoger to sort
out.

http://codereview.appspot.com/5940046/diff/1/gyp/common_conditions.gypi
File gyp/common_conditions.gypi (right):

http://codereview.appspot.com/5940046/diff/1/gyp/common_conditions.gypi#newco...
gyp/common_conditions.gypi:28: 'conditions': [
some weird tab issue here

http://codereview.appspot.com/5940046/diff/1/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

http://codereview.appspot.com/5940046/diff/1/gyp/common_variables.gypi#newcode64
gyp/common_variables.gypi:64: 'skia_angle%': 1,
tabs?

http://codereview.appspot.com/5940046/diff/1/gyp/common_variables.gypi#newcode74
gyp/common_variables.gypi:74: 'skia_angle%': '<(skia_angle)',
tabs?

http://codereview.appspot.com/5940046/diff/1/third_party/glu/libtess/sweep.c
File third_party/glu/libtess/sweep.c (left):

http://codereview.appspot.com/5940046/diff/1/third_party/glu/libtess/sweep.c#...
third_party/glu/libtess/sweep.c:469: weights[1] = 0.5 * t1 / (t1 + t2);
On 2012/03/27 20:47:59, robertphillips wrote:
> These two changes silence a compiler complaint on Windows - conversion from
> double to float

I'm ever so slightly leery of doing this since this file is external.

http://codereview.appspot.com/5940046/diff/3001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

http://codereview.appspot.com/5940046/diff/3001/gm/gmmain.cpp#newcode641
gm/gmmain.cpp:641: SkDebugf("    --angle: use ANGLE backend on Windows.\n");
Let's add the : to match and mesagl as well for consistency.

http://codereview.appspot.com/5940046/diff/3001/include/views/SkOSWindow_Win.h
File include/views/SkOSWindow_Win.h (right):

http://codereview.appspot.com/5940046/diff/3001/include/views/SkOSWindow_Win....
include/views/SkOSWindow_Win.h:71: angle::EGLDisplay          fDisplay;
supernit: can we keep the alignment of the member var names?

http://codereview.appspot.com/5940046/diff/3001/src/gpu/gl/angle/GrGLCreateAN...
File src/gpu/gl/angle/GrGLCreateANGLEInterface.cpp (right):

http://codereview.appspot.com/5940046/diff/3001/src/gpu/gl/angle/GrGLCreateAN...
src/gpu/gl/angle/GrGLCreateANGLEInterface.cpp:123: #if 0
Curious why we have #if 0?  Does angle not define the functions? Could we call
some eglGetProcAddress type function in case they add it in the future?

[epoger, 2012-03-28 14:45:52]

non-gyp files LGTM

I agree with Brian that the non-gyp files are OK (modulo a few minor issues
below)

Please separate the gyp files from this CL and send me that one separately...
then we can put on our hip-waders and fix the gyp.

http://codereview.appspot.com/5940046/diff/3001/samplecode/SampleApp.h
File samplecode/SampleApp.h (right):

http://codereview.appspot.com/5940046/diff/3001/samplecode/SampleApp.h#newcode51
samplecode/SampleApp.h:51: virtual void init(SampleWindow* win, bool
useAltContext) = 0;
please document useAltContext parameter

http://codereview.appspot.com/5940046/diff/3001/samplecode/SampleDegenerateTw...
File samplecode/SampleDegenerateTwoPtRadials.cpp (left):

http://codereview.appspot.com/5940046/diff/3001/samplecode/SampleDegenerateTw...
samplecode/SampleDegenerateTwoPtRadials.cpp:69: SkRect rect;
In the future, I would prefer that unrelated cleanup be done separately from
already-large CLs.  But I don't know if that matches the skia-dev
hive-mindset...

http://codereview.appspot.com/5940046/diff/3001/src/gpu/gl/angle/SkANGLEGLCon...
File src/gpu/gl/angle/SkANGLEGLContext.cpp (right):

http://codereview.appspot.com/5940046/diff/3001/src/gpu/gl/angle/SkANGLEGLCon...
src/gpu/gl/angle/SkANGLEGLContext.cpp:11: //using namespace angle;
why this commented-out line?

http://codereview.appspot.com/5940046/diff/3001/src/views/win/SkOSWindow_win.cpp
File src/views/win/SkOSWindow_win.cpp (right):

http://codereview.appspot.com/5940046/diff/3001/src/views/win/SkOSWindow_win....
src/views/win/SkOSWindow_win.cpp:400: angle::EGLContext* eglContext,
angle::EGLSurface* eglSurface) {
strange tab-alignment

http://codereview.appspot.com/5940046/diff/3001/src/views/win/SkOSWindow_win....
src/views/win/SkOSWindow_win.cpp:418: //       EGL_SAMPLE_BUFFERS, 1,
why the commented-out line? please add a few words to the commented-out line
explaining why it's there (or just remove it, whichever you prefer)

http://codereview.appspot.com/5940046/diff/3001/src/views/win/SkOSWindow_win....
src/views/win/SkOSWindow_win.cpp:450: surface =
angle::eglCreateWindowSurface(display, config, (EGLNativeWindowType)hWnd,
surfaceAttribList);
>100 characters

http://codereview.appspot.com/5940046/diff/3001/third_party/glu/libtess/sweep.c
File third_party/glu/libtess/sweep.c (right):

http://codereview.appspot.com/5940046/diff/3001/third_party/glu/libtess/sweep...
third_party/glu/libtess/sweep.c:468: weights[0] = (GLfloat)(0.5 * t2 / (t1 +
t2));
blah blah blah "separate CL next time" blah blah blah

[robertphillips, 2012-03-28 15:31:42]

So ... what should we do w.r.t. the sweep.c change? As a far background task I
am trying to get the Windows compilation warning free.

[robertphillips, 2012-03-28 15:35:42]

http://codereview.appspot.com/5940046/diff/3001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

http://codereview.appspot.com/5940046/diff/3001/gm/gmmain.cpp#newcode641
gm/gmmain.cpp:641: SkDebugf("    --angle: use ANGLE backend on Windows.\n");
On 2012/03/28 14:06:12, bsalomon wrote:
> Let's add the : to match and mesagl as well for consistency.

Done.

http://codereview.appspot.com/5940046/diff/3001/include/views/SkOSWindow_Win.h
File include/views/SkOSWindow_Win.h (right):

http://codereview.appspot.com/5940046/diff/3001/include/views/SkOSWindow_Win....
include/views/SkOSWindow_Win.h:71: angle::EGLDisplay          fDisplay;
On 2012/03/28 14:06:12, bsalomon wrote:
> supernit: can we keep the alignment of the member var names?

Done.

http://codereview.appspot.com/5940046/diff/3001/samplecode/SampleApp.h
File samplecode/SampleApp.h (right):

http://codereview.appspot.com/5940046/diff/3001/samplecode/SampleApp.h#newcode51
samplecode/SampleApp.h:51: virtual void init(SampleWindow* win, bool
useAltContext) = 0;
On 2012/03/28 14:45:53, epoger wrote:
> please document useAltContext parameter

I would like to hold off on doing so since I hope to remove the useAltContext
parameter in the next delivery.

http://codereview.appspot.com/5940046/diff/3001/src/gpu/gl/angle/GrGLCreateAN...
File src/gpu/gl/angle/GrGLCreateANGLEInterface.cpp (right):

http://codereview.appspot.com/5940046/diff/3001/src/gpu/gl/angle/GrGLCreateAN...
src/gpu/gl/angle/GrGLCreateANGLEInterface.cpp:123: #if 0
On 2012/03/28 14:06:12, bsalomon wrote:
> Curious why we have #if 0?  Does angle not define the functions? Could we call
> some eglGetProcAddress type function in case they add it in the future?

Done.

http://codereview.appspot.com/5940046/diff/3001/src/gpu/gl/angle/GrGLCreateAN...
src/gpu/gl/angle/GrGLCreateANGLEInterface.cpp:123: #if 0
On 2012/03/28 14:06:12, bsalomon wrote:
> Curious why we have #if 0?  Does angle not define the functions? Could we call
> some eglGetProcAddress type function in case they add it in the future?

Done. angle declares the functions (in glext.h) but never defines them.

http://codereview.appspot.com/5940046/diff/3001/src/gpu/gl/angle/SkANGLEGLCon...
File src/gpu/gl/angle/SkANGLEGLContext.cpp (right):

http://codereview.appspot.com/5940046/diff/3001/src/gpu/gl/angle/SkANGLEGLCon...
src/gpu/gl/angle/SkANGLEGLContext.cpp:11: //using namespace angle;
On 2012/03/28 14:45:53, epoger wrote:
> why this commented-out line?

Done.

http://codereview.appspot.com/5940046/diff/3001/src/views/win/SkOSWindow_win.cpp
File src/views/win/SkOSWindow_win.cpp (right):

http://codereview.appspot.com/5940046/diff/3001/src/views/win/SkOSWindow_win....
src/views/win/SkOSWindow_win.cpp:400: angle::EGLContext* eglContext,
angle::EGLSurface* eglSurface) {
On 2012/03/28 14:45:53, epoger wrote:
> strange tab-alignment

Done.

http://codereview.appspot.com/5940046/diff/3001/src/views/win/SkOSWindow_win....
src/views/win/SkOSWindow_win.cpp:418: //       EGL_SAMPLE_BUFFERS, 1,
On 2012/03/28 14:45:53, epoger wrote:
> why the commented-out line? please add a few words to the commented-out line
> explaining why it's there (or just remove it, whichever you prefer)

Done.

http://codereview.appspot.com/5940046/diff/3001/src/views/win/SkOSWindow_win....
src/views/win/SkOSWindow_win.cpp:450: surface =
angle::eglCreateWindowSurface(display, config, (EGLNativeWindowType)hWnd,
surfaceAttribList);
On 2012/03/28 14:45:53, epoger wrote:
> >100 characters

Done.

[robertphillips, 2012-03-28 17:05:48]

committed as r3519

[epoger, 2012-03-28 17:16:54]

On 2012/03/28 15:31:42, robertphillips wrote:
> So ... what should we do w.r.t. the sweep.c change? As a far background task I
> am trying to get the Windows compilation warning free.

I see that, when you committed the CL, you modified
http://code.google.com/p/skia/source/browse/trunk/third_party/glu/README.skia . 
That seems like a good start.  (Although the CL you landed does not match the
most recent version uploaded to the code review tool, so that change does not
appear here.  It would be better to land the same set of changes that were last
uploaded to the code review tool.)

I think it would be good to follow a pattern like this when we make changes in
third_party (but I don't know if we have consensus on this):

#ifdef SKIA_VERSION_MAJOR
  // eliminate a compiler warning
  weights[0] = (GLfloat)(0.5 * t2 / (t1 + t2));
  weights[1] = (GLfloat)(0.5 * t1 / (t1 + t2));
#else
  weights[0] = 0.5 * t2 / (t1 + t2);
  weights[1] = 0.5 * t1 / (t1 + t2);
#endif

[epoger, 2012-03-28 17:18:01]

On 2012/03/28 17:16:54, epoger wrote:
> #ifdef SKIA_VERSION_MAJOR

Actually, I guess SKIA_VERSION_MAJOR wouldn't be defined in there.  But you know
what I mean. :-)

[robertphillips, 2012-03-28 17:26:18]

So ... what do we want to do? It appears that the way we are tracking changes to
the glu/libtess project is just recording them in README.skia. The pro of the
#ifdef form is that it preserves all of the old code, the con is that it could
quickly get a bit cumbersome.

[bsalomon, 2012-03-28 17:44:36]

On 2012/03/28 17:26:18, robertphillips wrote:
> So ... what do we want to do? It appears that the way we are tracking changes
to
> the glu/libtess project is just recording them in README.skia. The pro of the
> #ifdef form is that it preserves all of the old code, the con is that it could
> quickly get a bit cumbersome.

IMO documenting it in the README.skia is sufficient.