Added window manager graphics performance tests to gl_Bench.

Added window manager graphics performance tests to gl_Bench.

Changed window size to 1280x768. Added tests to verify graphics performance is sufficient for window manager. Added a test to verify that swap buffers runs at the 60hz.

To test this, I ran gl_Bench on an eee-pc.

[piman, 2010-03-24 18:16:54]

http://codereview.chromium.org/1282001/diff/1/2
File client/site_tests/gl_Bench/src/main.cc (right):

http://codereview.chromium.org/1282001/diff/1/2#newcode76
client/site_tests/gl_Bench/src/main.cc:76: uint64_t ideal_time = 1000000 / 60; 
// This is how long a swap should take
We can't guarantee 60 Hz even when vsync, because it depends on the LCD
timings...

http://codereview.chromium.org/1282001/diff/1/2#newcode79
client/site_tests/gl_Bench/src/main.cc:79: if (duration < min_time || duration >
max_time) {
the test is incorrect since it doesn't take iter into account.

http://codereview.chromium.org/1282001/diff/1/2#newcode86
client/site_tests/gl_Bench/src/main.cc:86: }
This test really isn't different from the SwapTest, except it does some extra
checks. These checks can be done in the python wrapper instead for more
flexibility, so I'd recommend simply stripping that test out.

http://codereview.chromium.org/1282001/diff/1/2#newcode578
client/site_tests/gl_Bench/src/main.cc:578: static uint32_t
texture1_1[WINDOW_HEIGHT*WINDOW_WIDTH];
you don't need all these arrays, because you're just copying the base texture
into them, then loading them with glTexImage2D (which also does a copy). You can
simply do a glTexImage2D on the texture_base array, it won't try to share
buffers or anything like that.

http://codereview.chromium.org/1282001/diff/1/2#newcode599
client/site_tests/gl_Bench/src/main.cc:599: glTexImage2D(GL_TEXTURE_2D, 0,
GL_RGBA8,
use GL_RGBA for the internal format, for compatibility with gles. RGBA8888 is
what you'll get anyway.

http://codereview.chromium.org/1282001/diff/1/2#newcode661
client/site_tests/gl_Bench/src/main.cc:661: LoadTexture(texture2);
I'm not sure this is a fair comparison to the WM update.

In this case, we're doing a memcpy, then a glTexImage2D (which does one or
possibly 2 extra copies).
In the WM case, we use X to render, using shared memory, and we use the
texture-from-pixmap extension to turn it into a texture. The implementation
details may vary a lot (on how many copies are done), but in theory it could be
done with 0 copies.

This may be somewhat representative (the expectation could be adjusted), but one
of the goals for the test is to test the performance of the texture-from-pixmap
extension so that we're closer to the real world.

http://codereview.chromium.org/1282001/diff/1/2#newcode662
client/site_tests/gl_Bench/src/main.cc:662: // TODO(papakipos): add color
interpolation here, and modulate texture against it
80 chars

http://codereview.chromium.org/1282001/diff/1/2#newcode669
client/site_tests/gl_Bench/src/main.cc:669: // TODO(papakipos): add color
interpolation here, and modulate texture against it
80 chars

http://codereview.chromium.org/1282001/diff/1/2#newcode707
client/site_tests/gl_Bench/src/main.cc:707: glVertexPointer(2, GL_FLOAT, 0,
vbo_vertex ? 0 : buffer_vertex);
If VBO aren't supported (they need to be from the HW spec, and that's tested in
a separate test, but anyway), the vertex array will point to a local buffer.
It's a bad idea, because the buffer will go away when the function exits, and
that can lead to strange bugs.
Instead I would just CHECK that the vbo is != 0 or make the buffer static.

[Alexey Marinichev, 2010-03-24 18:23:21]

http://codereview.chromium.org/1282001/diff/1/2
File client/site_tests/gl_Bench/src/main.cc (right):

http://codereview.chromium.org/1282001/diff/1/2#newcode68
client/site_tests/gl_Bench/src/main.cc:68: void SwapVerifyTestFunc(int iter) {
This test should probably be merged with SwapTest.  We can get Bench to return
minimum and maximum values, then we don't have to do any of that here.

http://codereview.chromium.org/1282001/diff/1/2#newcode76
client/site_tests/gl_Bench/src/main.cc:76: uint64_t ideal_time = 1000000 / 60; 
// This is how long a swap should take
On 2010/03/24 18:16:54, piman wrote:
> We can't guarantee 60 Hz even when vsync, because it depends on the LCD
> timings...
Checking that min and max values are close enough to the average and average
looks sane should be enough.

http://codereview.chromium.org/1282001/diff/1/2#newcode78
client/site_tests/gl_Bench/src/main.cc:78: uint64_t max_time = (uint64_t)
(ideal_time * 1.1);
Use C++ style casts: static_cast<uint64_t>(arg).

http://codereview.chromium.org/1282001/diff/1/2#newcode80
client/site_tests/gl_Bench/src/main.cc:80: printf("# Time was out of range\n");
This just prints a log message that will be filtered out by the python script. 
Failing tests should print "testname: Nan".

http://codereview.chromium.org/1282001/diff/1/2#newcode591
client/site_tests/gl_Bench/src/main.cc:591: }
memset(texture_base, sizeof(texture_base), 0x80) will effectively do the same.

http://codereview.chromium.org/1282001/diff/1/2#newcode599
client/site_tests/gl_Bench/src/main.cc:599: glTexImage2D(GL_TEXTURE_2D, 0,
GL_RGBA8,
GLES only has GL_RGBA.

http://codereview.chromium.org/1282001/diff/1/2#newcode604
client/site_tests/gl_Bench/src/main.cc:604: void DrawTexturedQuad() {
This function doesn't do anything else.  Calling glDrawArrays directly seems
more straightforward.

http://codereview.chromium.org/1282001/diff/1/2#newcode615
client/site_tests/gl_Bench/src/main.cc:615: for (int i = 0 ; i < iter; ++i) {
The loop should have as few state changes as possible.

http://codereview.chromium.org/1282001/diff/1/3
File client/site_tests/gl_Bench/src/main.h (right):

http://codereview.chromium.org/1282001/diff/1/3#newcode66
client/site_tests/gl_Bench/src/main.h:66: #define WINDOW_HEIGHT 768
This might break other tests that want the window size to be power of two.  We
should probably use the smaller sized window for the time being and then
generalize how windows are created.

[papakipos, 2010-03-24 20:18:22]

Addressed all reviewer issues, rebased, retested.

http://codereview.chromium.org/1282001/diff/1/2
File client/site_tests/gl_Bench/src/main.cc (right):

http://codereview.chromium.org/1282001/diff/1/2#newcode68
client/site_tests/gl_Bench/src/main.cc:68: void SwapVerifyTestFunc(int iter) {
ok i dropped swapverifytest.
i'll work on another test to verify this from the python wrapper.

On 2010/03/24 18:23:21, Alexey Marinichev wrote:
> This test should probably be merged with SwapTest.  We can get Bench to return
> minimum and maximum values, then we don't have to do any of that here.

http://codereview.chromium.org/1282001/diff/1/2#newcode76
client/site_tests/gl_Bench/src/main.cc:76: uint64_t ideal_time = 1000000 / 60; 
// This is how long a swap should take
removed this test
On 2010/03/24 18:16:54, piman wrote:
> We can't guarantee 60 Hz even when vsync, because it depends on the LCD
> timings...

http://codereview.chromium.org/1282001/diff/1/2#newcode76
client/site_tests/gl_Bench/src/main.cc:76: uint64_t ideal_time = 1000000 / 60; 
// This is how long a swap should take
removed this test
On 2010/03/24 18:23:21, Alexey Marinichev wrote:
> On 2010/03/24 18:16:54, piman wrote:
> > We can't guarantee 60 Hz even when vsync, because it depends on the LCD
> > timings...
> Checking that min and max values are close enough to the average and average
> looks sane should be enough.

http://codereview.chromium.org/1282001/diff/1/2#newcode78
client/site_tests/gl_Bench/src/main.cc:78: uint64_t max_time = (uint64_t)
(ideal_time * 1.1);
removed this test
On 2010/03/24 18:23:21, Alexey Marinichev wrote:
> Use C++ style casts: static_cast<uint64_t>(arg).

http://codereview.chromium.org/1282001/diff/1/2#newcode79
client/site_tests/gl_Bench/src/main.cc:79: if (duration < min_time || duration >
max_time) {
removed this test
On 2010/03/24 18:16:54, piman wrote:
> the test is incorrect since it doesn't take iter into account.

http://codereview.chromium.org/1282001/diff/1/2#newcode80
client/site_tests/gl_Bench/src/main.cc:80: printf("# Time was out of range\n");
removed this test
On 2010/03/24 18:23:21, Alexey Marinichev wrote:
> This just prints a log message that will be filtered out by the python script.

> Failing tests should print "testname: Nan".

http://codereview.chromium.org/1282001/diff/1/2#newcode86
client/site_tests/gl_Bench/src/main.cc:86: }
agreed, will drop it and do it the way you suggest.

On 2010/03/24 18:16:54, piman wrote:
> This test really isn't different from the SwapTest, except it does some extra
> checks. These checks can be done in the python wrapper instead for more
> flexibility, so I'd recommend simply stripping that test out.

http://codereview.chromium.org/1282001/diff/1/2#newcode578
client/site_tests/gl_Bench/src/main.cc:578: static uint32_t
texture1_1[WINDOW_HEIGHT*WINDOW_WIDTH];
my test is also measuring the time of copying
one cpu buffer to another before loading it into a texture with glTexImage2D, so
i need at least two buffers.
I agree I could drop the others. I will make that change.

On 2010/03/24 18:16:54, piman wrote:
> you don't need all these arrays, because you're just copying the base texture
> into them, then loading them with glTexImage2D (which also does a copy). You
can
> simply do a glTexImage2D on the texture_base array, it won't try to share
> buffers or anything like that.

http://codereview.chromium.org/1282001/diff/1/2#newcode591
client/site_tests/gl_Bench/src/main.cc:591: }
i want to also cause the cpu memory read traffic in addition to the writes.
i'm trying to simulate chrome crafting a frambuffer. (yes, it's a very rough
approximation.)
i'll add a comment to this effect.

On 2010/03/24 18:23:21, Alexey Marinichev wrote:
> memset(texture_base, sizeof(texture_base), 0x80) will effectively do the same.

http://codereview.chromium.org/1282001/diff/1/2#newcode599
client/site_tests/gl_Bench/src/main.cc:599: glTexImage2D(GL_TEXTURE_2D, 0,
GL_RGBA8,
On 2010/03/24 18:16:54, piman wrote:
> use GL_RGBA for the internal format, for compatibility with gles. RGBA8888 is
> what you'll get anyway.

Done.

http://codereview.chromium.org/1282001/diff/1/2#newcode599
client/site_tests/gl_Bench/src/main.cc:599: glTexImage2D(GL_TEXTURE_2D, 0,
GL_RGBA8,
On 2010/03/24 18:23:21, Alexey Marinichev wrote:
> GLES only has GL_RGBA.

Done.

http://codereview.chromium.org/1282001/diff/1/2#newcode604
client/site_tests/gl_Bench/src/main.cc:604: void DrawTexturedQuad() {
ok done

On 2010/03/24 18:23:21, Alexey Marinichev wrote:
> This function doesn't do anything else.  Calling glDrawArrays directly seems
> more straightforward.

http://codereview.chromium.org/1282001/diff/1/2#newcode615
client/site_tests/gl_Bench/src/main.cc:615: for (int i = 0 ; i < iter; ++i) {
i think the loop does have a minimal number of state changes.
did i miss something?

On 2010/03/24 18:23:21, Alexey Marinichev wrote:
> The loop should have as few state changes as possible.

http://codereview.chromium.org/1282001/diff/1/2#newcode661
client/site_tests/gl_Bench/src/main.cc:661: LoadTexture(texture2);
I'd like to check this in, since it provides a certain level of test (better
than what we have right now).
I agree it could be extended later to be yet more accurate, but i'd rather not
hold up this checkin now.
I'll add a comment indicating that the glTexImage2D is likely doing more copies
than it should.
This test, if it passes, should indicate ample performance for the window
manager. 

On 2010/03/24 18:16:54, piman wrote:
> I'm not sure this is a fair comparison to the WM update.
> 
> In this case, we're doing a memcpy, then a glTexImage2D (which does one or
> possibly 2 extra copies).
> In the WM case, we use X to render, using shared memory, and we use the
> texture-from-pixmap extension to turn it into a texture. The implementation
> details may vary a lot (on how many copies are done), but in theory it could
be
> done with 0 copies.
> 
> This may be somewhat representative (the expectation could be adjusted), but
one
> of the goals for the test is to test the performance of the
texture-from-pixmap
> extension so that we're closer to the real world.

http://codereview.chromium.org/1282001/diff/1/2#newcode662
client/site_tests/gl_Bench/src/main.cc:662: // TODO(papakipos): add color
interpolation here, and modulate texture against it
On 2010/03/24 18:16:54, piman wrote:
> 80 chars

Done.

http://codereview.chromium.org/1282001/diff/1/2#newcode669
client/site_tests/gl_Bench/src/main.cc:669: // TODO(papakipos): add color
interpolation here, and modulate texture against it
On 2010/03/24 18:16:54, piman wrote:
> 80 chars

Done.

http://codereview.chromium.org/1282001/diff/1/2#newcode707
client/site_tests/gl_Bench/src/main.cc:707: glVertexPointer(2, GL_FLOAT, 0,
vbo_vertex ? 0 : buffer_vertex);
ok, after discussion with alexey, i'll make a separate changelist to rip out all
the vbo-failure-handling code paths in gl_bench.

On 2010/03/24 18:16:54, piman wrote:
> If VBO aren't supported (they need to be from the HW spec, and that's tested
in
> a separate test, but anyway), the vertex array will point to a local buffer.
> It's a bad idea, because the buffer will go away when the function exits, and
> that can lead to strange bugs.
> Instead I would just CHECK that the vbo is != 0 or make the buffer static.

http://codereview.chromium.org/1282001/diff/1/3
File client/site_tests/gl_Bench/src/main.h (right):

http://codereview.chromium.org/1282001/diff/1/3#newcode66
client/site_tests/gl_Bench/src/main.h:66: #define WINDOW_HEIGHT 768
ok, i dropped this and figured out how to scale the reported results instead.

On 2010/03/24 18:23:21, Alexey Marinichev wrote:
> This might break other tests that want the window size to be power of two.  We
> should probably use the smaller sized window for the time being and then
> generalize how windows are created.

[Alexey Marinichev, 2010-03-24 20:42:25]

LGTM with one comment.

http://codereview.chromium.org/1282001/diff/10001/11001
File client/deps/glbench/src/main.cc (right):

http://codereview.chromium.org/1282001/diff/10001/11001#newcode789
client/deps/glbench/src/main.cc:789: RunTest(CompositingTestFunc,
"fps_frame_compositing",
Maybe include 1280 and 768 in the test name?

http://codereview.chromium.org/1282001/diff/10001/11001#newcode798
client/deps/glbench/src/main.cc:798: RunTest(CompositingTestFunc,
"fps_frame_no_fill_compositing",
Same thing about 1280 and 768.

[papakipos, 2010-03-24 20:47:02]

Implemented requested name change, retested.

http://codereview.chromium.org/1282001/diff/10001/11001
File client/deps/glbench/src/main.cc (right):

http://codereview.chromium.org/1282001/diff/10001/11001#newcode789
client/deps/glbench/src/main.cc:789: RunTest(CompositingTestFunc,
"fps_frame_compositing",
On 2010/03/24 20:42:25, Alexey Marinichev wrote:
> Maybe include 1280 and 768 in the test name?

Done.

http://codereview.chromium.org/1282001/diff/10001/11001#newcode798
client/deps/glbench/src/main.cc:798: RunTest(CompositingTestFunc,
"fps_frame_no_fill_compositing",
On 2010/03/24 20:42:25, Alexey Marinichev wrote:
> Same thing about 1280 and 768.

Done.