Addition of GL-virtualization layer for skia.

Implementation of a GL-virtualization layer for Skia.  This allows for environments using skia to specify a GL implementation at run-time, instead of relying on the linker to pull in the appropriate GL impl.

A new structure, GrGLInterface is exposed.  This struct contains a set of function pointers that should point to an appropriate GL implementation.

This change also removes the reliance on GLew on windows builds.

Committed:  http://code.google.com/p/skia/source/detail?r=937

[twiz1, 2011-03-07 19:37:05]

Hi Mike and Brian,

PTAL.

I composed this change to minimize the impact on the existing Ganesh code. 
There are only a few small changes here and there, but the GR_GL calling style
is preserved.

I generated the call-sites by grepping through the source-base, so only those
functions called are exported.

Jeff

[bsalomon, 2011-03-07 20:02:04]

Hi Jeff,

Would it be possible to use function pointers rather than virtuals to achieve
the indirection? I'm thinking that we could then simultaneously eliminate the
unfortunate GL_GL / GR_GLEXT distinction.  At some point the number of things
that require a getProcAddress at init time on Win32 will increase by removing
glew.

Implementing the direct version on top of the current macro/func getting stuff
seems right for now but I'd like to eliminate all that voodoo in the medium
term. In a function pointer impl the current stuff could be one way to populate
the function pointers.

If it simplifies anything, the IMG multisampling stuff can go. It doesn't work
correctly and there is no known customer for it. Only do it now if it means less
work for you, though. 

We can defer this to a follow on change but we'll want to replace GL_* #defines
(e.g. GL_STENCIL_BUFFER_BIT, GL_LESS, etc) with something defined inside Gr. It
could be a bunch of GR_GL_ defines or a giant enum in GrGLInterface. Ultimately
we want to consoliate any gl header dependence to GrGLInterface implementations.

Really minor: let's not refer to skia in the comments. We're checked into skia
but its conceivable that at some point in the future Skia/Gr could be factored
into a skia lib, a gr lib, and a shared collection/math lib. We want to at least
pretend that Gr is separable :)


On 2011/03/07 19:37:05, twiz1 wrote:
> Hi Mike and Brian,
> 
> PTAL.
> 
> I composed this change to minimize the impact on the existing Ganesh code. 
> There are only a few small changes here and there, but the GR_GL calling style
> is preserved.
> 
> I generated the call-sites by grepping through the source-base, so only those
> functions called are exported.
> 
> Jeff

[twiz1, 2011-03-07 20:18:32]

Thanks for taking a look, Brian.

On 2011/03/07 20:02:04, bsalomon wrote:
> Hi Jeff,
> 
> Would it be possible to use function pointers rather than virtuals to achieve
> the indirection? I'm thinking that we could then simultaneously eliminate the
> unfortunate GL_GL / GR_GLEXT distinction.  At some point the number of things
> that require a getProcAddress at init time on Win32 will increase by removing
> glew.

I'll upload a new version that removes the exts pointer, and exposes function
pointers instead of virtual functions shortly.

> 
> Implementing the direct version on top of the current macro/func getting stuff
> seems right for now but I'd like to eliminate all that voodoo in the medium
> term. In a function pointer impl the current stuff could be one way to
populate
> the function pointers.
> 
> If it simplifies anything, the IMG multisampling stuff can go. It doesn't work
> correctly and there is no known customer for it. Only do it now if it means
less
> work for you, though. 

Will briefly look into removing.
> 
> We can defer this to a follow on change but we'll want to replace GL_*
#defines
> (e.g. GL_STENCIL_BUFFER_BIT, GL_LESS, etc) with something defined inside Gr.
It
> could be a bunch of GR_GL_ defines or a giant enum in GrGLInterface.
Ultimately
> we want to consoliate any gl header dependence to GrGLInterface
implementations.

Yes, it is unfortunate that my implementation of GrGLInterface pulls in all of
the gl headers.  I did this because I wanted to use the various GLenum (et al)
types.

> 
> Really minor: let's not refer to skia in the comments. We're checked into skia
> but its conceivable that at some point in the future Skia/Gr could be factored
> into a skia lib, a gr lib, and a shared collection/math lib. We want to at
least
> pretend that Gr is separable :)

Will do.  I was semi-aware of the distinction from our talks, but not that use
of 'skia' should be avoided.

> 
> 
> On 2011/03/07 19:37:05, twiz1 wrote:
> > Hi Mike and Brian,
> > 
> > PTAL.
> > 
> > I composed this change to minimize the impact on the existing Ganesh code. 
> > There are only a few small changes here and there, but the GR_GL calling
style
> > is preserved.
> > 
> > I generated the call-sites by grepping through the source-base, so only
those
> > functions called are exported.
> > 
> > Jeff

[twiz1, 2011-03-11 00:23:06]

I uploaded a new version, addressing your suggestions.

Please take another look.

I removed the use of glew on windows for the gpu files.  Note that the
sample-app still links with glew because of its use in SkOsWindow_win.

Also, I have only tried compiling this on Windows.  Is there another target that
I can build to make sure I won't break the gpu path on Linux and Mac?

Jeff

On 2011/03/07 20:18:32, twiz1 wrote:
> Thanks for taking a look, Brian.
> 
> On 2011/03/07 20:02:04, bsalomon wrote:
> > Hi Jeff,
> > 
> > Would it be possible to use function pointers rather than virtuals to
achieve
> > the indirection? I'm thinking that we could then simultaneously eliminate
the
> > unfortunate GL_GL / GR_GLEXT distinction.  At some point the number of
things
> > that require a getProcAddress at init time on Win32 will increase by
removing
> > glew.
> 
> I'll upload a new version that removes the exts pointer, and exposes function
> pointers instead of virtual functions shortly.
> 
> > 
> > Implementing the direct version on top of the current macro/func getting
stuff
> > seems right for now but I'd like to eliminate all that voodoo in the medium
> > term. In a function pointer impl the current stuff could be one way to
> populate
> > the function pointers.
> > 
> > If it simplifies anything, the IMG multisampling stuff can go. It doesn't
work
> > correctly and there is no known customer for it. Only do it now if it means
> less
> > work for you, though. 
> 
> Will briefly look into removing.
> > 
> > We can defer this to a follow on change but we'll want to replace GL_*
> #defines
> > (e.g. GL_STENCIL_BUFFER_BIT, GL_LESS, etc) with something defined inside Gr.
> It
> > could be a bunch of GR_GL_ defines or a giant enum in GrGLInterface.
> Ultimately
> > we want to consoliate any gl header dependence to GrGLInterface
> implementations.
> 
> Yes, it is unfortunate that my implementation of GrGLInterface pulls in all of
> the gl headers.  I did this because I wanted to use the various GLenum (et al)
> types.
> 
> > 
> > Really minor: let's not refer to skia in the comments. We're checked into
skia
> > but its conceivable that at some point in the future Skia/Gr could be
factored
> > into a skia lib, a gr lib, and a shared collection/math lib. We want to at
> least
> > pretend that Gr is separable :)
> 
> Will do.  I was semi-aware of the distinction from our talks, but not that use
> of 'skia' should be avoided.
> 
> > 
> > 
> > On 2011/03/07 19:37:05, twiz1 wrote:
> > > Hi Mike and Brian,
> > > 
> > > PTAL.
> > > 
> > > I composed this change to minimize the impact on the existing Ganesh code.

> > > There are only a few small changes here and there, but the GR_GL calling
> style
> > > is preserved.
> > > 
> > > I generated the call-sites by grepping through the source-base, so only
> those
> > > functions called are exported.
> > > 
> > > Jeff

[bsalomon, 2011-03-11 14:29:28]

I think this is looking great. There is an xcode project for SampleApp in the
xcode dir in skia. SampleApp doesn't currently build on Linux but the gpu code
is built if you do a "make all" from the trunk of a skia checkout.

We also need to make sure it works in Chrome. You should be able to apply your
patch to third_party/skia in a chrome tree.

I'm wondering if we'll get redefinition warnings on a windows Chrome build for
the GL_* macros. I think GR_WIN32_BUILD is true there. This could force our hand
to create our own prefix GR_GL_TEXTURE0 or something to that effect. 

I think you'll have to #define GR_GL_FUNCTION_TYPE in GrGLConfig_chrome.h

It's awesome that Gr doesn't need glew with these changes. I'm happy to figure
out the wgl mojo to remove glew from SkOSWindow_Win once we have this in.

As a later step I think we should move away from the macro voodoo used to setup
GL from the platform headers. It might just be simpler to write a GL init cpp
for each platform (mac, linux, win, android, ...) rather than the current system
of setting all the macros correctly to allow "generic" code extract them.

On 2011/03/11 00:23:06, twiz1 wrote:
> I uploaded a new version, addressing your suggestions.
> 
> Please take another look.
> 
> I removed the use of glew on windows for the gpu files.  Note that the
> sample-app still links with glew because of its use in SkOsWindow_win.
> 
> Also, I have only tried compiling this on Windows.  Is there another target
that
> I can build to make sure I won't break the gpu path on Linux and Mac?
> 
> Jeff
> 
> On 2011/03/07 20:18:32, twiz1 wrote:
> > Thanks for taking a look, Brian.
> > 
> > On 2011/03/07 20:02:04, bsalomon wrote:
> > > Hi Jeff,
> > > 
> > > Would it be possible to use function pointers rather than virtuals to
> achieve
> > > the indirection? I'm thinking that we could then simultaneously eliminate
> the
> > > unfortunate GL_GL / GR_GLEXT distinction.  At some point the number of
> things
> > > that require a getProcAddress at init time on Win32 will increase by
> removing
> > > glew.
> > 
> > I'll upload a new version that removes the exts pointer, and exposes
function
> > pointers instead of virtual functions shortly.
> > 
> > > 
> > > Implementing the direct version on top of the current macro/func getting
> stuff
> > > seems right for now but I'd like to eliminate all that voodoo in the
medium
> > > term. In a function pointer impl the current stuff could be one way to
> > populate
> > > the function pointers.
> > > 
> > > If it simplifies anything, the IMG multisampling stuff can go. It doesn't
> work
> > > correctly and there is no known customer for it. Only do it now if it
means
> > less
> > > work for you, though. 
> > 
> > Will briefly look into removing.
> > > 
> > > We can defer this to a follow on change but we'll want to replace GL_*
> > #defines
> > > (e.g. GL_STENCIL_BUFFER_BIT, GL_LESS, etc) with something defined inside
Gr.
> > It
> > > could be a bunch of GR_GL_ defines or a giant enum in GrGLInterface.
> > Ultimately
> > > we want to consoliate any gl header dependence to GrGLInterface
> > implementations.
> > 
> > Yes, it is unfortunate that my implementation of GrGLInterface pulls in all
of
> > the gl headers.  I did this because I wanted to use the various GLenum (et
al)
> > types.
> > 
> > > 
> > > Really minor: let's not refer to skia in the comments. We're checked into
> skia
> > > but its conceivable that at some point in the future Skia/Gr could be
> factored
> > > into a skia lib, a gr lib, and a shared collection/math lib. We want to at
> > least
> > > pretend that Gr is separable :)
> > 
> > Will do.  I was semi-aware of the distinction from our talks, but not that
use
> > of 'skia' should be avoided.
> > 
> > > 
> > > 
> > > On 2011/03/07 19:37:05, twiz1 wrote:
> > > > Hi Mike and Brian,
> > > > 
> > > > PTAL.
> > > > 
> > > > I composed this change to minimize the impact on the existing Ganesh
code.
> 
> > > > There are only a few small changes here and there, but the GR_GL calling
> > style
> > > > is preserved.
> > > > 
> > > > I generated the call-sites by grepping through the source-base, so only
> > those
> > > > functions called are exported.
> > > > 
> > > > Jeff

[twiz1, 2011-03-14 17:50:19]

PTA(nother)L.

On 2011/03/11 14:29:28, bsalomon wrote:
> I think this is looking great. There is an xcode project for SampleApp in the
> xcode dir in skia. SampleApp doesn't currently build on Linux but the gpu code
> is built if you do a "make all" from the trunk of a skia checkout.

I've tested my changes on linux, mac and windows, as per your suggestions.

> 
> We also need to make sure it works in Chrome. You should be able to apply your
> patch to third_party/skia in a chrome tree.

I've patched my change into chrome manually, and I'm about to submit a try bot
test, just to make sure that it's not going to break other Chrome configs.

> 
> I'm wondering if we'll get redefinition warnings on a windows Chrome build for
> the GL_* macros. I think GR_WIN32_BUILD is true there. This could force our
hand
> to create our own prefix GR_GL_TEXTURE0 or something to that effect. 

I added a new macro, EMIT_GL_HEADERS to control this behaviour, and prevent the
symbol clashes.

> 
> I think you'll have to #define GR_GL_FUNCTION_TYPE in GrGLConfig_chrome.h

Because the gles headers that chrome links with do not export __stdcall
functions, the macro is not required.

> 
> It's awesome that Gr doesn't need glew with these changes. I'm happy to figure
> out the wgl mojo to remove glew from SkOSWindow_Win once we have this in.
> 
> As a later step I think we should move away from the macro voodoo used to
setup
> GL from the platform headers. It might just be simpler to write a GL init cpp
> for each platform (mac, linux, win, android, ...) rather than the current
system
> of setting all the macros correctly to allow "generic" code extract them.

Yeah, I agree.  At this point, I'd just like to get the system in, so we can
iterate on it.

> 
> On 2011/03/11 00:23:06, twiz1 wrote:
> > I uploaded a new version, addressing your suggestions.
> > 
> > Please take another look.
> > 
> > I removed the use of glew on windows for the gpu files.  Note that the
> > sample-app still links with glew because of its use in SkOsWindow_win.
> > 
> > Also, I have only tried compiling this on Windows.  Is there another target
> that
> > I can build to make sure I won't break the gpu path on Linux and Mac?
> > 
> > Jeff
> > 
> > On 2011/03/07 20:18:32, twiz1 wrote:
> > > Thanks for taking a look, Brian.
> > > 
> > > On 2011/03/07 20:02:04, bsalomon wrote:
> > > > Hi Jeff,
> > > > 
> > > > Would it be possible to use function pointers rather than virtuals to
> > achieve
> > > > the indirection? I'm thinking that we could then simultaneously
eliminate
> > the
> > > > unfortunate GL_GL / GR_GLEXT distinction.  At some point the number of
> > things
> > > > that require a getProcAddress at init time on Win32 will increase by
> > removing
> > > > glew.
> > > 
> > > I'll upload a new version that removes the exts pointer, and exposes
> function
> > > pointers instead of virtual functions shortly.
> > > 
> > > > 
> > > > Implementing the direct version on top of the current macro/func getting
> > stuff
> > > > seems right for now but I'd like to eliminate all that voodoo in the
> medium
> > > > term. In a function pointer impl the current stuff could be one way to
> > > populate
> > > > the function pointers.
> > > > 
> > > > If it simplifies anything, the IMG multisampling stuff can go. It
doesn't
> > work
> > > > correctly and there is no known customer for it. Only do it now if it
> means
> > > less
> > > > work for you, though. 
> > > 
> > > Will briefly look into removing.
> > > > 
> > > > We can defer this to a follow on change but we'll want to replace GL_*
> > > #defines
> > > > (e.g. GL_STENCIL_BUFFER_BIT, GL_LESS, etc) with something defined inside
> Gr.
> > > It
> > > > could be a bunch of GR_GL_ defines or a giant enum in GrGLInterface.
> > > Ultimately
> > > > we want to consoliate any gl header dependence to GrGLInterface
> > > implementations.
> > > 
> > > Yes, it is unfortunate that my implementation of GrGLInterface pulls in
all
> of
> > > the gl headers.  I did this because I wanted to use the various GLenum (et
> al)
> > > types.
> > > 
> > > > 
> > > > Really minor: let's not refer to skia in the comments. We're checked
into
> > skia
> > > > but its conceivable that at some point in the future Skia/Gr could be
> > factored
> > > > into a skia lib, a gr lib, and a shared collection/math lib. We want to
at
> > > least
> > > > pretend that Gr is separable :)
> > > 
> > > Will do.  I was semi-aware of the distinction from our talks, but not that
> use
> > > of 'skia' should be avoided.
> > > 
> > > > 
> > > > 
> > > > On 2011/03/07 19:37:05, twiz1 wrote:
> > > > > Hi Mike and Brian,
> > > > > 
> > > > > PTAL.
> > > > > 
> > > > > I composed this change to minimize the impact on the existing Ganesh
> code.
> > 
> > > > > There are only a few small changes here and there, but the GR_GL
calling
> > > style
> > > > > is preserved.
> > > > > 
> > > > > I generated the call-sites by grepping through the source-base, so
only
> > > those
> > > > > functions called are exported.
> > > > > 
> > > > > Jeff

[bsalomon, 2011-03-14 18:57:11]

LGTM

On Mon, Mar 14, 2011 at 1:50 PM, <twiz@google.com> wrote:

> PTA(nother)L.
>
>
> On 2011/03/11 14:29:28, bsalomon wrote:
>
>> I think this is looking great. There is an xcode project for SampleApp
>>
> in the
>
>> xcode dir in skia. SampleApp doesn't currently build on Linux but the
>>
> gpu code
>
>> is built if you do a "make all" from the trunk of a skia checkout.
>>
>
> I've tested my changes on linux, mac and windows, as per your
> suggestions.
>
>
>
>  We also need to make sure it works in Chrome. You should be able to
>>
> apply your
>
>> patch to third_party/skia in a chrome tree.
>>
>
> I've patched my change into chrome manually, and I'm about to submit a
> try bot test, just to make sure that it's not going to break other
> Chrome configs.
>
>
>
>  I'm wondering if we'll get redefinition warnings on a windows Chrome
>>
> build for
>
>> the GL_* macros. I think GR_WIN32_BUILD is true there. This could
>>
> force our hand
>
>> to create our own prefix GR_GL_TEXTURE0 or something to that effect.
>>
>
> I added a new macro, EMIT_GL_HEADERS to control this behaviour, and
> prevent the symbol clashes.
>
>
>
>  I think you'll have to #define GR_GL_FUNCTION_TYPE in
>>
> GrGLConfig_chrome.h
>
> Because the gles headers that chrome links with do not export __stdcall
> functions, the macro is not required.
>
>
>
>  It's awesome that Gr doesn't need glew with these changes. I'm happy
>>
> to figure
>
>> out the wgl mojo to remove glew from SkOSWindow_Win once we have this
>>
> in.
>
>  As a later step I think we should move away from the macro voodoo used
>>
> to setup
>
>> GL from the platform headers. It might just be simpler to write a GL
>>
> init cpp
>
>> for each platform (mac, linux, win, android, ...) rather than the
>>
> current system
>
>> of setting all the macros correctly to allow "generic" code extract
>>
> them.
>
> Yeah, I agree.  At this point, I'd just like to get the system in, so we
> can iterate on it.
>
>
>
>  On 2011/03/11 00:23:06, twiz1 wrote:
>> > I uploaded a new version, addressing your suggestions.
>> >
>> > Please take another look.
>> >
>> > I removed the use of glew on windows for the gpu files.  Note that
>>
> the
>
>> > sample-app still links with glew because of its use in
>>
> SkOsWindow_win.
>
>> >
>> > Also, I have only tried compiling this on Windows.  Is there another
>>
> target
>
>> that
>> > I can build to make sure I won't break the gpu path on Linux and
>>
> Mac?
>
>> >
>> > Jeff
>> >
>> > On 2011/03/07 20:18:32, twiz1 wrote:
>> > > Thanks for taking a look, Brian.
>> > >
>> > > On 2011/03/07 20:02:04, bsalomon wrote:
>> > > > Hi Jeff,
>> > > >
>> > > > Would it be possible to use function pointers rather than
>>
> virtuals to
>
>> > achieve
>> > > > the indirection? I'm thinking that we could then simultaneously
>>
> eliminate
>
>> > the
>> > > > unfortunate GL_GL / GR_GLEXT distinction.  At some point the
>>
> number of
>
>> > things
>> > > > that require a getProcAddress at init time on Win32 will
>>
> increase by
>
>> > removing
>>
>> > > > glew.
>> > >
>> > > I'll upload a new version that removes the exts pointer, and
>>
> exposes
>
>> function
>> > > pointers instead of virtual functions shortly.
>> > >
>> > > >
>> > > > Implementing the direct version on top of the current macro/func
>>
> getting
>
>> > stuff
>> > > > seems right for now but I'd like to eliminate all that voodoo in
>>
> the
>
>> medium
>> > > > term. In a function pointer impl the current stuff could be one
>>
> way to
>
>> > > populate
>> > > > the function pointers.
>> > > >
>> > > > If it simplifies anything, the IMG multisampling stuff can go.
>>
> It doesn't
>
>> > work
>> > > > correctly and there is no known customer for it. Only do it now
>>
> if it
>
>> means
>> > > less
>> > > > work for you, though.
>> > >
>> > > Will briefly look into removing.
>> > > >
>> > > > We can defer this to a follow on change but we'll want to
>>
> replace GL_*
>
>> > > #defines
>> > > > (e.g. GL_STENCIL_BUFFER_BIT, GL_LESS, etc) with something
>>
> defined inside
>
>> Gr.
>> > > It
>> > > > could be a bunch of GR_GL_ defines or a giant enum in
>>
> GrGLInterface.
>
>> > > Ultimately
>> > > > we want to consoliate any gl header dependence to GrGLInterface
>> > > implementations.
>> > >
>> > > Yes, it is unfortunate that my implementation of GrGLInterface
>>
> pulls in all
>
>> of
>> > > the gl headers.  I did this because I wanted to use the various
>>
> GLenum (et
>
>> al)
>> > > types.
>> > >
>> > > >
>> > > > Really minor: let's not refer to skia in the comments. We're
>>
> checked into
>
>> > skia
>> > > > but its conceivable that at some point in the future Skia/Gr
>>
> could be
>
>> > factored
>> > > > into a skia lib, a gr lib, and a shared collection/math lib. We
>>
> want to at
>
>> > > least
>> > > > pretend that Gr is separable :)
>> > >
>> > > Will do.  I was semi-aware of the distinction from our talks, but
>>
> not that
>
>> use
>> > > of 'skia' should be avoided.
>> > >
>> > > >
>> > > >
>> > > > On 2011/03/07 19:37:05, twiz1 wrote:
>> > > > > Hi Mike and Brian,
>> > > > >
>> > > > > PTAL.
>> > > > >
>> > > > > I composed this change to minimize the impact on the existing
>>
> Ganesh
>
>> code.
>> >
>> > > > > There are only a few small changes here and there, but the
>>
> GR_GL calling
>
>> > > style
>> > > > > is preserved.
>> > > > >
>> > > > > I generated the call-sites by grepping through the
>>
> source-base, so only
>
>> > > those
>> > > > > functions called are exported.
>> > > > >
>> > > > > Jeff
>>
>
>
>
> http://codereview.appspot.com/4254059/
>