Adding additional optional parameters to imagediffdb to make calling from Cluster Telemetry possibl…

Adding additional optional parameters to imagediffdb to make calling from Cluster Telemetry possible.

R=epoger@google.com

Committed: https://code.google.com/p/skia/source/detail?r=12816

[epoger, 2013-12-20 19:46:37]

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

https://codereview.chromium.org/117823008/diff/50001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb.py:3: """
Have you confirmed that its self-test still passes?  (That self-test is not run
automatically yet, so we need to check it manually)

https://codereview.chromium.org/117823008/diff/50001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb.py:61: images_subdir: the subdirectory images
are stored in. Optional parameter,
what are "images" as opposed to "actual images"?  perhaps expected_images_subdir
?

Tell me if I have this right: in my existing code, the assumption is that it's
fine to store both expected and actual images in the same dir, because they are
named using checksums and thus we don't have to worry about name collisions. 
But you are using filenames that do NOT include checksums, so you expect to have
two different images with the same filename (and thus must store them in
different dirs).  Right?

https://codereview.chromium.org/117823008/diff/50001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb.py:62: the default value is in
DEFAULT_IMAGES_SUBDIR.
Thank you for being descriptive, but I think we're actually better off *without*
the "Optional parameter, ..." bit throughout.  That is clearly expressed in the
method signature; let's not duplicate that documentation.

https://codereview.chromium.org/117823008/diff/50001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb.py:67: differences using the
expected_image_locator if expected_image_locator
Confusing description... so, if this is False, but expected_image_locator !=
actual_image_locator, what then?

Proposal: eliminate this new parameter, and always name the differences
'expected_image_locator-vs-actual_image_locator'.  OK, so the filename will be
repetitive ('youtube-vs-youtube.png'), but that seems like less of a worry than
varying rules on filename construction.

https://codereview.chromium.org/117823008/diff/50001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb.py:76: str(expected_image_locator) +
image_suffix),
Warning: if you are not using checksums to generate the image locators,
download_and_open_image() may say "oh, you already have 'youtube.png' in this
directory, I don't need to download it" and then you'll get old images that will
confuse the heck out of you.

Workarounds:
1. use checksums within your image locators (so a different bitmap always means
a different locator)
2. clear out the directory between runs  (downside: you will have to download
every image every time, you won't benefit from cacheing)

[rmistry, 2013-12-20 20:00:04]

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

https://codereview.chromium.org/117823008/diff/50001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb.py:3: """
On 2013/12/20 19:46:37, epoger wrote:
> Have you confirmed that its self-test still passes?  (That self-test is not
run
> automatically yet, so we need to check it manually)

Yes I ran it before and after my changes.

https://codereview.chromium.org/117823008/diff/50001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb.py:61: images_subdir: the subdirectory images
are stored in. Optional parameter,
On 2013/12/20 19:46:37, epoger wrote:
> what are "images" as opposed to "actual images"?  perhaps
expected_images_subdir
> ?

Ah right. Done.

> 
> Tell me if I have this right: in my existing code, the assumption is that it's
> fine to store both expected and actual images in the same dir, because they
are
> named using checksums and thus we don't have to worry about name collisions. 
> But you are using filenames that do NOT include checksums, so you expect to
have
> two different images with the same filename (and thus must store them in
> different dirs).  Right?

Exactly.

https://codereview.chromium.org/117823008/diff/50001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb.py:62: the default value is in
DEFAULT_IMAGES_SUBDIR.
On 2013/12/20 19:46:37, epoger wrote:
> Thank you for being descriptive, but I think we're actually better off
*without*
> the "Optional parameter, ..." bit throughout.  That is clearly expressed in
the
> method signature; let's not duplicate that documentation.

Done.

https://codereview.chromium.org/117823008/diff/50001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb.py:67: differences using the
expected_image_locator if expected_image_locator
On 2013/12/20 19:46:37, epoger wrote:
> Confusing description... so, if this is False, but expected_image_locator !=
> actual_image_locator, what then?
> 
> Proposal: eliminate this new parameter, and always name the differences
> 'expected_image_locator-vs-actual_image_locator'.  OK, so the filename will be
> repetitive ('youtube-vs-youtube.png'), but that seems like less of a worry
than
> varying rules on filename construction.

Agreed. Done.

https://codereview.chromium.org/117823008/diff/50001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb.py:76: str(expected_image_locator) +
image_suffix),
On 2013/12/20 19:46:37, epoger wrote:
> Warning: if you are not using checksums to generate the image locators,
> download_and_open_image() may say "oh, you already have 'youtube.png' in this
> directory, I don't need to download it" and then you'll get old images that
will
> confuse the heck out of you.

I tested this through the python interpreter and it seemed to work ok. But I
will keep this in mind incase I see strange behavior, in which case I can clear
out the directory between runs.

Also about the downloading part: all my files are available locally, when I call
this util I will actually pass in 'None' urls for download to verify that it
does not try to go get something.

> 
> Workarounds:
> 1. use checksums within your image locators (so a different bitmap always
means
> a different locator)
> 2. clear out the directory between runs  (downside: you will have to download
> every image every time, you won't benefit from cacheing)

[epoger, 2013-12-20 20:05:08]

lgtm

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

https://codereview.chromium.org/117823008/diff/50001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb.py:76: str(expected_image_locator) +
image_suffix),
On 2013/12/20 20:00:04, rmistry wrote:
> Also about the downloading part: all my files are available locally, when I
call
> this util I will actually pass in 'None' urls for download to verify that it
> does not try to go get something.

Sounds like a good idea.  More than anything, I wanted to make sure you knew
about this possibility.

You might actually consider adding a parameter to the object, telling it whether
to download images or to assume that they will already be available on local
disk (and raise an Exception otherwise).  As a TODO.

[rmistry, 2013-12-20 20:53:27]

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

https://codereview.chromium.org/117823008/diff/50001/gm/rebaseline_server/ima...
gm/rebaseline_server/imagediffdb.py:76: str(expected_image_locator) +
image_suffix),
On 2013/12/20 20:05:08, epoger wrote:
> On 2013/12/20 20:00:04, rmistry wrote:
> > Also about the downloading part: all my files are available locally, when I
> call
> > this util I will actually pass in 'None' urls for download to verify that it
> > does not try to go get something.
> 
> Sounds like a good idea.  More than anything, I wanted to make sure you knew
> about this possibility.
> 
> You might actually consider adding a parameter to the object, telling it
whether
> to download images or to assume that they will already be available on local
> disk (and raise an Exception otherwise).  As a TODO.

Added a TODO.

[epoger, 2013-12-20 20:56:30]

lgtm

[rmistry, 2013-12-21 19:07:44]

Committed patchset #5 manually as r12816 (presubmit successful).