Modify skpdiff to write diffs directly to provided directories

This eliminates the need to copy the generated images from a temporary directory to the directory that is served by the rebaseline_server.

BUG=skia:2815, skia:2818
Committed: https://skia.googlesource.com/skia/+/21b342d19c71fa5abe7e4aa5cfb518fd04cb9d67

[stephana, 2014-08-11 14:58:31]

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

https://codereview.chromium.org/457203003/diff/1/tools/skpdiff/SkDiffContext....
tools/skpdiff/SkDiffContext.cpp:101: }
This uses the <string> and <algorithm> headers included above. I didn't see a
lot of use of the C++ standard library in the Skia source - is that something to
be avoided ?

[epoger, 2014-08-11 18:51:41]

For starters, I removed NOTRY=true from the description.  Because this CL
includes C code, it is useful to run this through the trybots to watch for any
per-platform compiler peculiarities.  We see them too often. :-)

I'll be back soon with a real code review...

[epoger, 2014-08-11 19:33:28]

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

https://codereview.chromium.org/457203003/diff/1/gm/rebaseline_server/imagedi...
gm/rebaseline_server/imagediffdb.py:177: diff_image_locator =
_get_difference_locator(
I *think* this is the only call to _get_difference_locator(), so if we get rid
of it, I think we should delete _get_difference_locator() as well.

BUT, as my note within SkDiffContext.cpp says, I'm worried that moving the
filename-construction logic into skpdiff is not the right direction.

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

https://codereview.chromium.org/457203003/diff/1/gm/rebaseline_server/imagedi...
gm/rebaseline_server/imagediffdb.py:142: '--rgbDiffDir', skpdiff_rgbdiff_dir,
Instead of adding the --longnames parameter and allowing skpdiff to construct
the filenames, maybe we could do this instead?


diff_image_locator = _get_difference_locator(blah blah blah)
basename = str(diff_image_locator) + image_suffix
rgb_diff_path = os.path.join(storage_root, RGBDIFFS_SUBDIR, basename)
white_diff_path = os.path.join(storage_root, WHITEDIFFS_SUBDIR, basename)

find_run_binary.run_command(
    [SKPDIFF_BINARY, '-p', expected_image_file, actual_image_file,
     '--jsonp', 'false',
     '--output', skpdiff_summary_file,
     '--differs', 'perceptual', 'different_pixels',
     '--rgbDiffPath', rgb_diff_path,
     '--whiteDiffPath', white_diff_path,
     ])

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

https://codereview.chromium.org/457203003/diff/1/tools/skpdiff/SkDiffContext....
tools/skpdiff/SkDiffContext.cpp:98: result.append("-vs-");
Looking at _get_difference_locator() in imagediffdb.py, I see:

"We must keep this function in sync with getImageDiffRelativeUrl() in
static/loader.js"

I suspect that would be the case here too, right?

I worry that this change makes our code yet more fragile... now this naming
convention must be kept in sync between loader.js and this native skpdiff code
(which is even further away from loader.js than imagediffdb.py is)

Could we instead continue to construct the filename in imagediffdb.py, and pass
the fully constructed filename into skpdiff?  I think that would be more
robust...

https://codereview.chromium.org/457203003/diff/1/tools/skpdiff/SkDiffContext....
tools/skpdiff/SkDiffContext.cpp:101: }
On 2014/08/11 14:58:31, stephana wrote:
> This uses the <string> and <algorithm> headers included above. I didn't see a
> lot of use of the C++ standard library in the Skia source - is that something
to
> be avoided ? 

A fine question... in my experience, within the Skia code we have always favored
our homebrew library (SkString and such) rather than bringing in the C++
standard library.

In a tool like skpdiff (as opposed to the core API), it probably doesn't matter
either way... but I guess for the core API we don't want to depend on the C++
std library.

I recommend you ask Mike Reed for guidance on this.

In this particular case, it seems like SkString should offer a replace() method.
 Searching through our code (git grep replace | grep cpp), I find several
implementations:

src/utils/SkRTConf.cpp : static inline void str_replace(char *s, char search,
char replace)
tools/picture_utils.cpp : void replace_char(SkString* str, const char oldChar,
const char newChar)
tools/skimage_main.cpp : static void replace_char(SkString* str, const char
oldChar, const char newChar)

[stephana, 2014-08-11 20:03:57]

Thanks Elliot.

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

https://codereview.chromium.org/457203003/diff/1/gm/rebaseline_server/imagedi...
gm/rebaseline_server/imagediffdb.py:177: diff_image_locator =
_get_difference_locator(
Agreed.

On 2014/08/11 19:33:27, epoger wrote:
> I *think* this is the only call to _get_difference_locator(), so if we get rid
> of it, I think we should delete _get_difference_locator() as well.
> 
> BUT, as my note within SkDiffContext.cpp says, I'm worried that moving the
> filename-construction logic into skpdiff is not the right direction.

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

https://codereview.chromium.org/457203003/diff/1/gm/rebaseline_server/imagedi...
gm/rebaseline_server/imagediffdb.py:142: '--rgbDiffDir', skpdiff_rgbdiff_dir,
That was going to be my first approach (namely to specify the output files
directly).
It's fine when there are only two pairs of images, but if we want to compare
many in a single call to skpdiff I would have to provide a list of output files
and I thought it would become unwieldy and error prone. 


On 2014/08/11 19:33:27, epoger wrote:
> Instead of adding the --longnames parameter and allowing skpdiff to construct
> the filenames, maybe we could do this instead?
> 
> 
> diff_image_locator = _get_difference_locator(blah blah blah)
> basename = str(diff_image_locator) + image_suffix
> rgb_diff_path = os.path.join(storage_root, RGBDIFFS_SUBDIR, basename)
> white_diff_path = os.path.join(storage_root, WHITEDIFFS_SUBDIR, basename)
> 
> find_run_binary.run_command(
>     [SKPDIFF_BINARY, '-p', expected_image_file, actual_image_file,
>      '--jsonp', 'false',
>      '--output', skpdiff_summary_file,
>      '--differs', 'perceptual', 'different_pixels',
>      '--rgbDiffPath', rgb_diff_path,
>      '--whiteDiffPath', white_diff_path,
>      ])

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

https://codereview.chromium.org/457203003/diff/1/tools/skpdiff/SkDiffContext....
tools/skpdiff/SkDiffContext.cpp:98: result.append("-vs-");
Yes, I just realized that it breaks the front-end code. 
Ok, I'll construct the filename in imagediffdb.py and pass it to skpdiff. 

But I would also advocate to pass the relative URL of the diffs from the backend
to the frontend, instead of generating the name on the frontend. Getting rid of
that fragility. 

On 2014/08/11 19:33:28, epoger wrote:
> Looking at _get_difference_locator() in imagediffdb.py, I see:
> 
> "We must keep this function in sync with getImageDiffRelativeUrl() in
> static/loader.js"
> 
> I suspect that would be the case here too, right?
> 
> I worry that this change makes our code yet more fragile... now this naming
> convention must be kept in sync between loader.js and this native skpdiff code
> (which is even further away from loader.js than imagediffdb.py is)
> 
> Could we instead continue to construct the filename in imagediffdb.py, and
pass
> the fully constructed filename into skpdiff?  I think that would be more
> robust...

https://codereview.chromium.org/457203003/diff/1/tools/skpdiff/SkDiffContext....
tools/skpdiff/SkDiffContext.cpp:101: }
Make sense, I'll consult with Mike about this. 

On 2014/08/11 19:33:28, epoger wrote:
> On 2014/08/11 14:58:31, stephana wrote:
> > This uses the <string> and <algorithm> headers included above. I didn't see
a
> > lot of use of the C++ standard library in the Skia source - is that
something
> to
> > be avoided ? 
> 
> A fine question... in my experience, within the Skia code we have always
favored
> our homebrew library (SkString and such) rather than bringing in the C++
> standard library.
> 
> In a tool like skpdiff (as opposed to the core API), it probably doesn't
matter
> either way... but I guess for the core API we don't want to depend on the C++
> std library.
> 
> I recommend you ask Mike Reed for guidance on this.
> 
> In this particular case, it seems like SkString should offer a replace()
method.
>  Searching through our code (git grep replace | grep cpp), I find several
> implementations:
> 
> src/utils/SkRTConf.cpp : static inline void str_replace(char *s, char search,
> char replace)
> tools/picture_utils.cpp : void replace_char(SkString* str, const char oldChar,
> const char newChar)
> tools/skimage_main.cpp : static void replace_char(SkString* str, const char
> oldChar, const char newChar)

[epoger, 2014-08-12 18:03:40]

https://codereview.chromium.org/457203003/diff/20001/gm/rebaseline_server/ima...
File gm/rebaseline_server/imagediffdb.py (right):

https://codereview.chromium.org/457203003/diff/20001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb.py:193: # TODO: stub method to be filled in
Do you intend to fill in this method before committing the CL?

[stephana, 2014-08-12 19:25:13]

https://codereview.chromium.org/457203003/diff/20001/gm/rebaseline_server/ima...
File gm/rebaseline_server/imagediffdb.py (right):

https://codereview.chromium.org/457203003/diff/20001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb.py:193: # TODO: stub method to be filled in
No. That's an artifact. Will remove it. 

On 2014/08/12 18:03:40, epoger wrote:
> Do you intend to fill in this method before committing the CL?

[epoger, 2014-08-12 19:52:24]

https://codereview.chromium.org/457203003/diff/40001/gm/rebaseline_server/sta...
File gm/rebaseline_server/static/live-loader.js (right):

https://codereview.chromium.org/457203003/diff/40001/gm/rebaseline_server/sta...
gm/rebaseline_server/static/live-loader.js:669: // if we are already sorting by
this column then toggle between asc/desc
I guess you added this to address http://skbug.com/2818 ('rebaseline_server:
header broken in UI for SKP viewing but GM viewing')?

Ideally, you would commit that within a separate CL, but since you've done it
here I think you should stick with it this time.

https://codereview.chromium.org/457203003/diff/40001/tools/skpdiff/SkDiffCont...
File tools/skpdiff/SkDiffContext.cpp (right):

https://codereview.chromium.org/457203003/diff/40001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.cpp:21: #include <string>
Please add a TODO(stephana) comment here, mentioning that you are going to add
SkString::replaceChar() and get rid of these.

https://codereview.chromium.org/457203003/diff/40001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.cpp:98: result.append("-vs-");
Please add a comment here warning about keeping these things in sync:

We must keep this function in sync with getImageDiffRelativeUrl() in
static/loader.js

https://codereview.chromium.org/457203003/diff/40001/tools/skpdiff/SkDiffCont...
File tools/skpdiff/SkDiffContext.h (right):

https://codereview.chromium.org/457203003/diff/40001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.h:55: * Sets a flag wether to generate file names
that a combination
I find the description confusing... maybe something more explicit, like this?


Modify the pattern used to generate commonName.

For example, for:
  baselinePath=/tmp/dir/image-before.png
  testPath=/tmp/dir/image-after.png

If setLongNames(true), commonName would be:
  image-before-png-vs-image-after-png.png

If setLongNames(false), commonName would be:
  image-.png

https://codereview.chromium.org/457203003/diff/40001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.h:97: *    "commonName"     : string containing the
common prefix of the baselinePath
Please update this docstring to indicate different settings of commonName based
on useLongNames boolean.

(If you describe the explicit examples above as I suggest, this doc can just
point at that one)

[stephana, 2014-08-12 21:36:49]

https://codereview.chromium.org/457203003/diff/40001/gm/rebaseline_server/sta...
File gm/rebaseline_server/static/live-loader.js (right):

https://codereview.chromium.org/457203003/diff/40001/gm/rebaseline_server/sta...
gm/rebaseline_server/static/live-loader.js:669: // if we are already sorting by
this column then toggle between asc/desc
Agreed, I should not have mixed that in. Lesson learned. 

On 2014/08/12 19:52:23, epoger wrote:
> I guess you added this to address http://skbug.com/2818 ('rebaseline_server:
> header broken in UI for SKP viewing but GM viewing')?
> 
> Ideally, you would commit that within a separate CL, but since you've done it
> here I think you should stick with it this time.

https://codereview.chromium.org/457203003/diff/40001/tools/skpdiff/SkDiffCont...
File tools/skpdiff/SkDiffContext.cpp (right):

https://codereview.chromium.org/457203003/diff/40001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.cpp:21: #include <string>
On 2014/08/12 19:52:23, epoger wrote:
> Please add a TODO(stephana) comment here, mentioning that you are going to add
> SkString::replaceChar() and get rid of these.

Acknowledged.

https://codereview.chromium.org/457203003/diff/40001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.cpp:98: result.append("-vs-");
On 2014/08/12 19:52:24, epoger wrote:
> Please add a comment here warning about keeping these things in sync:
> 
> We must keep this function in sync with getImageDiffRelativeUrl() in
> static/loader.js

Acknowledged.

https://codereview.chromium.org/457203003/diff/40001/tools/skpdiff/SkDiffCont...
File tools/skpdiff/SkDiffContext.h (right):

https://codereview.chromium.org/457203003/diff/40001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.h:55: * Sets a flag wether to generate file names
that a combination
Ok. Will do. 

On 2014/08/12 19:52:24, epoger wrote:
> I find the description confusing... maybe something more explicit, like this?
> 
> 
> Modify the pattern used to generate commonName.
> 
> For example, for:
>   baselinePath=/tmp/dir/image-before.png
>   testPath=/tmp/dir/image-after.png
> 
> If setLongNames(true), commonName would be:
>   image-before-png-vs-image-after-png.png
> 
> If setLongNames(false), commonName would be:
>   image-.png
>

https://codereview.chromium.org/457203003/diff/40001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.h:97: *    "commonName"     : string containing the
common prefix of the baselinePath
On 2014/08/12 19:52:24, epoger wrote:
> Please update this docstring to indicate different settings of commonName
based
> on useLongNames boolean.
> 
> (If you describe the explicit examples above as I suggest, this doc can just
> point at that one)

Acknowledged.

[epoger, 2014-08-12 21:39:49]

lgtm

[stephana, 2014-08-12 21:41:06]

The CQ bit was checked by stephana@google.com

[commit-bot: I haz the power, 2014-08-12 21:42:21]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/stephana@google.com/457203003/80001

[commit-bot: I haz the power, 2014-08-12 21:50:28]

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

[commit-bot: I haz the power, 2014-08-12 21:56:38]

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

[commit-bot: I haz the power, 2014-08-12 21:56:42]

Try jobs failed on following builders:
  Build-Win-VS2013-x86-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Win-VS2013-x86-Debug-Trybot/builds...)

[tfarina, 2014-08-13 17:24:03]

https://codereview.chromium.org/457203003/diff/40001/tools/skpdiff/SkDiffCont...
File tools/skpdiff/SkDiffContext.cpp (right):

https://codereview.chromium.org/457203003/diff/40001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.cpp:21: #include <string>
On 2014/08/12 19:52:23, epoger wrote:
> Please add a TODO(stephana) comment here, mentioning that you are going to add
> SkString::replaceChar() and get rid of these.

I think this is -> https://codereview.chromium.org/322253005

Which I hopefully to take from rs.prinja and move it forward and make it
landable.

[epoger, 2014-08-13 17:26:25]

LGTM at patchset 6

[stephana, 2014-08-13 17:27:45]

The CQ bit was checked by stephana@google.com

[commit-bot: I haz the power, 2014-08-13 17:28:16]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/stephana@google.com/457203003/100001

[commit-bot: I haz the power, 2014-08-13 17:36:13]

Committed patchset #6 (100001) as 21b342d19c71fa5abe7e4aa5cfb518fd04cb9d67