Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
20527002:
process gm expectation violations (Closed)
|
Created:
7 years, 5 months ago by Zach Reizner Modified:
7 years, 4 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
Descriptionprocess gm expectation violations
R=epoger@google.com
Committed: https://code.google.com/p/skia/source/detail?r=10395
Patch Set 1 #
Total comments: 16
Patch Set 2 : #
Total comments: 3
Patch Set 3 : indent #Messages
Total messages: 7 (0 generated)
to try this out, invoke this within trunk: $ make tools $ tools/rebaseline.py --expectations-filename-output="updated-results.json" $ tools/skpdiff/skpdiff_server.py
Looks good for the most part! Copious comments below. https://codereview.chromium.org/20527002/diff/1/tools/skpdiff/skpdiff_server.py File tools/skpdiff/skpdiff_server.py (right): https://codereview.chromium.org/20527002/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:1: #!/usr/bin/python Just to make sure I understand: the core change in this CL is: skpdiff_server picks up much of the functionality of svndiff: it can read in expected-results.json and actual-results.json files, find tests for which expectations don't match actuals, and download all those expected and actual images from Google Storage for display by the browser. So, is the idea that a future CL will add the ability to receive commands from the web viewer and copy checksum values from actual-results.json to expected-results.json at the user's discretion? https://codereview.chromium.org/20527002/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:2: # -*- coding: utf-8 -*- Be sure to mark this file as executable, if you didn't already. If you don't, this will be the result: $ tools/skpdiff/skpdiff_server.py bash: tools/skpdiff/skpdiff_server.py: Permission denied $ python tools/skpdiff/skpdiff_server.py script dir : /usr/local/google/home/epoger/src/skia/red/trunk/tools/skpdiff tools dir : /usr/local/google/home/epoger/src/skia/red/trunk/tools root dir : /usr/local/google/home/epoger/src/skia/red/trunk expectations dir : /usr/local/google/home/epoger/src/skia/red/trunk/expectations/gm skpdiff path : /usr/local/google/home/epoger/src/skia/red/trunk/out/Debug/skpdiff tmp dir : /tmp/tmpfOkU2Dskpdiff expected image dir : /tmp/tmpfOkU2Dskpdiff/expected actual image dir : /tmp/tmpfOkU2Dskpdiff/actual Non-default runtime configuration options: Navigate thine browser to: 127.0.0.1:8080 https://codereview.chromium.org/20527002/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:3: Here's an alternate set of steps to test this code, so you actually get some diffs to view: $ make tools $ rm expectations/gm/base-*/updated-results.json $ for PLATFORM in expectations/gm/base-* ; do if [ -e $PLATFORM/updated-results.json ] ; then diff $PLATFORM/expected-results.json $PLATFORM/updated-results.json; fi; done $ tools/rebaseline.py --expectations-filename-output updated-results.json --actuals-base-url https://skia-autogen.googlecode.com/svn-history/r2400/gm-actual $ for PLATFORM in expectations/gm/base-* ; do if [ -e $PLATFORM/updated-results.json ] ; then diff $PLATFORM/expected-results.json $PLATFORM/updated-results.json; fi; done 2623c2623 < 15666965705698152905 --- > 17054431316752729196 2632c2632 < 297568835278809227 --- > 7292150392805847499 [...] 2560c2560 < 11156208867963674446 --- > 16358520639652320103 940c940 < 304572795717583895 --- > 16700250845218992862 $ python tools/skpdiff/skpdiff_server.py https://codereview.chromium.org/20527002/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:4: from __future__ import print_function I guess this is more of a viewer.html issue, but when I tried this out myself I was confused by the UI. When I hovered over an image, I saw dotted lines around it but I didn't see any changes. When I clicked on an image (in either expected or actual column), I saw it change to match the OTHER one... I wasn't clear on what that meant. https://codereview.chromium.org/20527002/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:95: old_hash, new_hash): I think it would be clearer if you made this function download exactly one image (pass in image_name and hash, write the downloaded file to image_path) https://codereview.chromium.org/20527002/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:105: # Seperate the image name into a GM name and syffix More like... # Extract the test name (e.g. "imageblur") from the image name (e.g. # "imageblur_gpu.png") https://codereview.chromium.org/20527002/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:121: def download_actuals(expectations_dir, expected_name, updated_name, It's downloading both expected and actual images, so I think this function should be called something else (maybe download_all_images ?) https://codereview.chromium.org/20527002/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:234: # Handle skpdiff_output.json manually because it is loaded manually into Is it really necessary to handle this specially? Seems like it would be simpler to just handle it like any other file. Maybe you do so because the web page requires the skpdiff_output.json file to be JSONP-formatted (i.e. JSON with padding), as mentioned below? If so, maybe handle it specially, but change the comment: # Handle skpdiff_output.json differently, because the web viewer requires it to # be padded (JSONP format), and skpdiff's output is not padded. P.S. *Why* does the web viewer require this? https://codereview.chromium.org/20527002/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:279: print('Navigate thine browser to: {}:{}'.format(*server_address)) maybe change this line as follows, so the user can just click on the link: print('Navigate thine browser to: http://{}:{}'.format(*server_address))
https://codereview.chromium.org/20527002/diff/1/tools/skpdiff/skpdiff_server.py File tools/skpdiff/skpdiff_server.py (right): https://codereview.chromium.org/20527002/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:1: #!/usr/bin/python On 2013/07/26 16:15:57, epoger wrote: > Just to make sure I understand: the core change in this CL is: > > skpdiff_server picks up much of the functionality of svndiff: it can read in > expected-results.json and actual-results.json files, find tests for which > expectations don't match actuals, and download all those expected and actual > images from Google Storage for display by the browser. > > So, is the idea that a future CL will add the ability to receive commands from > the web viewer and copy checksum values from actual-results.json to > expected-results.json at the user's discretion? Yep https://codereview.chromium.org/20527002/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:2: # -*- coding: utf-8 -*- On 2013/07/26 16:15:57, epoger wrote: > Be sure to mark this file as executable, if you didn't already. If you don't, > this will be the result: > > $ tools/skpdiff/skpdiff_server.py > bash: tools/skpdiff/skpdiff_server.py: Permission denied > > $ python tools/skpdiff/skpdiff_server.py > script dir : > /usr/local/google/home/epoger/src/skia/red/trunk/tools/skpdiff > tools dir : /usr/local/google/home/epoger/src/skia/red/trunk/tools > root dir : /usr/local/google/home/epoger/src/skia/red/trunk > expectations dir : > /usr/local/google/home/epoger/src/skia/red/trunk/expectations/gm > skpdiff path : > /usr/local/google/home/epoger/src/skia/red/trunk/out/Debug/skpdiff > tmp dir : /tmp/tmpfOkU2Dskpdiff > expected image dir : /tmp/tmpfOkU2Dskpdiff/expected > actual image dir : /tmp/tmpfOkU2Dskpdiff/actual > Non-default runtime configuration options: > Navigate thine browser to: 127.0.0.1:8080 Done. https://codereview.chromium.org/20527002/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:4: from __future__ import print_function On 2013/07/26 16:15:57, epoger wrote: > I guess this is more of a viewer.html issue, but when I tried this out myself I > was confused by the UI. When I hovered over an image, I saw dotted lines around > it but I didn't see any changes. When I clicked on an image (in either expected > or actual column), I saw it change to match the OTHER one... I wasn't clear on > what that meant. Weird. Clicking is not supposed to do anything. Hovering over is supposed to do the switch of the image you're hovering over. Which browser version are you on? https://codereview.chromium.org/20527002/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:95: old_hash, new_hash): On 2013/07/26 16:15:57, epoger wrote: > I think it would be clearer if you made this function download exactly one image > (pass in image_name and hash, write the downloaded file to image_path) Done. https://codereview.chromium.org/20527002/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:105: # Seperate the image name into a GM name and syffix On 2013/07/26 16:15:57, epoger wrote: > More like... > > # Extract the test name (e.g. "imageblur") from the image name (e.g. > # "imageblur_gpu.png") Done. https://codereview.chromium.org/20527002/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:121: def download_actuals(expectations_dir, expected_name, updated_name, On 2013/07/26 16:15:57, epoger wrote: > It's downloading both expected and actual images, so I think this function > should be called something else (maybe download_all_images ?) Done.
LGTM if you will correct a couple of small things... https://codereview.chromium.org/20527002/diff/1/tools/skpdiff/skpdiff_server.py File tools/skpdiff/skpdiff_server.py (right): https://codereview.chromium.org/20527002/diff/1/tools/skpdiff/skpdiff_server.... tools/skpdiff/skpdiff_server.py:121: def download_actuals(expectations_dir, expected_name, updated_name, On 2013/07/26 18:00:40, Zach Reizner wrote: > On 2013/07/26 16:15:57, epoger wrote: > > It's downloading both expected and actual images, so I think this function > > should be called something else (maybe download_all_images ?) > > Done. Ah, I get the "updated" part of the name (it's a bit confusing, because the function downloads images referred to from both expected_name and updated_name). Maybe rename this download_changed_images, so we have distinct meanings for "updated" and "changed"? https://codereview.chromium.org/20527002/diff/5001/tools/skpdiff/skpdiff_serv... File tools/skpdiff/skpdiff_server.py (right): https://codereview.chromium.org/20527002/diff/5001/tools/skpdiff/skpdiff_serv... tools/skpdiff/skpdiff_server.py:95: """Download the expected and actual results into the given paths. please correct this description (it's no longer accurate) https://codereview.chromium.org/20527002/diff/5001/tools/skpdiff/skpdiff_serv... tools/skpdiff/skpdiff_server.py:172: expected_image_prefix + image_name, a really big deal: please modify the indent on these lines, now that the function name got shorter https://codereview.chromium.org/20527002/diff/5001/tools/skpdiff/skpdiff_serv... tools/skpdiff/skpdiff_server.py:344: expected_image_dir, actual_image_dir) please update indent level on this line
lgtm
Message was sent while issue was closed.
Committed patchset #3 manually as r10395 (presubmit successful). |
