rebaseline_server: use just skpdiff, not Python Image Library

rebaseline_server: use just skpdiff, not Python Image Library

BUG=skia:2414
Committed: https://skia.googlesource.com/skia/+/54f1ad8bb5bdd2ac2ea7981427abeb193383d449

[epoger, 2014-06-12 07:02:07]

https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagedi...
File gm/rebaseline_server/imagediffdb.py (right):

https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagedi...
gm/rebaseline_server/imagediffdb.py:1: #!/usr/bin/python
Toplevel comments:

This CL is not ready for line-by-line review yet.  Instead, I'd like to use it
as fodder for discussion.

Before this CL, rebaseline_server uses two subsystems to generate diffs:
1. Python Image Library to generate diff images
2. skpdiff to calculate perceptual diff measurements

My guess was that this was wasteful, and it would be faster to just call skpdiff
to do *everything* rather than perform any image-diffing operations in
Python-land.

However, it seems like it is actually SLIGHTLY SLOWER after this CL.
Before this CL: took 128 seconds to prepare diffs for all 55 builders
After this CL: took 130 seconds

I tried to make sure that all of the source images were already downloaded to
local disk before launching those tests, so I think all that time is being spent
on performing the diffs, not downloading files.  (But I should make absolutely
sure of that.)

I have these hopes for improving performance in an skpdiff world, which we could
not do in a PIL world:
1. Send image pairs into skpdiff in batches, rather than one pair at a time
2. Tweak performance of skpdiff binary (including building it in Release mode,
not Debug)

I hope we get a chance to discuss this live sometime soon.

Also- right now, I have 2 rebaseline_server instances running, one with the old
code (before this CL) and one with the new code.  Feel free to check 'em out...
I think their results are identical.
Before: http://172.24.179.183:1111
After: http://172.24.179.183:2222

https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagedi...
gm/rebaseline_server/imagediffdb.py:104: # EPOGER: check for 80 columns
throughout
These EPOGERs indicate changes that I *know* I want to make before committing
this CL.

https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagedi...
gm/rebaseline_server/imagediffdb.py:123: '--differs', 'perceptual',
'different_pixels',
This is one cause of the slowdown... we just to tell skpdiff to only run the
"perceptual" differ; now we tell it to also run the "different_pixels" differ.

https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagedi...
gm/rebaseline_server/imagediffdb.py:166: # EPOGER: instead of
_find_single_file_in_dir(), we can now use
If we could make skpdiff write its output directly into the desired directory,
that would make us a bit faster...

https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagedi...
gm/rebaseline_server/imagediffdb.py:174: shutil.rmtree(skpdiff_output_dir)
All of these writing files and then deleting directories must be a waste of
time...

https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagepa...
File gm/rebaseline_server/imagepair_test.py (right):

https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagepa...
gm/rebaseline_server/imagepair_test.py:90: 'perceptualDifference':
0.06620300000000157,
There are minute changes to the perceptual differences we see after this CL.  I
guess that's because we now read that value out of skpdiff's JSON output file,
not its CSV output file.

https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/testdat...
File
gm/rebaseline_server/testdata/outputs/expected/compare_to_expectations_test.CompareToExpectationsTest.test_gm/gm.json
(left):

https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/testdat...
gm/rebaseline_server/testdata/outputs/expected/compare_to_expectations_test.CompareToExpectationsTest.test_gm/gm.json:205:
"differenceData": {
I figured out that the two missing differenceData chunks in this unittest
correspond to image pairs with mismatched sizes.  skpdiff totally barfs (like,
totally) when you give it a pair of images with different sizes.

So we get different behavior for mismatched-size pairs now.  But I'm not sure
whether the old or new behavior is better anyway.

https://codereview.chromium.org/325413003/diff/1/tools/skpdiff/SkDiffContext.cpp
File tools/skpdiff/SkDiffContext.cpp (right):

https://codereview.chromium.org/325413003/diff/1/tools/skpdiff/SkDiffContext....
tools/skpdiff/SkDiffContext.cpp:319: stream.writeText("           
\"maxRedDiff\": ");
Here I jammed in the various parameters that I had previously been getting from
PIL, into skpdiff's output.  I don't know what the cleanest way to do this would
be...

https://codereview.chromium.org/325413003/diff/1/tools/skpdiff/SkDifferentPix...
File tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp (right):

https://codereview.chromium.org/325413003/diff/1/tools/skpdiff/SkDifferentPix...
tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp:87: uint32_t redDiff =
abs(SkColorGetR(baselinePixel) - SkColorGetR(testPixel));
This section of the code seems like a good place to look for performance
killers.

[jcgregorio, 2014-06-12 12:58:00]

On 2014/06/12 07:02:07, epoger wrote:
>
https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagedi...
> File gm/rebaseline_server/imagediffdb.py (right):
> 
>
https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagedi...
> gm/rebaseline_server/imagediffdb.py:1: #!/usr/bin/python
> Toplevel comments:
> 
> This CL is not ready for line-by-line review yet.  Instead, I'd like to use it
> as fodder for discussion.
> 
> Before this CL, rebaseline_server uses two subsystems to generate diffs:
> 1. Python Image Library to generate diff images
> 2. skpdiff to calculate perceptual diff measurements
> 
> My guess was that this was wasteful, and it would be faster to just call
skpdiff
> to do *everything* rather than perform any image-diffing operations in
> Python-land.
> 
> However, it seems like it is actually SLIGHTLY SLOWER after this CL.
> Before this CL: took 128 seconds to prepare diffs for all 55 builders
> After this CL: took 130 seconds
> 
> I tried to make sure that all of the source images were already downloaded to
> local disk before launching those tests, so I think all that time is being
spent
> on performing the diffs, not downloading files.  (But I should make absolutely
> sure of that.)
> 
> I have these hopes for improving performance in an skpdiff world, which we
could
> not do in a PIL world:
> 1. Send image pairs into skpdiff in batches, rather than one pair at a time
> 2. Tweak performance of skpdiff binary (including building it in Release mode,
> not Debug)
> 
> I hope we get a chance to discuss this live sometime soon.

Maybe use multiprocessing to run diffs in parallel?

https://docs.python.org/2/library/multiprocessing.html

> 
> Also- right now, I have 2 rebaseline_server instances running, one with the
old
> code (before this CL) and one with the new code.  Feel free to check 'em
out...
> I think their results are identical.
> Before: http://172.24.179.183:1111
> After: http://172.24.179.183:2222
> 
>
https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagedi...
> gm/rebaseline_server/imagediffdb.py:104: # EPOGER: check for 80 columns
> throughout
> These EPOGERs indicate changes that I *know* I want to make before committing
> this CL.
> 
>
https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagedi...
> gm/rebaseline_server/imagediffdb.py:123: '--differs', 'perceptual',
> 'different_pixels',
> This is one cause of the slowdown... we just to tell skpdiff to only run the
> "perceptual" differ; now we tell it to also run the "different_pixels" differ.
> 
>
https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagedi...
> gm/rebaseline_server/imagediffdb.py:166: # EPOGER: instead of
> _find_single_file_in_dir(), we can now use
> If we could make skpdiff write its output directly into the desired directory,
> that would make us a bit faster...
> 
>
https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagedi...
> gm/rebaseline_server/imagediffdb.py:174: shutil.rmtree(skpdiff_output_dir)
> All of these writing files and then deleting directories must be a waste of
> time...
> 
>
https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagepa...
> File gm/rebaseline_server/imagepair_test.py (right):
> 
>
https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagepa...
> gm/rebaseline_server/imagepair_test.py:90: 'perceptualDifference':
> 0.06620300000000157,
> There are minute changes to the perceptual differences we see after this CL. 
I
> guess that's because we now read that value out of skpdiff's JSON output file,
> not its CSV output file.
> 
>
https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/testdat...
> File
>
gm/rebaseline_server/testdata/outputs/expected/compare_to_expectations_test.CompareToExpectationsTest.test_gm/gm.json
> (left):
> 
>
https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/testdat...
>
gm/rebaseline_server/testdata/outputs/expected/compare_to_expectations_test.CompareToExpectationsTest.test_gm/gm.json:205:
> "differenceData": {
> I figured out that the two missing differenceData chunks in this unittest
> correspond to image pairs with mismatched sizes.  skpdiff totally barfs (like,
> totally) when you give it a pair of images with different sizes.
> 
> So we get different behavior for mismatched-size pairs now.  But I'm not sure
> whether the old or new behavior is better anyway.
> 
>
https://codereview.chromium.org/325413003/diff/1/tools/skpdiff/SkDiffContext.cpp
> File tools/skpdiff/SkDiffContext.cpp (right):
> 
>
https://codereview.chromium.org/325413003/diff/1/tools/skpdiff/SkDiffContext....
> tools/skpdiff/SkDiffContext.cpp:319: stream.writeText("           
> \"maxRedDiff\": ");
> Here I jammed in the various parameters that I had previously been getting
from
> PIL, into skpdiff's output.  I don't know what the cleanest way to do this
would
> be...
> 
>
https://codereview.chromium.org/325413003/diff/1/tools/skpdiff/SkDifferentPix...
> File tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp (right):
> 
>
https://codereview.chromium.org/325413003/diff/1/tools/skpdiff/SkDifferentPix...
> tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp:87: uint32_t redDiff =
> abs(SkColorGetR(baselinePixel) - SkColorGetR(testPixel));
> This section of the code seems like a good place to look for performance
> killers.

[djsollen, 2014-06-12 13:27:00]

https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagedi...
File gm/rebaseline_server/imagediffdb.py (right):

https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagedi...
gm/rebaseline_server/imagediffdb.py:1: #!/usr/bin/python
On 2014/06/12 07:02:07, epoger wrote:
> Toplevel comments:
> 
> This CL is not ready for line-by-line review yet.  Instead, I'd like to use it
> as fodder for discussion.
> 
> Before this CL, rebaseline_server uses two subsystems to generate diffs:
> 1. Python Image Library to generate diff images
> 2. skpdiff to calculate perceptual diff measurements
> 
> My guess was that this was wasteful, and it would be faster to just call
skpdiff
> to do *everything* rather than perform any image-diffing operations in
> Python-land.
> 
> However, it seems like it is actually SLIGHTLY SLOWER after this CL.
> Before this CL: took 128 seconds to prepare diffs for all 55 builders
> After this CL: took 130 seconds
> 
> I tried to make sure that all of the source images were already downloaded to
> local disk before launching those tests, so I think all that time is being
spent
> on performing the diffs, not downloading files.  (But I should make absolutely
> sure of that.)
> 
> I have these hopes for improving performance in an skpdiff world, which we
could
> not do in a PIL world:
> 1. Send image pairs into skpdiff in batches, rather than one pair at a time
> 2. Tweak performance of skpdiff binary (including building it in Release mode,
> not Debug)

Building in Release should get a speed up in that the compiler should optimize
more, but looking through the code I didn't see any ASSERTS in the tight loops. 
The other promising thing to do would be to use the multithreading option.

> 
> I hope we get a chance to discuss this live sometime soon.
> 
> Also- right now, I have 2 rebaseline_server instances running, one with the
old
> code (before this CL) and one with the new code.  Feel free to check 'em
out...
> I think their results are identical.
> Before: http://172.24.179.183:1111
> After: http://172.24.179.183:2222

https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagedi...
gm/rebaseline_server/imagediffdb.py:123: '--differs', 'perceptual',
'different_pixels',
On 2014/06/12 07:02:07, epoger wrote:
> This is one cause of the slowdown... we just to tell skpdiff to only run the
> "perceptual" differ; now we tell it to also run the "different_pixels" differ.

If you're not specifying the number of threads it should default to using as
many as it can.  Which should run those in diffs in parallel, if you detect it
isn't then we need to fix that.

This should be much slower than running them all in one shot as the perceptual
diff always takes longer and so some of the time it is running on a single core.
 If you do them all at once then the all cores should stay busier longer getting
you better throughput.

https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagedi...
gm/rebaseline_server/imagediffdb.py:166: # EPOGER: instead of
_find_single_file_in_dir(), we can now use
On 2014/06/12 07:02:07, epoger wrote:
> If we could make skpdiff write its output directly into the desired directory,
> that would make us a bit faster...

That shouldn't be too hard.

https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/testdat...
File
gm/rebaseline_server/testdata/outputs/expected/compare_to_expectations_test.CompareToExpectationsTest.test_gm/gm.json
(left):

https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/testdat...
gm/rebaseline_server/testdata/outputs/expected/compare_to_expectations_test.CompareToExpectationsTest.test_gm/gm.json:205:
"differenceData": {
On 2014/06/12 07:02:07, epoger wrote:
> I figured out that the two missing differenceData chunks in this unittest
> correspond to image pairs with mismatched sizes.  skpdiff totally barfs (like,
> totally) when you give it a pair of images with different sizes.
> 
> So we get different behavior for mismatched-size pairs now.  But I'm not sure
> whether the old or new behavior is better anyway.

We should update skpdiff to fail gracefully. File a bug if you want me to look
into it.

https://codereview.chromium.org/325413003/diff/1/tools/skpdiff/SkDiffContext.h
File tools/skpdiff/SkDiffContext.h (right):

https://codereview.chromium.org/325413003/diff/1/tools/skpdiff/SkDiffContext....
tools/skpdiff/SkDiffContext.h:100: *    "whiteDiffPath"  : (optional) string
containing the path to a bitmap
Why do you need a whiteDiffPath?  It stores the exact same info as the alpha
mask in differencePath.  In the UI you should then be able to just draw a white
div the size of the image and then draw the alpha mask image overtop.

https://codereview.chromium.org/325413003/diff/1/tools/skpdiff/SkDifferentPix...
File tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp (right):

https://codereview.chromium.org/325413003/diff/1/tools/skpdiff/SkDifferentPix...
tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp:16: * pair.
isn't that the same as adding them explicitly to the Results struct with a way
to indicate they are optional?  Seems like that would be more efficient than
maintaining an arbitrary list while ensuring that the diff tools only set values
that the reader knows how to parse.

[mtklein, 2014-06-12 13:42:00]

> Before this CL, rebaseline_server uses two subsystems to generate diffs:
> 1. Python Image Library to generate diff images
> 2. skpdiff to calculate perceptual diff measurements
> 
> My guess was that this was wasteful, and it would be faster to just call
skpdiff
> to do *everything* rather than perform any image-diffing operations in
> Python-land.

PIL is mostly implemented in C.  There's no inherent waste implied by using PIL,
but it's a fairly hopeful sign that our Debug-mode compiled skpdiff is
competitive with the platform-provided optimized PIL.  I do imagine that skpdiff
could run much faster when compiled with -O3 (I agree, NDEBUG probably doesn't
matter), and because it's under our control we can always pour more effort
making it faster.  That differing-pixels metric, for instance, certainly can be
vectorized.

Are you running this all under a profiler?  It's hard to beat profiling to see
where the time's spent.  I'd be happy to show you how to use perf or
Instruments.

[epoger, 2014-06-12 14:02:35]

Thanks, all!  I'll come pester you for followup discussion shortly.

https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagedi...
File gm/rebaseline_server/imagediffdb.py (right):

https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagedi...
gm/rebaseline_server/imagediffdb.py:1: #!/usr/bin/python
> > I have these hopes for improving performance in an skpdiff world, which we
> could
> > not do in a PIL world:
> > 1. Send image pairs into skpdiff in batches, rather than one pair at a time
> > 2. Tweak performance of skpdiff binary (including building it in Release
mode,
> > not Debug)
> 
> Building in Release should get a speed up in that the compiler should optimize
> more, but looking through the code I didn't see any ASSERTS in the tight
loops. 
> The other promising thing to do would be to use the multithreading option.

Right.  I guess to get the most out of multithreading (within skpdiff, I guess
you mean), I'll need to pass a whole batch of image pairs into skpdiff.  Which
will present its own problems.  But I should be able to take advantage of that
in a future CL.

I also want to re-evaluate performance of this at a high level-- I want to
figure out where most of the time is going.  I am beginning to suspect that the
perceptual diff metric is our performance bottleneck, and I don't think it's
providing all that much value (compared to simpler metrics like the old
weightedDiffMeasure).

Also, I'm sure there's lots of fat to trim from the "first run" case (which
downloads all the needed image files), which is probably most painful for
developers doing rebaselines.

https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagedi...
gm/rebaseline_server/imagediffdb.py:1: #!/usr/bin/python
On 2014/06/12 13:42:00, mtklein wrote:
> > Before this CL, rebaseline_server uses two subsystems to generate diffs:
> > 1. Python Image Library to generate diff images
> > 2. skpdiff to calculate perceptual diff measurements
> > 
> > My guess was that this was wasteful, and it would be faster to just call
> skpdiff
> > to do *everything* rather than perform any image-diffing operations in
> > Python-land.
> 
> PIL is mostly implemented in C.  There's no inherent waste implied by using
PIL,
> but it's a fairly hopeful sign that our Debug-mode compiled skpdiff is
> competitive with the platform-provided optimized PIL.

My main concern is that we are passing the same image pairs to TWO places (PIL
and skpdiff) and thus duplicating much of the work (looping over all the pixels
and comparing them).

I think it's also possible that doing everything in PIL and kicking skpdiff to
the curb would work reasonably well.  Another way of thinking about that: making
our own contributions to the PIL native code to perform the particular
operations we want.

> Are you running this all under a profiler?  It's hard to beat profiling to see
> where the time's spent.  I'd be happy to show you how to use perf or
> Instruments.

No, I haven't been, and I agree that would be good to do.  Thank you for the
offer.  Let's see where this goes--after my vacation I may well take you up on
it.

https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagedi...
gm/rebaseline_server/imagediffdb.py:123: '--differs', 'perceptual',
'different_pixels',
On 2014/06/12 13:27:00, djsollen wrote:
> On 2014/06/12 07:02:07, epoger wrote:
> > This is one cause of the slowdown... we just to tell skpdiff to only run the
> > "perceptual" differ; now we tell it to also run the "different_pixels"
differ.
> 
> If you're not specifying the number of threads it should default to using as
> many as it can.  Which should run those in diffs in parallel, if you detect it
> isn't then we need to fix that.
> 
> This should be much slower than running them all in one shot as the perceptual
> diff always takes longer and so some of the time it is running on a single
core.
>  If you do them all at once then the all cores should stay busier longer
getting
> you better throughput.

Good explanation, thanks.  I definitely want to explore batching multiple image
pairs.  I think we can leave that for a future CL.

https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/imagedi...
gm/rebaseline_server/imagediffdb.py:166: # EPOGER: instead of
_find_single_file_in_dir(), we can now use
On 2014/06/12 13:26:59, djsollen wrote:
> On 2014/06/12 07:02:07, epoger wrote:
> > If we could make skpdiff write its output directly into the desired
directory,
> > that would make us a bit faster...
> 
> That shouldn't be too hard.

The main problem is that right now you can tell skpdiff the DIRECTORY to write
its files into, but not the FILENAME to use.

https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/testdat...
File
gm/rebaseline_server/testdata/outputs/expected/compare_to_expectations_test.CompareToExpectationsTest.test_gm/gm.json
(left):

https://codereview.chromium.org/325413003/diff/1/gm/rebaseline_server/testdat...
gm/rebaseline_server/testdata/outputs/expected/compare_to_expectations_test.CompareToExpectationsTest.test_gm/gm.json:205:
"differenceData": {
On 2014/06/12 13:27:00, djsollen wrote:
> On 2014/06/12 07:02:07, epoger wrote:
> > I figured out that the two missing differenceData chunks in this unittest
> > correspond to image pairs with mismatched sizes.  skpdiff totally barfs
(like,
> > totally) when you give it a pair of images with different sizes.
> > 
> > So we get different behavior for mismatched-size pairs now.  But I'm not
sure
> > whether the old or new behavior is better anyway.
> 
> We should update skpdiff to fail gracefully. File a bug if you want me to look
> into it.

It's actually already pretty clear when skpdiff failed (not so clear WHY it
failed--image dimension mismatch, or what).  Yes, I think skpdiff could stand to
be improved a bit in that department but I think it's decent already.

The good news is that the end result is fine.  If you diff two images with
differing dimensions, you're gonna have a bad time.

https://codereview.chromium.org/325413003/diff/1/tools/skpdiff/SkDiffContext.h
File tools/skpdiff/SkDiffContext.h (right):

https://codereview.chromium.org/325413003/diff/1/tools/skpdiff/SkDiffContext....
tools/skpdiff/SkDiffContext.h:100: *    "whiteDiffPath"  : (optional) string
containing the path to a bitmap
On 2014/06/12 13:27:00, djsollen wrote:
> Why do you need a whiteDiffPath?  It stores the exact same info as the alpha
> mask in differencePath.  In the UI you should then be able to just draw a
white
> div the size of the image and then draw the alpha mask image overtop.

I'll try that.  A big concern for me is making it as easy as possible for the
client to render the comparison rows.  Do you think the alpha mask will perform
just about as well as a standard single bitmap on constrained platforms,
rendering 50 per page?  (I don't know how expensive the alpha blends are)

https://codereview.chromium.org/325413003/diff/1/tools/skpdiff/SkDifferentPix...
File tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp (right):

https://codereview.chromium.org/325413003/diff/1/tools/skpdiff/SkDifferentPix...
tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp:16: * pair.
On 2014/06/12 13:27:00, djsollen wrote:
> isn't that the same as adding them explicitly to the Results struct with a way
> to indicate they are optional?  Seems like that would be more efficient than
> maintaining an arbitrary list while ensuring that the diff tools only set
values
> that the reader knows how to parse.

Yeah, that seems quite reasonable, thanks for the idea.

I look forward to discussing with you the pros and cons of the
various-differs-running-in-parallel model that is currently used in skpdiff.  It
bugs me that multiple threads are simultaneously iterating over the same image
pairs... seems like they are wasting throughput to improve latency, and it makes
the process as a whole take longer.

[epoger, 2014-06-12 14:39:41]

Summary of live discussions with mtklein/djsollen: I'm going to clean this up,
and get it committed.  Moving the operations from PIL into our skpdiff native
code will give us a better opportunity to speed it up.

[epoger, 2014-06-12 16:48:14]

Derek (and anyone else)- ready for line-by-line review at patchset 5.

To see behavior before and after this CL (should be almost identical):
BEFORE: http://172.24.179.183:1111
AFTER: http://172.24.179.183:2222

[epoger, 2014-06-12 19:03:33]

Eric- can you please review the Python bits?  Derek is reviewing the native
code.

[djsollen, 2014-06-12 19:11:16]

https://codereview.chromium.org/325413003/diff/100001/tools/skpdiff/SkDiffCon...
File tools/skpdiff/SkDiffContext.h (right):

https://codereview.chromium.org/325413003/diff/100001/tools/skpdiff/SkDiffCon...
tools/skpdiff/SkDiffContext.h:152: int                       fWidth;
SkSize instead of 2 fields?

https://codereview.chromium.org/325413003/diff/100001/tools/skpdiff/SkDiffCon...
tools/skpdiff/SkDiffContext.h:154: int                  fMaxRedDiff;
if these fields are optional how do you tell if they were filled in?

https://codereview.chromium.org/325413003/diff/100001/tools/skpdiff/SkImageDi...
File tools/skpdiff/SkImageDiffer.h (right):

https://codereview.chromium.org/325413003/diff/100001/tools/skpdiff/SkImageDi...
tools/skpdiff/SkImageDiffer.h:30: int maxRedDiff; // optional
should this be stored as one color and you just put the max diff for each
channel in the appropriate spot?

Also since this field is optional how do you know if the diff wrote to it?

https://codereview.chromium.org/325413003/diff/100001/tools/skpdiff/SkImageDi...
tools/skpdiff/SkImageDiffer.h:56: virtual bool diff(SkBitmap* baseline,
SkBitmap* test, bool computeAlphaMask,
lets pass another struct (or bitfield) that represents all of those bools.

[borenet, 2014-06-12 19:19:04]

Python code looks fine to me.

https://codereview.chromium.org/325413003/diff/100001/gm/rebaseline_server/im...
File gm/rebaseline_server/imagediffdb.py (right):

https://codereview.chromium.org/325413003/diff/100001/gm/rebaseline_server/im...
gm/rebaseline_server/imagediffdb.py:137: record['maxRedDiff'],
record['maxGreenDiff'], record['maxBlueDiff']]
Could this be a tuple instead of a list? (or even a NamedTuple?)

[epoger, 2014-06-12 20:04:58]

Thanks, guys.  Please see patchset 6.

https://codereview.chromium.org/325413003/diff/100001/gm/rebaseline_server/im...
File gm/rebaseline_server/imagediffdb.py (right):

https://codereview.chromium.org/325413003/diff/100001/gm/rebaseline_server/im...
gm/rebaseline_server/imagediffdb.py:137: record['maxRedDiff'],
record['maxGreenDiff'], record['maxBlueDiff']]
On 2014/06/12 19:19:04, borenet wrote:
> Could this be a tuple instead of a list? (or even a NamedTuple?)

Added a TODO.  For now, I wanted to keep the changes limited to imagediffdb.py
and its innards; changing this list to a tuple will affect users of
imagediffdb.py.

https://codereview.chromium.org/325413003/diff/100001/tools/skpdiff/SkDiffCon...
File tools/skpdiff/SkDiffContext.h (right):

https://codereview.chromium.org/325413003/diff/100001/tools/skpdiff/SkDiffCon...
tools/skpdiff/SkDiffContext.h:152: int                       fWidth;
On 2014/06/12 19:11:15, djsollen wrote:
> SkSize instead of 2 fields?

SkBitmap has integer width and height, not SkScalar (as would be wrapped in
SkSize).  So I changed it to use SkISize.

https://codereview.chromium.org/325413003/diff/100001/tools/skpdiff/SkDiffCon...
tools/skpdiff/SkDiffContext.h:154: int                  fMaxRedDiff;
On 2014/06/12 19:11:16, djsollen wrote:
> if these fields are optional how do you tell if they were filled in?

Good question.  Added a TODO.

https://codereview.chromium.org/325413003/diff/100001/tools/skpdiff/SkImageDi...
File tools/skpdiff/SkImageDiffer.h (right):

https://codereview.chromium.org/325413003/diff/100001/tools/skpdiff/SkImageDi...
tools/skpdiff/SkImageDiffer.h:30: int maxRedDiff; // optional
On 2014/06/12 19:11:16, djsollen wrote:
> should this be stored as one color and you just put the max diff for each
> channel in the appropriate spot?

I guess you mean as an SkColor, with each byte inside showing the max
per-channel diff?

We *could* do that, but the way this information is used currently is a
human-readable report (e.g., "120, 0, 45").  So I think that would be a step in
the wrong direction.
 
> Also since this field is optional how do you know if the diff wrote to it?

Good question.  Added a TODO.

https://codereview.chromium.org/325413003/diff/100001/tools/skpdiff/SkImageDi...
tools/skpdiff/SkImageDiffer.h:56: virtual bool diff(SkBitmap* baseline,
SkBitmap* test, bool computeAlphaMask,
On 2014/06/12 19:11:16, djsollen wrote:
> lets pass another struct (or bitfield) that represents all of those bools. 

Done.

[djsollen, 2014-06-12 20:11:08]

https://codereview.chromium.org/325413003/diff/120001/tools/skpdiff/SkDiffCon...
File tools/skpdiff/SkDiffContext.h (right):

https://codereview.chromium.org/325413003/diff/120001/tools/skpdiff/SkDiffCon...
tools/skpdiff/SkDiffContext.h:155: SkSize                     fSize;
SkISize?

https://codereview.chromium.org/325413003/diff/120001/tools/skpdiff/SkDiffere...
File tools/skpdiff/SkDifferentPixelsMetric.h (right):

https://codereview.chromium.org/325413003/diff/120001/tools/skpdiff/SkDiffere...
tools/skpdiff/SkDifferentPixelsMetric.h:32: const
SkImageDiffer::BitmapsToCreate& bitmapsToCreate,
shouldn't need SkImageDiffer::

https://codereview.chromium.org/325413003/diff/120001/tools/skpdiff/SkDiffere...
File tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp (right):

https://codereview.chromium.org/325413003/diff/120001/tools/skpdiff/SkDiffere...
tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp:18: const
SkImageDiffer::BitmapsToCreate& bitmapsToCreate,
I don't think you need SkImageDiffer:: here

[epoger, 2014-06-12 20:18:57]

https://codereview.chromium.org/325413003/diff/120001/tools/skpdiff/SkDiffCon...
File tools/skpdiff/SkDiffContext.h (right):

https://codereview.chromium.org/325413003/diff/120001/tools/skpdiff/SkDiffCon...
tools/skpdiff/SkDiffContext.h:155: SkSize                     fSize;
On 2014/06/12 20:11:07, djsollen wrote:
> SkISize?

Done.

https://codereview.chromium.org/325413003/diff/120001/tools/skpdiff/SkDiffere...
File tools/skpdiff/SkDifferentPixelsMetric.h (right):

https://codereview.chromium.org/325413003/diff/120001/tools/skpdiff/SkDiffere...
tools/skpdiff/SkDifferentPixelsMetric.h:32: const
SkImageDiffer::BitmapsToCreate& bitmapsToCreate,
On 2014/06/12 20:11:07, djsollen wrote:
> shouldn't need SkImageDiffer::

Done.

https://codereview.chromium.org/325413003/diff/120001/tools/skpdiff/SkDiffere...
File tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp (right):

https://codereview.chromium.org/325413003/diff/120001/tools/skpdiff/SkDiffere...
tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp:18: const
SkImageDiffer::BitmapsToCreate& bitmapsToCreate,
On 2014/06/12 20:11:07, djsollen wrote:
> I don't think you need SkImageDiffer:: here

Done.

[epoger, 2014-06-17 10:16:03]

Hey, Derek- Do you think this is commitable?  I'd like to commit it soon if
possible, before the patch gets out of date.

[epoger, 2014-07-01 16:51:56]

Patchset 8 rebases to current tip of tree (since it's been 2 weeks since I last
asked for an LG on this)

I am also seeing some presubmit warnings now (pylint failures), which I don't
remember seeing 2 weeks ago... I will upload another patchset later today to fix
those.

[epoger, 2014-07-01 17:03:36]

Eric/Mike/Ravi- I'd like to go ahead and commit this.  I think it passed Derek's
muster, although he didn't give me an LG before going on break.

[borenet, 2014-07-01 17:17:22]

Python still LGTM.  The rest of it seems fine to me as well, but I'm not very
familiar with that code.  You added a number of TODOs without an owner - usually
the linter complains about that, right?

https://codereview.chromium.org/325413003/diff/180001/tools/skpdiff/SkDiffCon...
File tools/skpdiff/SkDiffContext.cpp (right):

https://codereview.chromium.org/325413003/diff/180001/tools/skpdiff/SkDiffCon...
tools/skpdiff/SkDiffContext.cpp:283: // TODO: Would it be better to use the
jsoncpp library to write out the JSON?
TODO with no owner.

https://codereview.chromium.org/325413003/diff/180001/tools/skpdiff/SkDiffCon...
File tools/skpdiff/SkDiffContext.h (right):

https://codereview.chromium.org/325413003/diff/180001/tools/skpdiff/SkDiffCon...
tools/skpdiff/SkDiffContext.h:146: // TODO: Some of these fields are required,
while others are optional
TODO with no owner.

https://codereview.chromium.org/325413003/diff/180001/tools/skpdiff/SkDiffere...
File tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp (right):

https://codereview.chromium.org/325413003/diff/180001/tools/skpdiff/SkDiffere...
tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp:61: // TODO: The code below
already assumes 4 bytes per pixel, so I think
...

https://codereview.chromium.org/325413003/diff/180001/tools/skpdiff/SkDiffere...
tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp:69: // TODO: This loop looks like
a good place to work on performance,
...

[epoger, 2014-07-01 17:56:59]

On 2014/07/01 17:17:22, borenet wrote:
> Python still LGTM.  The rest of it seems fine to me as well, but I'm not very
> familiar with that code.  You added a number of TODOs without an owner -
usually
> the linter complains about that, right?

In patchset 10, I have attributed all the TODOs within the C code, and linked
them to bugs.  (The linter did not complain about the lack of attributions, and
I've been seeing unattributed TODOs in other peoples' CLs, so I thought I didn't
need them.  But I think it's better to have them.)

Anyone object to me committing this?  Speak now, or forever hold your peace.

[epoger, 2014-07-02 13:37:55]

The CQ bit was checked by epoger@google.com

[commit-bot: I haz the power, 2014-07-02 13:38:24]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/epoger@google.com/325413003/200001

[commit-bot: I haz the power, 2014-07-02 13:51:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Build-Win7-VS2010-x86-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Win7-VS2010-x86-Debug-Trybot/build...)

[commit-bot: I haz the power, 2014-07-02 13:57:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-02 13:57:58]

Try jobs failed on following builders:
  Build-Win7-VS2010-x86-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Win7-VS2010-x86-Debug-Trybot/build...)

[epoger, 2014-07-02 14:29:35]

The CQ bit was checked by epoger@google.com

[commit-bot: I haz the power, 2014-07-02 14:30:37]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/epoger@google.com/325413003/220001

[commit-bot: I haz the power, 2014-07-02 14:43:08]

Change committed as 54f1ad8bb5bdd2ac2ea7981427abeb193383d449