Basic pixel tests for cc

Basic pixel tests for cc

BUG=


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171544

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171735

[jamesr, 2012-12-01 01:38:13]

This is a ghetto start to pixel tests for cc.

piman@ - there's nifty utilities for reading/writing/comparing/diffing PNGs in
the ui/compositor dir for those pixel tests.  I want to use those here - where
should they go?  In cc/test/ since ui/compositor/ depends on cc/ anyway?

[danakj, 2012-12-01 01:56:36]

Neet :)

https://codereview.chromium.org/11412289/diff/1/cc/cc_tests.gyp
File cc/cc_tests.gyp (right):

https://codereview.chromium.org/11412289/diff/1/cc/cc_tests.gyp#newcode19
cc/cc_tests.gyp:19: 'gl_renderer_pixel_test.cc',
pixeltest instead of pixel_test?

https://codereview.chromium.org/11412289/diff/1/cc/gl_renderer.h
File cc/gl_renderer.h (right):

https://codereview.chromium.org/11412289/diff/1/cc/gl_renderer.h#newcode43
cc/gl_renderer.h:43: bool initialize();
I don't see why you need this?

https://codereview.chromium.org/11412289/diff/1/cc/gl_renderer_pixel_test.cc
File cc/gl_renderer_pixel_test.cc (right):

https://codereview.chromium.org/11412289/diff/1/cc/gl_renderer_pixel_test.cc#...
cc/gl_renderer_pixel_test.cc:1: // Copyright 2012 The Chromium Authors. All
rights reserved.
should this file be chromium style names and indenting?

https://codereview.chromium.org/11412289/diff/1/cc/gl_renderer_pixel_test.cc#...
cc/gl_renderer_pixel_test.cc:95: FilePath base_path;
Can the non-test-specific stuff go in a test baseclass?

https://codereview.chromium.org/11412289/diff/1/cc/gl_renderer_pixel_test.cc#...
cc/gl_renderer_pixel_test.cc:99:
file_util::WriteFile(test_data_directory.AppendASCII("green.png"),
Why writing a file instead of comparing?

[jamesr, 2012-12-01 01:57:59]

>
https://codereview.chromium.org/11412289/diff/1/cc/gl_renderer_pixel_test.cc#...
> cc/gl_renderer_pixel_test.cc:99:
> file_util::WriteFile(test_data_directory.AppendASCII("green.png"),
> Why writing a file instead of comparing?

There's code that does this already in ui/compositor/, so I want to figure out
how to reuse it (see question above to piman@)

[piman, 2012-12-01 02:16:33]

On 2012/12/01 01:38:13, jamesr wrote:
> This is a ghetto start to pixel tests for cc.
> 
> piman@ - there's nifty utilities for reading/writing/comparing/diffing PNGs in
> the ui/compositor dir for those pixel tests.  I want to use those here - where
> should they go?  In cc/test/ since ui/compositor/ depends on cc/ anyway?

That sg.

[jamesr, 2012-12-03 07:33:40]

This is I think a minimum viable first cut.  I'd like to refactor some more of
the shared logic in the test out of the test case and into a testing base class,
but otherwise I think this is ready to go.

[danakj, 2012-12-03 19:14:10]

Looks good, mostly some questions/concerns about the copypasta for cc_paths

https://codereview.chromium.org/11412289/diff/5001/cc/cc.gyp
File cc/cc.gyp (right):

https://codereview.chromium.org/11412289/diff/5001/cc/cc.gyp#newcode20
cc/cc.gyp:20: 'cc_paths.cc',
should this be instead "src/cc/tests/paths.cc"?

https://codereview.chromium.org/11412289/diff/5001/cc/cc_paths.cc
File cc/cc_paths.cc (right):

https://codereview.chromium.org/11412289/diff/5001/cc/cc_paths.cc#newcode13
cc/cc_paths.cc:13: bool PathProvider(int key, FilePath* result) {
static?

https://codereview.chromium.org/11412289/diff/5001/cc/cc_paths.cc#newcode16
cc/cc_paths.cc:16: // The following are only valid in the development
environment, and
"development environment" -> "perf tests" or something?

https://codereview.chromium.org/11412289/diff/5001/cc/cc_paths.h
File cc/cc_paths.h (right):

https://codereview.chromium.org/11412289/diff/5001/cc/cc_paths.h#newcode13
cc/cc_paths.h:13: PATH_START = 4000,
magic 4000.. comment?

https://codereview.chromium.org/11412289/diff/5001/cc/cc_paths.h#newcode15
cc/cc_paths.h:15: // Valid only in development environment.
this sounds like copy pasta, and i'm not sure it is right here..?

https://codereview.chromium.org/11412289/diff/5001/cc/cc_paths.h#newcode16
cc/cc_paths.h:16: DIR_TEST_DATA,            // Directory where unit test data
resides.
what's up with the weird comment whitespace?

https://codereview.chromium.org/11412289/diff/5001/cc/gl_renderer_pixeltest.cc
File cc/gl_renderer_pixeltest.cc (right):

https://codereview.chromium.org/11412289/diff/5001/cc/gl_renderer_pixeltest.c...
cc/gl_renderer_pixeltest.cc:30: static gfx::Size fakeSize(200, 200);
nit: fake_size

https://codereview.chromium.org/11412289/diff/5001/cc/gl_renderer_pixeltest.c...
cc/gl_renderer_pixeltest.cc:34: static LayerTreeSettings fakeSettings;
nit: fake_settings

https://codereview.chromium.org/11412289/diff/5001/cc/gl_renderer_pixeltest.c...
cc/gl_renderer_pixeltest.cc:73: rect,
can you use temp vars for these so we can see what's being set from the caller?

https://codereview.chromium.org/11412289/diff/5001/cc/test/pixel_test_utils.cc
File cc/test/pixel_test_utils.cc (right):

https://codereview.chromium.org/11412289/diff/5001/cc/test/pixel_test_utils.c...
cc/test/pixel_test_utils.cc:16: if (gfx::PNGCodec::EncodeBGRASkBitmap(bitmap,
true, &png_data) &&
Use a temp var here for discard_transparency please (also why do we do that?)

https://codereview.chromium.org/11412289/diff/5001/cc/test/pixel_test_utils.c...
cc/test/pixel_test_utils.cc:19: int size = static_cast<int>(png_data.size());
doh /o\

https://codereview.chromium.org/11412289/diff/5001/ui/gl/gl_implementation_ma...
File ui/gl/gl_implementation_mac.cc (right):

https://codereview.chromium.org/11412289/diff/5001/ui/gl/gl_implementation_ma...
ui/gl/gl_implementation_mac.cc:52: // Some unit test targets depend on osmesa
but aren't build as app
nit: s/build/built/

[piman, 2012-12-03 19:28:28]

LGTM

[enne (OOO), 2012-12-03 21:40:29]

lgtm.  Thanks for biting the bullet and implementing this.  It'll be great to
have.

Two unimportant style nits:

https://codereview.chromium.org/11412289/diff/5001/cc/gl_renderer_pixeltest.cc
File cc/gl_renderer_pixeltest.cc (right):

https://codereview.chromium.org/11412289/diff/5001/cc/gl_renderer_pixeltest.c...
cc/gl_renderer_pixeltest.cc:47: class GLRendererPixeltest : public testing::Test
{
Can you be consistent in your capitalization of Test? Pixeltest (here) vs.
PerfTest and PixelTest (elsewhere)?

https://codereview.chromium.org/11412289/diff/5001/cc/test/pixel_test_graphic...
File cc/test/pixel_test_graphics_context.h (right):

https://codereview.chromium.org/11412289/diff/5001/cc/test/pixel_test_graphic...
cc/test/pixel_test_graphics_context.h:31:
scoped_ptr<WebKit::WebGraphicsContext3D> m_context;
m_context => context_

[jamesr, 2012-12-04 02:21:01]

On 2012/12/03 19:14:10, danakj wrote:
> Looks good, mostly some questions/concerns about the copypasta for cc_paths
> 
> https://codereview.chromium.org/11412289/diff/5001/cc/cc.gyp
> File cc/cc.gyp (right):
> 
> https://codereview.chromium.org/11412289/diff/5001/cc/cc.gyp#newcode20
> cc/cc.gyp:20: 'cc_paths.cc',
> should this be instead "src/cc/tests/paths.cc"?

This is exposed here to mimic ui/gfx/gfx_paths, which similar exposes a path to
everyone who uses the module can have access to that module's test resources.
> 
> https://codereview.chromium.org/11412289/diff/5001/cc/cc_paths.cc
> File cc/cc_paths.cc (right):
> 
> https://codereview.chromium.org/11412289/diff/5001/cc/cc_paths.cc#newcode13
> cc/cc_paths.cc:13: bool PathProvider(int key, FilePath* result) {
> static?

The other PathProvider implementations seem to be this way.

> 
> https://codereview.chromium.org/11412289/diff/5001/cc/cc_paths.cc#newcode16
> cc/cc_paths.cc:16: // The following are only valid in the development
> environment, and
> "development environment" -> "perf tests" or something?

Rephrased a bit.  It's not just for perf tests - these pixel tests are unit
tests.

> 
> https://codereview.chromium.org/11412289/diff/5001/cc/cc_paths.h
> File cc/cc_paths.h (right):
> 
> https://codereview.chromium.org/11412289/diff/5001/cc/cc_paths.h#newcode13
> cc/cc_paths.h:13: PATH_START = 4000,
> magic 4000.. comment?

It's just picked not to collide with other module's PATH_START.  What sort of
comment would be useful?

>
https://codereview.chromium.org/11412289/diff/5001/cc/test/pixel_test_utils.c...
> cc/test/pixel_test_utils.cc:16: if (gfx::PNGCodec::EncodeBGRASkBitmap(bitmap,
> true, &png_data) &&
> Use a temp var here for discard_transparency please (also why do we do that?)

We do that because that's what the comparisons expect (see IsSameAsPNGFile).  If
we want to do alpha-aware comparisons we can get rid of this, just need to make
sure all the ui/compositor tests still pass.

> 
>
https://codereview.chromium.org/11412289/diff/5001/cc/test/pixel_test_utils.c...
> cc/test/pixel_test_utils.cc:19: int size = static_cast<int>(png_data.size());
> doh /o\

Yeah, that's what base::FileWriter takes.  I hope we don't have test PNGs that
are >2GB

[jamesr, 2012-12-04 02:21:46]

Ken - could you please OWNERS review the ui/gl/gl_implementation_mac.cc change? 
It's so I can use osmesa from a "normal" unit test binary (not a .app)

[Ken Russell (switch to Gerrit), 2012-12-04 02:47:13]

ui/gl LGTM

[commit-bot: I haz the power, 2012-12-04 05:45:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11412289/11002

[commit-bot: I haz the power, 2012-12-04 17:22:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11412289/8006

[commit-bot: I haz the power, 2012-12-04 21:45:40]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[jamesr, 2012-12-04 22:16:06]

On 2012/12/04 21:45:40, I haz the power (commit-bot) wrote:
> The commit queue went berserk retrying too often for a
> seemingly flaky test. Builder is android_dbg, revision is HEAD

Right, there's no osmesa (afaik) on android and if there was we probably
wouldn't want to use it.  I'll #ifdef the test.

[commit-bot: I haz the power, 2012-12-06 00:09:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11412289/15001

[commit-bot: I haz the power, 2012-12-06 05:22:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11412289/29002

[commit-bot: I haz the power, 2012-12-06 17:20:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11412289/29002

[commit-bot: I haz the power, 2012-12-06 18:47:49]

Change committed as 171544

[commit-bot: I haz the power, 2012-12-07 02:49:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamesr@chromium.org/11412289/39001

[commit-bot: I haz the power, 2012-12-07 08:52:04]

Change committed as 171735