Added an application framework for demos. Ported hello-triangle example in Op...

Added an application framework for demos. Ported hello-triangle example in OpenGL ES book to use the application framework.
BUG=26099
TEST=Try running hello_triangle executable, you should see a red triangle.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=35500

[greggman, 2009-12-11 07:24:04]

LGTM from my part.

But someone other than me or vangelis needs to find out that checking these in
is okay. I believe they if you tell legal that you re-wrote the samples and are
just giving some credit they'll probably approve it but IANAL

http://codereview.chromium.org/465051/diff/5009/9001
File gpu/gpu_demos/app_framework/application.cc (right):

http://codereview.chromium.org/465051/diff/5009/9001#newcode75
gpu/gpu_demos/app_framework/application.cc:75: Draw();
This might have no place here but if it was me I'd pass
"elapsed_time_since_last_draw_in_seconds" to Draw. Any app that wants to be
frame rate independent will need that info. It would be nice to abstract it out
here rather than have each app need to have code to do that.

What do you think?

http://codereview.chromium.org/465051/diff/5009/9001#newcode135
gpu/gpu_demos/app_framework/application.cc:135: NativeWindowHandle
Application::CreateNativeWindow() {
This is a private function so it probably does not matter but do you want to
call this Initialize instead of CreateNativeWindow?  NaCl and Pepper plugins
will not create a window but will instead be passed some kind of context. (mac
plugins do not get their own window)

[darin (slow to review), 2009-12-11 17:57:44]

how about just gpu/demos?  why the extra gpu_ in the file path?

[alokp, 2009-12-11 18:22:54]

Thanks Gregg. Do we have a legal for the chrome team who I can consult. Probably
Darin would know.

http://codereview.chromium.org/465051/diff/5009/9001
File gpu/gpu_demos/app_framework/application.cc (right):

http://codereview.chromium.org/465051/diff/5009/9001#newcode75
gpu/gpu_demos/app_framework/application.cc:75: Draw();
It is a good idea. I liked o3d's render event. However I would like to implement
it when I write such a demo. It will be easier to test.

http://codereview.chromium.org/465051/diff/5009/9001#newcode135
gpu/gpu_demos/app_framework/application.cc:135: NativeWindowHandle
Application::CreateNativeWindow() {
I will revisit it when we are ready to support pepper plugins. As you noted, it
is a private function and should not affect anything else.

[alokp, 2009-12-11 18:24:41]

On 2009/12/11 17:57:44, darin wrote:
> how about just gpu/demos?  why the extra gpu_ in the file path?

No particular reason. There is another directory named gpu/gpu_plugin. I was
trying to follow that pattern. I like demos better. I can change it if you want.

[darin (slow to review), 2009-12-11 18:45:46]

we should also change gpu/gpu_plugin to just gpu/plugin.  that would be more
consistent with the chromium codebase.  doesn't have to be done in this cl,
but it would be nice if someone would take on the task for consistency sake.
-darin

On Fri, Dec 11, 2009 at 10:24 AM, <alokp@chromium.org> wrote:

> On 2009/12/11 17:57:44, darin wrote:
>
>> how about just gpu/demos?  why the extra gpu_ in the file path?
>>
>
> No particular reason. There is another directory named gpu/gpu_plugin. I
> was
> trying to follow that pattern. I like demos better. I can change it if you
> want.
>
>
> http://codereview.chromium.org/465051
>

[Vangelis Kokkevis, 2009-12-11 19:15:04]

http://codereview.chromium.org/465051/diff/5009/9001
File gpu/gpu_demos/app_framework/application.cc (right):

http://codereview.chromium.org/465051/diff/5009/9001#newcode1
gpu/gpu_demos/app_framework/application.cc:1: // Copyright (c) 2006-2009 The
Chromium Authors. All rights reserved.
There's a lot of windows-specific code in this file.  We should probably
surround it by an 

#if PLATFORM(WIN_OS)

Longer term it would be ideal to have mac and linux versions of this as well but
given that the GPU process is not running on those OS's we can punt on it for
now.

[alokp, 2009-12-11 19:17:42]

changed gpu/gpu_demos -> gpu/demos

[vangelis, 2009-12-11 19:23:33]

Darin,
  Who should we consult with on the copyright issue here.  The sample code
is taken from an OGL ES book and it states that fact very clearly at the
header.  Is that enough?

Vangelis


On Fri, Dec 11, 2009 at 10:45 AM, Darin Fisher <darin@chromium.org> wrote:

> we should also change gpu/gpu_plugin to just gpu/plugin.  that would be
> more consistent with the chromium codebase.  doesn't have to be done in this
> cl, but it would be nice if someone would take on the task for consistency
> sake.
> -darin
>
>
> On Fri, Dec 11, 2009 at 10:24 AM, <alokp@chromium.org> wrote:
>
>> On 2009/12/11 17:57:44, darin wrote:
>>
>>> how about just gpu/demos?  why the extra gpu_ in the file path?
>>>
>>
>> No particular reason. There is another directory named gpu/gpu_plugin. I
>> was
>> trying to follow that pattern. I like demos better. I can change it if you
>> want.
>>
>>
>> http://codereview.chromium.org/465051
>>
>
>

[alokp, 2009-12-11 19:24:35]

http://codereview.chromium.org/465051/diff/5009/9001
File gpu/gpu_demos/app_framework/application.cc (right):

http://codereview.chromium.org/465051/diff/5009/9001#newcode1
gpu/gpu_demos/app_framework/application.cc:1: // Copyright (c) 2006-2009 The
Chromium Authors. All rights reserved.
On 2009/12/11 19:15:04, Vangelis Kokkevis wrote:
> There's a lot of windows-specific code in this file.  We should probably
> surround it by an 
> 
> #if PLATFORM(WIN_OS)
> 
> Longer term it would be ideal to have mac and linux versions of this as well
but
> given that the GPU process is not running on those OS's we can punt on it for
> now.
> 

Done.

[vangelis, 2009-12-17 21:17:42]

Now that we got the OK to check that code somewhere we need to figure out
where it goes.  It currently serves a test for Command Buffers but arguably
over time it will be more useful as a NaCl example.  Does Chromium currently
contain any stand-alone unit tests?  If so we could re-structure the demos
as unit tests of the GPU process and have them run by the buildbots.
Alternatively, we can check them in to the chromium source tree for now and
when we have the NaCl demos codesite project move them over there.  Darin,
any advice?

Vangelis


On Fri, Dec 11, 2009 at 10:45 AM, Darin Fisher <darin@chromium.org> wrote:

> we should also change gpu/gpu_plugin to just gpu/plugin.  that would be
> more consistent with the chromium codebase.  doesn't have to be done in this
> cl, but it would be nice if someone would take on the task for consistency
> sake.
> -darin
>
>
> On Fri, Dec 11, 2009 at 10:24 AM, <alokp@chromium.org> wrote:
>
>> On 2009/12/11 17:57:44, darin wrote:
>>
>>> how about just gpu/demos?  why the extra gpu_ in the file path?
>>>
>>
>> No particular reason. There is another directory named gpu/gpu_plugin. I
>> was
>> trying to follow that pattern. I like demos better. I can change it if you
>> want.
>>
>>
>> http://codereview.chromium.org/465051
>>
>
>

[darin (slow to review), 2009-12-17 22:11:04]

+1 for more unit tests.  a new gtest executable for gpu specific unit tests
is probably a good idea.

if you are in the "base" module then you add to base_unittests, if you are
in the "net" module then you add to net_unittests, etc.  so, it makes sense
for the "gpu" module to have a gpu_unittests executable.

-darin



On Thu, Dec 17, 2009 at 1:17 PM, Vangelis Kokkevis <vangelis@google.com>wrote:

> Now that we got the OK to check that code somewhere we need to figure out
> where it goes.  It currently serves a test for Command Buffers but arguably
> over time it will be more useful as a NaCl example.  Does Chromium currently
> contain any stand-alone unit tests?  If so we could re-structure the demos
> as unit tests of the GPU process and have them run by the buildbots.
> Alternatively, we can check them in to the chromium source tree for now and
> when we have the NaCl demos codesite project move them over there.  Darin,
> any advice?
>
> Vangelis
>
>
> On Fri, Dec 11, 2009 at 10:45 AM, Darin Fisher <darin@chromium.org> wrote:
>
>> we should also change gpu/gpu_plugin to just gpu/plugin.  that would be
>> more consistent with the chromium codebase.  doesn't have to be done in this
>> cl, but it would be nice if someone would take on the task for consistency
>> sake.
>> -darin
>>
>>
>> On Fri, Dec 11, 2009 at 10:24 AM, <alokp@chromium.org> wrote:
>>
>>> On 2009/12/11 17:57:44, darin wrote:
>>>
>>>> how about just gpu/demos?  why the extra gpu_ in the file path?
>>>>
>>>
>>> No particular reason. There is another directory named gpu/gpu_plugin. I
>>> was
>>> trying to follow that pattern. I like demos better. I can change it if
>>> you want.
>>>
>>>
>>> http://codereview.chromium.org/465051
>>>
>>
>>
>

[greggman, 2009-12-17 22:54:50]

More tests = good but I'm not sure how these would be unit tests. What
gunit macros would they use to test?


On Thu, Dec 17, 2009 at 1:17 PM, Vangelis Kokkevis <vangelis@google.com> wrote:
> Now that we got the OK to check that code somewhere we need to figure out
> where it goes.  It currently serves a test for Command Buffers but arguably
> over time it will be more useful as a NaCl example.  Does Chromium currently
> contain any stand-alone unit tests?  If so we could re-structure the demos
> as unit tests of the GPU process and have them run by the buildbots.
> Alternatively, we can check them in to the chromium source tree for now and
> when we have the NaCl demos codesite project move them over there.  Darin,
> any advice?
>
> Vangelis
>
>
> On Fri, Dec 11, 2009 at 10:45 AM, Darin Fisher <darin@chromium.org> wrote:
>>
>> we should also change gpu/gpu_plugin to just gpu/plugin.  that would be
>> more consistent with the chromium codebase.  doesn't have to be done in this
>> cl, but it would be nice if someone would take on the task for consistency
>> sake.
>> -darin
>>
>> On Fri, Dec 11, 2009 at 10:24 AM, <alokp@chromium.org> wrote:
>>>
>>> On 2009/12/11 17:57:44, darin wrote:
>>>>
>>>> how about just gpu/demos?  why the extra gpu_ in the file path?
>>>
>>> No particular reason. There is another directory named gpu/gpu_plugin. I
>>> was
>>> trying to follow that pattern. I like demos better. I can change it if
>>> you want.
>>>
>>> http://codereview.chromium.org/465051
>>
>
>

[vangelis, 2009-12-17 22:56:50]

On Thu, Dec 17, 2009 at 2:54 PM, Gregg Tavares <gman@chromium.org> wrote:

> More tests = good but I'm not sure how these would be unit tests. What
> gunit macros would they use to test?
>

Check for pixel values after reading back the contents of the frame buffer?



>
>
> On Thu, Dec 17, 2009 at 1:17 PM, Vangelis Kokkevis <vangelis@google.com>
> wrote:
> > Now that we got the OK to check that code somewhere we need to figure out
> > where it goes.  It currently serves a test for Command Buffers but
> arguably
> > over time it will be more useful as a NaCl example.  Does Chromium
> currently
> > contain any stand-alone unit tests?  If so we could re-structure the
> demos
> > as unit tests of the GPU process and have them run by the buildbots.
> > Alternatively, we can check them in to the chromium source tree for now
> and
> > when we have the NaCl demos codesite project move them over there.
> Darin,
> > any advice?
> >
> > Vangelis
> >
> >
> > On Fri, Dec 11, 2009 at 10:45 AM, Darin Fisher <darin@chromium.org>
> wrote:
> >>
> >> we should also change gpu/gpu_plugin to just gpu/plugin.  that would be
> >> more consistent with the chromium codebase.  doesn't have to be done in
> this
> >> cl, but it would be nice if someone would take on the task for
> consistency
> >> sake.
> >> -darin
> >>
> >> On Fri, Dec 11, 2009 at 10:24 AM, <alokp@chromium.org> wrote:
> >>>
> >>> On 2009/12/11 17:57:44, darin wrote:
> >>>>
> >>>> how about just gpu/demos?  why the extra gpu_ in the file path?
> >>>
> >>> No particular reason. There is another directory named gpu/gpu_plugin.
> I
> >>> was
> >>> trying to follow that pattern. I like demos better. I can change it if
> >>> you want.
> >>>
> >>> http://codereview.chromium.org/465051
> >>
> >
> >
>

[alokp1, 2009-12-17 23:01:46]

Does command buffer rely on opengl 2.0 support? If yes, do buildbots
support opengl 2.0? I think for now it will be easier to consider them
as demos and move the downloaded code to third_party and leave the
glue code in gpu/demos.

On Thu, Dec 17, 2009 at 3:56 PM, Vangelis Kokkevis <vangelis@google.com> wrote:
>
>
> On Thu, Dec 17, 2009 at 2:54 PM, Gregg Tavares <gman@chromium.org> wrote:
>>
>> More tests = good but I'm not sure how these would be unit tests. What
>> gunit macros would they use to test?
>
> Check for pixel values after reading back the contents of the frame buffer?
>
>>
>> On Thu, Dec 17, 2009 at 1:17 PM, Vangelis Kokkevis <vangelis@google.com>
>> wrote:
>> > Now that we got the OK to check that code somewhere we need to figure
>> > out
>> > where it goes.  It currently serves a test for Command Buffers but
>> > arguably
>> > over time it will be more useful as a NaCl example.  Does Chromium
>> > currently
>> > contain any stand-alone unit tests?  If so we could re-structure the
>> > demos
>> > as unit tests of the GPU process and have them run by the buildbots.
>> > Alternatively, we can check them in to the chromium source tree for now
>> > and
>> > when we have the NaCl demos codesite project move them over there.
>> > Darin,
>> > any advice?
>> >
>> > Vangelis
>> >
>> >
>> > On Fri, Dec 11, 2009 at 10:45 AM, Darin Fisher <darin@chromium.org>
>> > wrote:
>> >>
>> >> we should also change gpu/gpu_plugin to just gpu/plugin.  that would be
>> >> more consistent with the chromium codebase.  doesn't have to be done in
>> >> this
>> >> cl, but it would be nice if someone would take on the task for
>> >> consistency
>> >> sake.
>> >> -darin
>> >>
>> >> On Fri, Dec 11, 2009 at 10:24 AM, <alokp@chromium.org> wrote:
>> >>>
>> >>> On 2009/12/11 17:57:44, darin wrote:
>> >>>>
>> >>>> how about just gpu/demos?  why the extra gpu_ in the file path?
>> >>>
>> >>> No particular reason. There is another directory named gpu/gpu_plugin.
>> >>> I
>> >>> was
>> >>> trying to follow that pattern. I like demos better. I can change it if
>> >>> you want.
>> >>>
>> >>> http://codereview.chromium.org/465051
>> >>
>> >
>> >
>
>

[vangelis, 2009-12-17 23:09:15]

On Thu, Dec 17, 2009 at 3:01 PM, Alok Priyadarshi <alokp@google.com> wrote:

> Does command buffer rely on opengl 2.0 support? If yes, do buildbots
> support opengl 2.0? I think for now it will be easier to consider them
> as demos and move the downloaded code to third_party and leave the
> glue code in gpu/demos.
>

Yes, that is a problem, indeed.  We could write the demos as unit tests but
refrain from running the on the buildbot until we get the appropriate
infrastructure to do full graphics testing (which we'll need soon).  In the
meantime though, I think it should be fine to do this in steps.  Check them
in as demos in this CL (but move the copyrighted code to third_party).  A
subsequent CL can turn them into unit tests.

Vangelis


>
> On Thu, Dec 17, 2009 at 3:56 PM, Vangelis Kokkevis <vangelis@google.com>
> wrote:
> >
> >
> > On Thu, Dec 17, 2009 at 2:54 PM, Gregg Tavares <gman@chromium.org>
> wrote:
> >>
> >> More tests = good but I'm not sure how these would be unit tests. What
> >> gunit macros would they use to test?
> >
> > Check for pixel values after reading back the contents of the frame
> buffer?
> >
> >>
> >> On Thu, Dec 17, 2009 at 1:17 PM, Vangelis Kokkevis <vangelis@google.com
> >
> >> wrote:
> >> > Now that we got the OK to check that code somewhere we need to figure
> >> > out
> >> > where it goes.  It currently serves a test for Command Buffers but
> >> > arguably
> >> > over time it will be more useful as a NaCl example.  Does Chromium
> >> > currently
> >> > contain any stand-alone unit tests?  If so we could re-structure the
> >> > demos
> >> > as unit tests of the GPU process and have them run by the buildbots.
> >> > Alternatively, we can check them in to the chromium source tree for
> now
> >> > and
> >> > when we have the NaCl demos codesite project move them over there.
> >> > Darin,
> >> > any advice?
> >> >
> >> > Vangelis
> >> >
> >> >
> >> > On Fri, Dec 11, 2009 at 10:45 AM, Darin Fisher <darin@chromium.org>
> >> > wrote:
> >> >>
> >> >> we should also change gpu/gpu_plugin to just gpu/plugin.  that would
> be
> >> >> more consistent with the chromium codebase.  doesn't have to be done
> in
> >> >> this
> >> >> cl, but it would be nice if someone would take on the task for
> >> >> consistency
> >> >> sake.
> >> >> -darin
> >> >>
> >> >> On Fri, Dec 11, 2009 at 10:24 AM, <alokp@chromium.org> wrote:
> >> >>>
> >> >>> On 2009/12/11 17:57:44, darin wrote:
> >> >>>>
> >> >>>> how about just gpu/demos?  why the extra gpu_ in the file path?
> >> >>>
> >> >>> No particular reason. There is another directory named
> gpu/gpu_plugin.
> >> >>> I
> >> >>> was
> >> >>> trying to follow that pattern. I like demos better. I can change it
> if
> >> >>> you want.
> >> >>>
> >> >>> http://codereview.chromium.org/465051
> >> >>
> >> >
> >> >
> >
> >
>

[greggman, 2009-12-17 23:30:10]

On Thu, Dec 17, 2009 at 3:08 PM, Vangelis Kokkevis <vangelis@google.com> wrote:
>
>
> On Thu, Dec 17, 2009 at 3:01 PM, Alok Priyadarshi <alokp@google.com> wrote:
>>
>> Does command buffer rely on opengl 2.0 support? If yes, do buildbots
>> support opengl 2.0? I think for now it will be easier to consider them
>> as demos and move the downloaded code to third_party and leave the
>> glue code in gpu/demos.
>
> Yes, that is a problem, indeed.  We could write the demos as unit tests but
> refrain from running the on the buildbot until we get the appropriate
> infrastructure to do full graphics testing (which we'll need soon).  In the
> meantime though, I think it should be fine to do this in steps.  Check them
> in as demos in this CL (but move the copyrighted code to third_party).  A
> subsequent CL can turn them into unit tests.

Maybe this is a semantic issue but it doesn't seem like something that
has to render a scene and check pixels is a "unit test".

It will bring up the same issues that o3d has with those kinds of
tests, namely that it will need something like perceptualdiff (very
slow) to do the comparisons and it will have to pray that every OpenGL
renders the same.

On top of that this kind of test is really testing the local OpenGL
(not our code), not the code that calls OpenGL. We'll have unit tests
that test that if you call our emulated glSomeFunction(a,b,c) that the
local machine's glSomeFunction(a,b,c) gets called with the same
values. But below that, actually calling OpenGL is not a unit test.

Maybe these would be integration tests? I guess there is a place for
slow tests like the ones that render webpages?


> Vangelis
>
>>
>> On Thu, Dec 17, 2009 at 3:56 PM, Vangelis Kokkevis <vangelis@google.com>
>> wrote:
>> >
>> >
>> > On Thu, Dec 17, 2009 at 2:54 PM, Gregg Tavares <gman@chromium.org>
>> > wrote:
>> >>
>> >> More tests = good but I'm not sure how these would be unit tests. What
>> >> gunit macros would they use to test?
>> >
>> > Check for pixel values after reading back the contents of the frame
>> > buffer?
>> >
>> >>
>> >> On Thu, Dec 17, 2009 at 1:17 PM, Vangelis Kokkevis
>> >> <vangelis@google.com>
>> >> wrote:
>> >> > Now that we got the OK to check that code somewhere we need to figure
>> >> > out
>> >> > where it goes.  It currently serves a test for Command Buffers but
>> >> > arguably
>> >> > over time it will be more useful as a NaCl example.  Does Chromium
>> >> > currently
>> >> > contain any stand-alone unit tests?  If so we could re-structure the
>> >> > demos
>> >> > as unit tests of the GPU process and have them run by the buildbots.
>> >> > Alternatively, we can check them in to the chromium source tree for
>> >> > now
>> >> > and
>> >> > when we have the NaCl demos codesite project move them over there.
>> >> > Darin,
>> >> > any advice?
>> >> >
>> >> > Vangelis
>> >> >
>> >> >
>> >> > On Fri, Dec 11, 2009 at 10:45 AM, Darin Fisher <darin@chromium.org>
>> >> > wrote:
>> >> >>
>> >> >> we should also change gpu/gpu_plugin to just gpu/plugin.  that would
>> >> >> be
>> >> >> more consistent with the chromium codebase.  doesn't have to be done
>> >> >> in
>> >> >> this
>> >> >> cl, but it would be nice if someone would take on the task for
>> >> >> consistency
>> >> >> sake.
>> >> >> -darin
>> >> >>
>> >> >> On Fri, Dec 11, 2009 at 10:24 AM, <alokp@chromium.org> wrote:
>> >> >>>
>> >> >>> On 2009/12/11 17:57:44, darin wrote:
>> >> >>>>
>> >> >>>> how about just gpu/demos?  why the extra gpu_ in the file path?
>> >> >>>
>> >> >>> No particular reason. There is another directory named
>> >> >>> gpu/gpu_plugin.
>> >> >>> I
>> >> >>> was
>> >> >>> trying to follow that pattern. I like demos better. I can change it
>> >> >>> if
>> >> >>> you want.
>> >> >>>
>> >> >>> http://codereview.chromium.org/465051
>> >> >>
>> >> >
>> >> >
>> >
>> >
>
>

[alokp, 2009-12-30 19:02:59]

I have moved third-party code to third_party. Please re-review. The example has
started crashing inside command decoder when issuing glDrawArrays. I will let
apatrick/gman debug that issue - I could not find any problem in the draw code.

[alokp, 2009-12-30 20:37:50]

Adding glFlush() fixed the crash. Added a TODO indicating that glFlush should
not be necessary in addition to SwapBuffers().

[alokp, 2010-01-04 23:03:16]

Gregg: Could you please look at it once again. As suggested I have kept the
third-party code in third_party. Glue code is in gpu/demos.

[greggman, 2010-01-05 00:34:40]

LGTM