cc: Made image comparison for pixel tests error tolerant.

cc: Made image comparison for pixel tests error tolerant.

Added PixelComparator interface and two implementations of it: ExactPixelComparator and FuzzyPixelComparator. The FuzzyPixelComparator tolerates an adjustable amount of error in the image.

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

[reveman, 2013-03-07 07:50:44]

Would it be cleaner and more extensible if we just had a Comparator interface
and different implementations for each metric needed? Example:

class PixelComparator {
 public:
  virtual void Compare(const SkBitmap& a, const SkBitmap& b) const = 0;
                                                                                
 protected:
  virtual ~PixelComparator() {}
};

and in this patch all you'd need is an ExactPixelComparator implementation. The
PixelComparator implementation needed for AA is better left for the solid color
AA patch that actually uses it. The unit tests would have code like this:

EXPECT_TRUE(PixelsMatchReference(
            base::FilePath(FILE_PATH_LITERAL("green.png")),
            rect, ExactImageComparator()));

You could stick to ImageComparator if you think that's a better name. I used
"Pixel" here as it didn't seem necessary to introduce a new term and we already
use the pixel term for the tests.

https://codereview.chromium.org/12558003/diff/1/cc/test/image_comparator.cc
File cc/test/image_comparator.cc (right):

https://codereview.chromium.org/12558003/diff/1/cc/test/image_comparator.cc#n...
cc/test/image_comparator.cc:70: bitmap_a.height() != bitmap_b.height()) {
MatchesPNGFile already check the size. DCHECK is enough here.

https://codereview.chromium.org/12558003/diff/1/cc/test/image_comparator.cc#n...
cc/test/image_comparator.cc:86: if (bitmap_a.width() == 0 || bitmap_a.height()
== 0) {
move this to MatchesPNGFile if needed. DCHECK at most here.

https://codereview.chromium.org/12558003/diff/1/cc/test/pixel_test_utils.cc
File cc/test/pixel_test_utils.cc (right):

https://codereview.chromium.org/12558003/diff/1/cc/test/pixel_test_utils.cc#n...
cc/test/pixel_test_utils.cc:56: if (!comparator.Compare(gen_bmp, ref_bmp,
&metrics)) {
could the comparator instead be responsible for printing an error message?

[ernstm, 2013-03-07 19:28:30]

Good suggestions. I refactored the code to use the PixelComparator interface. I
split the ImageComparator into two implementations of that interface:
ExactPixelComparator and FuzzyPixelComparator.

On 2013/03/07 07:50:44, David Reveman wrote:
> Would it be cleaner and more extensible if we just had a Comparator interface
> and different implementations for each metric needed? Example:
> 
> class PixelComparator {
>  public:
>   virtual void Compare(const SkBitmap& a, const SkBitmap& b) const = 0;
>                                                                               
 
>  protected:
>   virtual ~PixelComparator() {}
> };
> 
> and in this patch all you'd need is an ExactPixelComparator implementation.
The
> PixelComparator implementation needed for AA is better left for the solid
color
> AA patch that actually uses it. The unit tests would have code like this:
> 
> EXPECT_TRUE(PixelsMatchReference(
>             base::FilePath(FILE_PATH_LITERAL("green.png")),
>             rect, ExactImageComparator()));
> 
> You could stick to ImageComparator if you think that's a better name. I used
> "Pixel" here as it didn't seem necessary to introduce a new term and we
already
> use the pixel term for the tests.
> 
> https://codereview.chromium.org/12558003/diff/1/cc/test/image_comparator.cc
> File cc/test/image_comparator.cc (right):
> 
>
https://codereview.chromium.org/12558003/diff/1/cc/test/image_comparator.cc#n...
> cc/test/image_comparator.cc:70: bitmap_a.height() != bitmap_b.height()) {
> MatchesPNGFile already check the size. DCHECK is enough here.
> 
>
https://codereview.chromium.org/12558003/diff/1/cc/test/image_comparator.cc#n...
> cc/test/image_comparator.cc:86: if (bitmap_a.width() == 0 || bitmap_a.height()
> == 0) {
> move this to MatchesPNGFile if needed. DCHECK at most here.
> 
> https://codereview.chromium.org/12558003/diff/1/cc/test/pixel_test_utils.cc
> File cc/test/pixel_test_utils.cc (right):
> 
>
https://codereview.chromium.org/12558003/diff/1/cc/test/pixel_test_utils.cc#n...
> cc/test/pixel_test_utils.cc:56: if (!comparator.Compare(gen_bmp, ref_bmp,
> &metrics)) {
> could the comparator instead be responsible for printing an error message?

[ernstm, 2013-03-07 19:30:07]

https://codereview.chromium.org/12558003/diff/1/cc/test/image_comparator.cc
File cc/test/image_comparator.cc (right):

https://codereview.chromium.org/12558003/diff/1/cc/test/image_comparator.cc#n...
cc/test/image_comparator.cc:70: bitmap_a.height() != bitmap_b.height()) {
Moved all dimension checking into Compare function instead, because some future
comparators might want to handle different dimensions in a special way.
On 2013/03/07 07:50:44, David Reveman wrote:
> MatchesPNGFile already check the size. DCHECK is enough here.

https://codereview.chromium.org/12558003/diff/1/cc/test/image_comparator.cc#n...
cc/test/image_comparator.cc:86: if (bitmap_a.width() == 0 || bitmap_a.height()
== 0) {
This got much simpler in the refactored code.
On 2013/03/07 07:50:44, David Reveman wrote:
> move this to MatchesPNGFile if needed. DCHECK at most here.

https://codereview.chromium.org/12558003/diff/1/cc/test/pixel_test_utils.cc
File cc/test/pixel_test_utils.cc (right):

https://codereview.chromium.org/12558003/diff/1/cc/test/pixel_test_utils.cc#n...
cc/test/pixel_test_utils.cc:56: if (!comparator.Compare(gen_bmp, ref_bmp,
&metrics)) {
On 2013/03/07 07:50:44, David Reveman wrote:
> could the comparator instead be responsible for printing an error message?

Done.

[reveman, 2013-03-07 21:41:25]

nice re-factor

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_comparator.cc
File cc/test/pixel_comparator.cc (right):

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_comparator.c...
cc/test/pixel_comparator.cc:15: {
{ should be on previous line

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_comparator.c...
cc/test/pixel_comparator.cc:20: << "Actual: " << actual_bmp.width() << "x" <<
actual_bmp.height() << "; "
line too long

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_comparator.c...
cc/test/pixel_comparator.cc:21: << "Expected: " << expected_bmp.width() << "x"
<< expected_bmp.height() << "";
line too long

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_comparator.c...
cc/test/pixel_comparator.cc:78: unsigned long long sum_abs_error_r = 0;
I think you should use int64/uint64 instead of long

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_comparator.c...
cc/test/pixel_comparator.cc:97: uint32_t kAlphaMask = 0x00FFFFFF;
This makes assumptions about the pixel format used by skia. Why not make it easy
on ourselves and require all reference images to have an alpha channel?

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_comparator.c...
cc/test/pixel_comparator.cc:141: float small_error_pixels_percentage =
static_cast<float>(small_error_pixels_count) /
line too long

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_comparator.h
File cc/test/pixel_comparator.h (right):

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_comparator.h...
cc/test/pixel_comparator.h:19: virtual ~PixelComparator() {};
don't need a ; after a }

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_comparator.h...
cc/test/pixel_comparator.h:23: const SkBitmap& expected_bmp) const;
To keep this as clean as possible, I'd prefer to keep the size check in
IsSameAsPNGFile until someone implements a comparator that needs it. Also not
clear to me why we would ever allow the bitmaps to have different sizes.

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_comparator.h...
cc/test/pixel_comparator.h:44: const unsigned int small_error_threshold = 1);
Style guide doesn't allow default function parameters.

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

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_test_utils.h...
cc/test/pixel_test_utils.h:23: // Compares with a PNG file on disk using the
given ImageComparator, and returns
s/ImageComparator/PixelComparator/

[ernstm, 2013-03-08 00:14:46]

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_comparator.cc
File cc/test/pixel_comparator.cc (right):

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_comparator.c...
cc/test/pixel_comparator.cc:15: {
On 2013/03/07 21:41:25, David Reveman wrote:
> { should be on previous line

Done.

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_comparator.c...
cc/test/pixel_comparator.cc:20: << "Actual: " << actual_bmp.width() << "x" <<
actual_bmp.height() << "; "
On 2013/03/07 21:41:25, David Reveman wrote:
> line too long

Done.

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_comparator.c...
cc/test/pixel_comparator.cc:21: << "Expected: " << expected_bmp.width() << "x"
<< expected_bmp.height() << "";
On 2013/03/07 21:41:25, David Reveman wrote:
> line too long

Done.

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_comparator.c...
cc/test/pixel_comparator.cc:78: unsigned long long sum_abs_error_r = 0;
On 2013/03/07 21:41:25, David Reveman wrote:
> I think you should use int64/uint64 instead of long

Done.

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_comparator.c...
cc/test/pixel_comparator.cc:97: uint32_t kAlphaMask = 0x00FFFFFF;
Modified to use SkColorSetA instead instead of direct bit manipulation.
On 2013/03/07 21:41:25, David Reveman wrote:
> This makes assumptions about the pixel format used by skia. Why not make it
easy
> on ourselves and require all reference images to have an alpha channel?

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_comparator.c...
cc/test/pixel_comparator.cc:141: float small_error_pixels_percentage =
static_cast<float>(small_error_pixels_count) /
On 2013/03/07 21:41:25, David Reveman wrote:
> line too long

Done.

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_comparator.h
File cc/test/pixel_comparator.h (right):

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_comparator.h...
cc/test/pixel_comparator.h:19: virtual ~PixelComparator() {};
On 2013/03/07 21:41:25, David Reveman wrote:
> don't need a ; after a }

Done.

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_comparator.h...
cc/test/pixel_comparator.h:23: const SkBitmap& expected_bmp) const;
On 2013/03/07 21:41:25, David Reveman wrote:
> To keep this as clean as possible, I'd prefer to keep the size check in
> IsSameAsPNGFile until someone implements a comparator that needs it. Also not
> clear to me why we would ever allow the bitmaps to have different sizes.

Done.

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_comparator.h...
cc/test/pixel_comparator.h:44: const unsigned int small_error_threshold = 1);
On 2013/03/07 21:41:25, David Reveman wrote:
> Style guide doesn't allow default function parameters.

Done.

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

https://codereview.chromium.org/12558003/diff/5001/cc/test/pixel_test_utils.h...
cc/test/pixel_test_utils.h:23: // Compares with a PNG file on disk using the
given ImageComparator, and returns
On 2013/03/07 21:41:25, David Reveman wrote:
> s/ImageComparator/PixelComparator/

Done.

[reveman, 2013-03-08 04:41:57]

lgtm but have jamesr review this before landing

https://codereview.chromium.org/12558003/diff/11001/cc/test/pixel_comparator.cc
File cc/test/pixel_comparator.cc (right):

https://codereview.chromium.org/12558003/diff/11001/cc/test/pixel_comparator....
cc/test/pixel_comparator.cc:118: if (abs_error_b > max_abs_error_b)
max_abs_error_b = abs_error_b;
nit: put max_abs_error_? = abs_error_?; statements on next line.

https://codereview.chromium.org/12558003/diff/11001/cc/test/pixel_test_utils.cc
File cc/test/pixel_test_utils.cc (left):

https://codereview.chromium.org/12558003/diff/11001/cc/test/pixel_test_utils....
cc/test/pixel_test_utils.cc:60: uint32_t kAlphaMask = 0x00FFFFFF;
oh, sorry I didn't realize the old code was doing this. thanks for improving
this in this patch. I think as a follow up we might want to change the ref
images to contain alpha and include that in our comparison.

https://codereview.chromium.org/12558003/diff/11001/cc/test/pixel_test_utils.cc
File cc/test/pixel_test_utils.cc (right):

https://codereview.chromium.org/12558003/diff/11001/cc/test/pixel_test_utils....
cc/test/pixel_test_utils.cc:52: << "Expected: " << ref_bmp.width() << "x" <<
ref_bmp.height();
nit: I'd avoid changing this just to make the patch cleaner but if you think
this output is better that's fine too.

[slavi, 2013-03-08 19:49:04]

https://codereview.chromium.org/12558003/diff/11001/cc/test/pixel_comparator.cc
File cc/test/pixel_comparator.cc (right):

https://codereview.chromium.org/12558003/diff/11001/cc/test/pixel_comparator....
cc/test/pixel_comparator.cc:100: std::max(SkColorGetR(actual_color),
SkColorGetR(expected_color)) -
Why not

std::abs(SkColorGetR(actual_color) - SkColorGetR(expected_color))

?

https://codereview.chromium.org/12558003/diff/11001/cc/test/pixel_comparator....
cc/test/pixel_comparator.cc:116: if (abs_error_r > max_abs_error_r)
max_abs_error_r = abs_error_r;
max_abs_error_r = std::max(max_abs_error_r, abs_error_r);

[danakj, 2013-03-18 17:45:22]

This will need a rebase, but I'd like to use this in my background blur
compositor filter tests. James WDYT?

https://codereview.chromium.org/12558003/diff/11001/cc/gl_renderer_pixeltest.cc
File cc/gl_renderer_pixeltest.cc (right):

https://codereview.chromium.org/12558003/diff/11001/cc/gl_renderer_pixeltest....
cc/gl_renderer_pixeltest.cc:67: bool PixelsMatchReference(base::FilePath
ref_file, gfx::Rect viewport_rect,
This code moved to cc/test/pixel_test.h|cc

[ernstm, 2013-03-18 20:19:39]

Rebase to tip of tree. Made changes requested by slavi and reveman.

https://codereview.chromium.org/12558003/diff/11001/cc/gl_renderer_pixeltest.cc
File cc/gl_renderer_pixeltest.cc (right):

https://codereview.chromium.org/12558003/diff/11001/cc/gl_renderer_pixeltest....
cc/gl_renderer_pixeltest.cc:67: bool PixelsMatchReference(base::FilePath
ref_file, gfx::Rect viewport_rect,
On 2013/03/18 17:45:22, danakj wrote:
> This code moved to cc/test/pixel_test.h|cc

Done.

https://codereview.chromium.org/12558003/diff/11001/cc/test/pixel_comparator.cc
File cc/test/pixel_comparator.cc (right):

https://codereview.chromium.org/12558003/diff/11001/cc/test/pixel_comparator....
cc/test/pixel_comparator.cc:100: std::max(SkColorGetR(actual_color),
SkColorGetR(expected_color)) -
On 2013/03/08 19:49:04, slavi wrote:
> Why not
> 
> std::abs(SkColorGetR(actual_color) - SkColorGetR(expected_color))
> 
> ?

Done.

https://codereview.chromium.org/12558003/diff/11001/cc/test/pixel_comparator....
cc/test/pixel_comparator.cc:116: if (abs_error_r > max_abs_error_r)
max_abs_error_r = abs_error_r;
On 2013/03/08 19:49:04, slavi wrote:
> max_abs_error_r = std::max(max_abs_error_r, abs_error_r);

Done.

https://codereview.chromium.org/12558003/diff/11001/cc/test/pixel_comparator....
cc/test/pixel_comparator.cc:118: if (abs_error_b > max_abs_error_b)
max_abs_error_b = abs_error_b;
On 2013/03/08 04:41:57, David Reveman wrote:
> nit: put max_abs_error_? = abs_error_?; statements on next line.

Done.

[commit-bot: I haz the power, 2013-03-27 20:48:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ernstm@chromium.org/12558003/30001

[commit-bot: I haz the power, 2013-03-27 21:00:56]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[ernstm, 2013-03-27 21:50:43]

Updatd layer_unitests.cc to use new cc::MatchesPNGFile instead of
cc::IsSameAsPNGFile

[piman, 2013-03-28 20:22:47]

lgtm

[ernstm, 2013-03-28 22:03:41]

Rebase to tip of tree. Added discard_alpha flag.

[danakj, 2013-03-28 22:17:24]

LGTM+nits

https://codereview.chromium.org/12558003/diff/41001/cc/test/pixel_comparator.cc
File cc/test/pixel_comparator.cc (right):

https://codereview.chromium.org/12558003/diff/41001/cc/test/pixel_comparator....
cc/test/pixel_comparator.cc:39: ++error_pixels_count;
How about logging the pixel differences here as well, like we do on TOT now?
Saves a step in debugging when something goes wrong.

https://codereview.chromium.org/12558003/diff/41001/cc/test/pixel_comparator.h
File cc/test/pixel_comparator.h (right):

https://codereview.chromium.org/12558003/diff/41001/cc/test/pixel_comparator....
cc/test/pixel_comparator.h:26: ExactPixelComparator(const bool discard_alpha);
explicit.

this should be covered by presubmit linter now if you fetch/rebase/upload again.

https://codereview.chromium.org/12558003/diff/41001/cc/test/pixel_comparator....
cc/test/pixel_comparator.h:57: // Limit for percentage of pixels with an error
nit: periods at end of sentences while you're fixing the explicit please

https://codereview.chromium.org/12558003/diff/41001/cc/test/pixel_test_utils.h
File cc/test/pixel_test_utils.h (right):

https://codereview.chromium.org/12558003/diff/41001/cc/test/pixel_test_utils....
cc/test/pixel_test_utils.h:26: bool MatchesPNGFile(const SkBitmap& gen_bmp,
base::FilePath ref_img_path,
nit: one argument per line

[danakj, 2013-03-28 22:18:57]

https://codereview.chromium.org/12558003/diff/41001/cc/test/pixel_comparator.cc
File cc/test/pixel_comparator.cc (right):

https://codereview.chromium.org/12558003/diff/41001/cc/test/pixel_comparator....
cc/test/pixel_comparator.cc:39: ++error_pixels_count;
On 2013/03/28 22:17:25, danakj wrote:
> How about logging the pixel differences here as well, like we do on TOT now?
> Saves a step in debugging when something goes wrong.

Or, I guess we don't on TOT. We would when
https://codereview.chromium.org/12571010/ lands. I can add it with that, so
don't worry about it.

[ernstm, 2013-03-28 23:10:26]

Added detailed logging of error pixels. Made coding style changes requested in
#15.

https://codereview.chromium.org/12558003/diff/41001/cc/test/pixel_comparator.cc
File cc/test/pixel_comparator.cc (right):

https://codereview.chromium.org/12558003/diff/41001/cc/test/pixel_comparator....
cc/test/pixel_comparator.cc:39: ++error_pixels_count;
On 2013/03/28 22:17:25, danakj wrote:
> How about logging the pixel differences here as well, like we do on TOT now?
> Saves a step in debugging when something goes wrong.

Done.

https://codereview.chromium.org/12558003/diff/41001/cc/test/pixel_comparator.h
File cc/test/pixel_comparator.h (right):

https://codereview.chromium.org/12558003/diff/41001/cc/test/pixel_comparator....
cc/test/pixel_comparator.h:26: ExactPixelComparator(const bool discard_alpha);
On 2013/03/28 22:17:25, danakj wrote:
> explicit.
> 
> this should be covered by presubmit linter now if you fetch/rebase/upload
again.

Done.

https://codereview.chromium.org/12558003/diff/41001/cc/test/pixel_comparator....
cc/test/pixel_comparator.h:57: // Limit for percentage of pixels with an error
On 2013/03/28 22:17:25, danakj wrote:
> nit: periods at end of sentences while you're fixing the explicit please

Done.

[commit-bot: I haz the power, 2013-03-28 23:18:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ernstm@chromium.org/12558003/50001

[commit-bot: I haz the power, 2013-03-29 04:09:52]

Change committed as 191288