Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24274003:
Create HTTP-based GM results viewer. (Closed)
|
Created:
7 years, 3 months ago by epoger Modified:
7 years, 2 months ago CC:
skia-review_googlegroups.com, rmistry, jcgregorio, benchen Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionCreate HTTP-based GM results viewer.
For now, it only allows VIEWING results... next, it will allow the user to
rebaseline GM results via the web interface.
R=borenet@google.com
Committed: https://code.google.com/p/skia/source/detail?r=11500
Patch Set 1 #
Total comments: 1
Patch Set 2 : cleanup_and_line_wraps #Patch Set 3 : svn_py #
Total comments: 27
Patch Set 4 : comments #Patch Set 5 : rework #
Total comments: 18
Patch Set 6 : small_fries #Patch Set 7 : linewrap #
Messages
Total messages: 13 (0 generated)
Ready for review at patchset 3. The server is running at http://c128.i.corp.google.com:8888/ , if you want to try it out. https://codereview.chromium.org/24274003/diff/1/gm/rebaseline_server/server.py File gm/rebaseline_server/server.py (right): https://codereview.chromium.org/24274003/diff/1/gm/rebaseline_server/server.p... gm/rebaseline_server/server.py:36: MIME_TYPE_MAP = {'': 'application/octet-stream', The MIME_TYPE_MAP and other chunks of this code were adapted from https://code.google.com/p/skia/source/browse/trunk/tools/skpdiff/skpdiff_serv... . The plan is for this new server to provide the functionality of all these current tools: - rebaseline.py - svndiff.py - skpdiff And hopefully we can delete those older tools once this one is complete. https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/svn.py File gm/rebaseline_server/svn.py (right): https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/svn.p... gm/rebaseline_server/svn.py:2: Copyright 2011 Google Inc. This file is "svn cp'd" from ../../tools . Should I instead import it directly from the tools dir? Right now, we have some ugly interdependencies between tools/ and gm/ .
https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/serve... File gm/rebaseline_server/server.py (right): https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/serve... gm/rebaseline_server/server.py:88: if self._export: Given that this tool will allow non-readonly access in the future, allowing access from an non-localhost address is not very safe, so print a warning on stdout if external is true, also add some text to the web page served in a big red font if external is true.
https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/resul... File gm/rebaseline_server/results.py (right): https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/resul... gm/rebaseline_server/results.py:51: a meta-dictionary (keyed by the builder type for each dictionary). nit: "builder name" https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/resul... gm/rebaseline_server/results.py:67: def OfType(self, result_type): Is this the only method which gets called on Results? If so, can we just index the results by result_type initially, so that this function (which is called many times) can do much less work? https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/resul... gm/rebaseline_server/results.py:89: # gm_json.JSONKEY_ACTUALRESULTS_* strings? If not, please change line 93 to use .get() instead of []. https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/serve... File gm/rebaseline_server/server.py (right): https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/serve... gm/rebaseline_server/server.py:72: Results instances, a new one for each request? Per-request sounds good, but I think it might be too slow. https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/serve... gm/rebaseline_server/server.py:84: shutil.rmtree(actuals_root) Why put this in a temp dir? Won't it be faster if it stays around? https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/svn.py File gm/rebaseline_server/svn.py (right): https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/svn.p... gm/rebaseline_server/svn.py:2: Copyright 2011 Google Inc. On 2013/09/24 16:46:40, epoger wrote: > This file is "svn cp'd" from ../../tools . > > Should I instead import it directly from the tools dir? Right now, we have some > ugly interdependencies between tools/ and gm/ . Personally, I'd prefer that these get fixed sooner rather than later, but to unblock yourself this is okay for now. Maybe file a cleanup bug?
https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/stati... File gm/rebaseline_server/static/view.html (right): https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/stati... gm/rebaseline_server/static/view.html:11: <body> For some reason, I'm really thrown off by this webpage's not being centered. Leaving that decision up to you. https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/stati... gm/rebaseline_server/static/view.html:38: <th ng:click="sortColumn='actualHashDigest'">Actual Image</th> I don't know how useful they were to other devs, but I find myself missing the diff images. I'm aware that we aren't generating them any more though.
Thanks Eric and Joe for your comments. Please see patchset 4. https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/resul... File gm/rebaseline_server/results.py (right): https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/resul... gm/rebaseline_server/results.py:51: a meta-dictionary (keyed by the builder type for each dictionary). On 2013/09/25 15:08:08, borenet wrote: > nit: "builder name" Done. https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/resul... gm/rebaseline_server/results.py:67: def OfType(self, result_type): On 2013/09/25 15:08:08, borenet wrote: > Is this the only method which gets called on Results? If so, can we just index > the results by result_type initially, so that this function (which is called > many times) can do much less work? That's a good point. Actually, it opens the wider discussion that I need to have about this: I think we should make the server return ALL result types (or at least all types except for success), because the user may want to jump around within those different result types during the rebaselining process. In other words... I think it probably makes the most sense for the client side to make a single query--"give me all results that might need rebaselining: failed, failure_ignored, and missing_expectations"--and then filter that view within the client. What do you think? https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/resul... gm/rebaseline_server/results.py:89: # gm_json.JSONKEY_ACTUALRESULTS_* strings? On 2013/09/25 15:08:08, borenet wrote: > If not, please change line 93 to use .get() instead of []. Done. https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/serve... File gm/rebaseline_server/server.py (right): https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/serve... gm/rebaseline_server/server.py:72: Results instances, a new one for each request? On 2013/09/25 15:08:08, borenet wrote: > Per-request sounds good, but I think it might be too slow. One thing that makes this a bit tricky is that I imagine this server performing multiple roles: 1. Run for a short time by a developer, to rebaseline tests. In this case, you don't want to sync expectations automatically (let the developer be in charge of his own trunk checkout), and it's not important to update the actuals because the server only runs for a few minutes. 2. Run for a long time, at a public server, for easy perusal of current GM failures. In this case, you want to sync expectations and actuals every now and then so they stay up to date. And you DON'T want to allow anyone to rebaseline results. As we push this tool forward, I will be pestering people for live discussion of this issue. https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/serve... gm/rebaseline_server/server.py:84: shutil.rmtree(actuals_root) On 2013/09/25 15:08:08, borenet wrote: > Why put this in a temp dir? Won't it be faster if it stays around? See my discussion above, of the different uses for this server. I guess I could handle both cases (long-lived and short-lived) by using a directory within current working dir (maybe ".gm-actuals" so it's hidden?), and either checking out or updating into it depending on whether the dir exists yet. What do you think of that idea? https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/serve... gm/rebaseline_server/server.py:88: if self._export: On 2013/09/24 20:19:35, jcgregorio wrote: > Given that this tool will allow non-readonly access in the future, allowing > access from an non-localhost address is not very safe, so print a warning on > stdout if external is true, also add some text to the web page served in a big > red font if external is true. I added the warning to stdout (and within the --help). I don't know off the top of my head how view.html can know whether --export is turned on, so for now I put that as a TODO. https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/stati... File gm/rebaseline_server/static/view.html (right): https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/stati... gm/rebaseline_server/static/view.html:11: <body> On 2013/09/25 15:09:51, borenet wrote: > For some reason, I'm really thrown off by this webpage's not being centered. > Leaving that decision up to you. I tried to make some adjustments, but my CSS-fu is weak. I would rather leave these stylistic adjustments for a future CL, if that's OK with you. https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/stati... gm/rebaseline_server/static/view.html:38: <th ng:click="sortColumn='actualHashDigest'">Actual Image</th> On 2013/09/25 15:09:51, borenet wrote: > I don't know how useful they were to other devs, but I find myself missing the > diff images. I'm aware that we aren't generating them any more though. Oh, absolutely. I added a TODO in lamenting this missing column. I can think of two ways of generating these pixel diffs: 1. Make results.py download the images themselves, calculate pixel diffs, and provide them to the client (perhaps as inline image data, within the JSON file?) 2. Make the client download the images into Canvas elements, and then calculate the pixel diffs as the user scrolls to them. This has several advantages (including making it possible to do nifty effects like a loupe), but due to security constraints requires the images to be loaded from the same domain as the JSON results themselves. My intent is to add a new dispatcher to server.py : if the client requests /images/gm/bitmap-64bitMD5/imageblur/5680959140053860741.png , fetch that file from under http://chromium-skia-gm.commondatastorage.googleapis.com/ and return it to the client. That will allow us to work around the security constraint, and the client can perform Canvas operations on the fetched image data. Thoughts? https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/svn.py File gm/rebaseline_server/svn.py (right): https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/svn.p... gm/rebaseline_server/svn.py:2: Copyright 2011 Google Inc. On 2013/09/25 15:08:08, borenet wrote: > On 2013/09/24 16:46:40, epoger wrote: > > This file is "svn cp'd" from ../../tools . > > > > Should I instead import it directly from the tools dir? Right now, we have > some > > ugly interdependencies between tools/ and gm/ . > > Personally, I'd prefer that these get fixed sooner rather than later, but to > unblock yourself this is okay for now. Maybe file a cleanup bug? I made this depend on tools/svn.py. We already had interdependencies between tools/ and gm/ , so at least things aren't any WORSE. If/when we want to untangle that interdependency, we can tackle it.
Notes from live discussion with Brian... https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/resul... File gm/rebaseline_server/results.py (right): https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/resul... gm/rebaseline_server/results.py:67: def OfType(self, result_type): On 2013/09/25 16:21:18, epoger wrote: > In other words... I think it probably makes the most sense for the client side > to make a single query--"give me all results that might need rebaselining: > failed, failure_ignored, and missing_expectations"--and then filter that view > within the client. Brian and I agree that this is the way to go (in a future CL, if not now): The client will make a single request to get results of ALL types--even success, if it doesn't take ridiculously long to load them all--and then the view will allow the user to filter, sort, etc. By default, the filters will be set to show only failing tests. https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/serve... File gm/rebaseline_server/server.py (right): https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/serve... gm/rebaseline_server/server.py:72: Results instances, a new one for each request? On 2013/09/25 16:21:18, epoger wrote: > On 2013/09/25 15:08:08, borenet wrote: > > Per-request sounds good, but I think it might be too slow. > > One thing that makes this a bit tricky is that I imagine this server performing > multiple roles: > > 1. Run for a short time by a developer, to rebaseline tests. > In this case, you don't want to sync expectations automatically (let the > developer be in charge of his own trunk checkout), and it's not important to > update the actuals because the server only runs for a few minutes. > > 2. Run for a long time, at a public server, for easy perusal of current GM > failures. > In this case, you want to sync expectations and actuals every now and then so > they stay up to date. And you DON'T want to allow anyone to rebaseline results. > > As we push this tool forward, I will be pestering people for live discussion of > this issue. Brian and I came up with an idea for a mode switch on the command line: --browseonly If it's set, then we are in mode #2 (browse only): - do not offer the user an interface to modify expectations (it's read-only) - automatically update both expectations and actuals frequently (modifying the page while the user is looking at it) Otherwise, we are in mode #1 (rebaselining): - allow the user to modify expectations - expectations and actuals are viewed as "a snapshot in time from when the server was launched"--do not update them after that. If the user wants to update them, he must stop and restart the server. https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/stati... File gm/rebaseline_server/static/view.html (right): https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/stati... gm/rebaseline_server/static/view.html:38: <th ng:click="sortColumn='actualHashDigest'">Actual Image</th> On 2013/09/25 16:21:18, epoger wrote: > On 2013/09/25 15:09:51, borenet wrote: > > I don't know how useful they were to other devs, but I find myself missing the > > diff images. I'm aware that we aren't generating them any more though. > > Oh, absolutely. I added a TODO in lamenting this missing column. > > I can think of two ways of generating these pixel diffs: > 1. Make results.py download the images themselves, calculate pixel diffs, and > provide them to the client (perhaps as inline image data, within the JSON file?) > 2. Make the client download the images into Canvas elements, and then calculate > the pixel diffs as the user scrolls to them. This has several advantages > (including making it possible to do nifty effects like a loupe), but due to > security constraints requires the images to be loaded from the same domain as > the JSON results themselves. > > My intent is to add a new dispatcher to server.py : if the client requests > /images/gm/bitmap-64bitMD5/imageblur/5680959140053860741.png , fetch that file > from under http://chromium-skia-gm.commondatastorage.googleapis.com/ and return > it to the client. That will allow us to work around the security constraint, > and the client can perform Canvas operations on the fetched image data. Brian has changed my mind about this. I now think we are better off calculating pixel diffs on the server, mainly for reasons of client load/performance... if the viewer is using a phone browser, the phone might melt trying to calculate pixel diffs. We can perform them on the server side; if we see server load problems because multiple users are using the service concurrently, we should be able to amortize the work over multiple users (all users will see the same diffs, after all). In terms of how the client loads those pixel diff images... I guess the server's initial JSON response can include checksums of the diffs (similar to how it provides checksums of the expected/actual images themselves), and the client can construct an image source URL using the checksum. P.S. This does not preclude the option of loading images into a Canvas on the client later on... it may be appropriate to do that in some cases. Later. :-)
On 2013/09/25 18:26:33, epoger wrote: > Notes from live discussion with Brian... > > https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/resul... > File gm/rebaseline_server/results.py (right): > > https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/resul... > gm/rebaseline_server/results.py:67: def OfType(self, result_type): > On 2013/09/25 16:21:18, epoger wrote: > > In other words... I think it probably makes the most sense for the client side > > to make a single query--"give me all results that might need rebaselining: > > failed, failure_ignored, and missing_expectations"--and then filter that view > > within the client. > > Brian and I agree that this is the way to go (in a future CL, if not now): The > client will make a single request to get results of ALL types--even success, if > it doesn't take ridiculously long to load them all--and then the view will allow > the user to filter, sort, etc. By default, the filters will be set to show only > failing tests. > > https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/serve... > File gm/rebaseline_server/server.py (right): > > https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/serve... > gm/rebaseline_server/server.py:72: Results instances, a new one for each > request? > On 2013/09/25 16:21:18, epoger wrote: > > On 2013/09/25 15:08:08, borenet wrote: > > > Per-request sounds good, but I think it might be too slow. > > > > One thing that makes this a bit tricky is that I imagine this server > performing > > multiple roles: > > > > 1. Run for a short time by a developer, to rebaseline tests. > > In this case, you don't want to sync expectations automatically (let the > > developer be in charge of his own trunk checkout), and it's not important to > > update the actuals because the server only runs for a few minutes. > > > > 2. Run for a long time, at a public server, for easy perusal of current GM > > failures. > > In this case, you want to sync expectations and actuals every now and then so > > they stay up to date. And you DON'T want to allow anyone to rebaseline > results. > > > > As we push this tool forward, I will be pestering people for live discussion > of > > this issue. > > Brian and I came up with an idea for a mode switch on the command line: > --browseonly > > If it's set, then we are in mode #2 (browse only): > - do not offer the user an interface to modify expectations (it's read-only) > - automatically update both expectations and actuals frequently (modifying the > page while the user is looking at it) > > Otherwise, we are in mode #1 (rebaselining): > - allow the user to modify expectations > - expectations and actuals are viewed as "a snapshot in time from when the > server was launched"--do not update them after that. If the user wants to > update them, he must stop and restart the server. Or maybe a "reset and reload" button? > > https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/stati... > File gm/rebaseline_server/static/view.html (right): > > https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/stati... > gm/rebaseline_server/static/view.html:38: <th > ng:click="sortColumn='actualHashDigest'">Actual Image</th> > On 2013/09/25 16:21:18, epoger wrote: > > On 2013/09/25 15:09:51, borenet wrote: > > > I don't know how useful they were to other devs, but I find myself missing > the > > > diff images. I'm aware that we aren't generating them any more though. > > > > Oh, absolutely. I added a TODO in lamenting this missing column. > > > > I can think of two ways of generating these pixel diffs: > > 1. Make results.py download the images themselves, calculate pixel diffs, and > > provide them to the client (perhaps as inline image data, within the JSON > file?) > > 2. Make the client download the images into Canvas elements, and then > calculate > > the pixel diffs as the user scrolls to them. This has several advantages > > (including making it possible to do nifty effects like a loupe), but due to > > security constraints requires the images to be loaded from the same domain as > > the JSON results themselves. > > > > My intent is to add a new dispatcher to server.py : if the client requests > > /images/gm/bitmap-64bitMD5/imageblur/5680959140053860741.png , fetch that file > > from under http://chromium-skia-gm.commondatastorage.googleapis.com/ and > return > > it to the client. That will allow us to work around the security constraint, > > and the client can perform Canvas operations on the fetched image data. > > Brian has changed my mind about this. I now think we are better off calculating > pixel diffs on the server, mainly for reasons of client load/performance... if > the viewer is using a phone browser, the phone might melt trying to calculate > pixel diffs. We can perform them on the server side; if we see server load > problems because multiple users are using the service concurrently, we should be > able to amortize the work over multiple users (all users will see the same > diffs, after all). > > In terms of how the client loads those pixel diff images... I guess the server's > initial JSON response can include checksums of the diffs (similar to how it > provides checksums of the expected/actual images themselves), and the client can > construct an image source URL using the checksum. > > P.S. This does not preclude the option of loading images into a Canvas on the > client later on... it may be appropriate to do that in some cases. Later. :-)
https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/resul... File gm/rebaseline_server/results.py (right): https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/resul... gm/rebaseline_server/results.py:67: def OfType(self, result_type): On 2013/09/25 18:26:33, epoger wrote: > On 2013/09/25 16:21:18, epoger wrote: > > In other words... I think it probably makes the most sense for the client side > > to make a single query--"give me all results that might need rebaselining: > > failed, failure_ignored, and missing_expectations"--and then filter that view > > within the client. > > Brian and I agree that this is the way to go (in a future CL, if not now): The > client will make a single request to get results of ALL types--even success, if > it doesn't take ridiculously long to load them all--and then the view will allow > the user to filter, sort, etc. By default, the filters will be set to show only > failing tests. > I agree. The page will be more interactive that way. https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/serve... File gm/rebaseline_server/server.py (right): https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/serve... gm/rebaseline_server/server.py:72: Results instances, a new one for each request? On 2013/09/25 18:26:33, epoger wrote: > On 2013/09/25 16:21:18, epoger wrote: > > On 2013/09/25 15:08:08, borenet wrote: > > > Per-request sounds good, but I think it might be too slow. > > > > One thing that makes this a bit tricky is that I imagine this server > performing > > multiple roles: > > > > 1. Run for a short time by a developer, to rebaseline tests. > > In this case, you don't want to sync expectations automatically (let the > > developer be in charge of his own trunk checkout), and it's not important to > > update the actuals because the server only runs for a few minutes. > > > > 2. Run for a long time, at a public server, for easy perusal of current GM > > failures. > > In this case, you want to sync expectations and actuals every now and then so > > they stay up to date. And you DON'T want to allow anyone to rebaseline > results. > > > > As we push this tool forward, I will be pestering people for live discussion > of > > this issue. > > Brian and I came up with an idea for a mode switch on the command line: > --browseonly > > If it's set, then we are in mode #2 (browse only): > - do not offer the user an interface to modify expectations (it's read-only) > - automatically update both expectations and actuals frequently (modifying the > page while the user is looking at it) > > Otherwise, we are in mode #1 (rebaselining): > - allow the user to modify expectations > - expectations and actuals are viewed as "a snapshot in time from when the > server was launched"--do not update them after that. If the user wants to > update them, he must stop and restart the server. Personally, I think a long-running server which is capable of doing the rebaselining is the "holy grail" here. I can see a lot of value in not needing to mess with my local machine at all to rebaseline (if, for example, I read a breakage email on my phone, I can go to this page, check some boxes and be done without ever having to touch my workstation). I guess there are a lot of concerns there (security), so these options are good, as long as #1 is stupid-easy. https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/serve... gm/rebaseline_server/server.py:84: shutil.rmtree(actuals_root) On 2013/09/25 16:21:18, epoger wrote: > On 2013/09/25 15:08:08, borenet wrote: > > Why put this in a temp dir? Won't it be faster if it stays around? > > See my discussion above, of the different uses for this server. > > I guess I could handle both cases (long-lived and short-lived) by using a > directory within current working dir (maybe ".gm-actuals" so it's hidden?), and > either checking out or updating into it depending on whether the dir exists yet. > > What do you think of that idea? I think that's fine. An alternative would be to accept an "--actuals-dir" flag, and have the default be a temp directory. https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/stati... File gm/rebaseline_server/static/view.html (right): https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/stati... gm/rebaseline_server/static/view.html:11: <body> On 2013/09/25 16:21:18, epoger wrote: > On 2013/09/25 15:09:51, borenet wrote: > > For some reason, I'm really thrown off by this webpage's not being centered. > > Leaving that decision up to you. > > I tried to make some adjustments, but my CSS-fu is weak. I would rather leave > these stylistic adjustments for a future CL, if that's OK with you. SGTM https://codereview.chromium.org/24274003/diff/5001/gm/rebaseline_server/stati... gm/rebaseline_server/static/view.html:38: <th ng:click="sortColumn='actualHashDigest'">Actual Image</th> On 2013/09/25 18:26:33, epoger wrote: > On 2013/09/25 16:21:18, epoger wrote: > > On 2013/09/25 15:09:51, borenet wrote: > > > I don't know how useful they were to other devs, but I find myself missing > the > > > diff images. I'm aware that we aren't generating them any more though. > > > > Oh, absolutely. I added a TODO in lamenting this missing column. > > > > I can think of two ways of generating these pixel diffs: > > 1. Make results.py download the images themselves, calculate pixel diffs, and > > provide them to the client (perhaps as inline image data, within the JSON > file?) > > 2. Make the client download the images into Canvas elements, and then > calculate > > the pixel diffs as the user scrolls to them. This has several advantages > > (including making it possible to do nifty effects like a loupe), but due to > > security constraints requires the images to be loaded from the same domain as > > the JSON results themselves. > > > > My intent is to add a new dispatcher to server.py : if the client requests > > /images/gm/bitmap-64bitMD5/imageblur/5680959140053860741.png , fetch that file > > from under http://chromium-skia-gm.commondatastorage.googleapis.com/ and > return > > it to the client. That will allow us to work around the security constraint, > > and the client can perform Canvas operations on the fetched image data. > > Brian has changed my mind about this. I now think we are better off calculating > pixel diffs on the server, mainly for reasons of client load/performance... if > the viewer is using a phone browser, the phone might melt trying to calculate > pixel diffs. We can perform them on the server side; if we see server load > problems because multiple users are using the service concurrently, we should be > able to amortize the work over multiple users (all users will see the same > diffs, after all). > > In terms of how the client loads those pixel diff images... I guess the server's > initial JSON response can include checksums of the diffs (similar to how it > provides checksums of the expected/actual images themselves), and the client can > construct an image source URL using the checksum. > > P.S. This does not preclude the option of loading images into a Canvas on the > client later on... it may be appropriate to do that in some cases. Later. :-) Yeah, #2 sounds awesome, particularly if you can do neat things like set thresholds and have the diff regenerate on the fly, but it will likely be slow if the client is slow or if there are lots of images.
OK guys, please see patchset 5. Rather than reply on each of the comment threads individually, I have annotated patchset 5 with highlights of the changes (including changes based on our conversations yesterday). Still available for you to play with at http://c128.i.corp.google.com:8888/ https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/resu... File gm/rebaseline_server/results.py (right): https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/resu... gm/rebaseline_server/results.py:49: def GetAll(self): Replaced the old OfType(result_type) method with GetAll() , that returns results of *all* types and lets the client filter/sort them as desired. And generates that set of results ONCE, during __init__(), rather than waiting until the client requests them. https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/resu... gm/rebaseline_server/results.py:93: for builder in sorted(self._actual_builder_dicts.keys()): The codereview diff makes this seem more different than the prior patchset than it really is. Basically, rather than throwing away results unless they match result_type, we gather them all together. https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/resu... gm/rebaseline_server/results.py:162: # TODO(epoger): For now, don't include succeeded results. Note this temporary restriction to keep the tool usable, until we figure out how to handle the performance issues around the large number of "succeeded" results. https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/serv... File gm/rebaseline_server/server.py (right): https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/serv... gm/rebaseline_server/server.py:97: actuals_repo.Update('.') Now updates the gm-actuals we checked out last time, if they're still around, rather than checking them out from scratch every time. And added --actuals-dir flag, so the user can determine where these files are kept. https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/serv... gm/rebaseline_server/server.py:147: For now, we ignore the remaining path info, because we only know how to This is a little funny... the client now requests /results/all instead of /results/$type . So we have this "vestigial" subpath which is currently unused. https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/stat... File gm/rebaseline_server/static/view.html (right): https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/stat... gm/rebaseline_server/static/view.html:32: TODO(epoger): For now, I have disabled viewing the Another temporary constraint to keep things usable, performance-wise, until we come up with a long-term fix. https://codereview.chromium.org/24274003/diff/28001/tools/svn.py File tools/svn.py (right): https://codereview.chromium.org/24274003/diff/28001/tools/svn.py#newcode88 tools/svn.py:88: def Update(self, path): I had to add Update() functionality to tools/svn.py
https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/resu... File gm/rebaseline_server/results.py (right): https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/resu... gm/rebaseline_server/results.py:49: def GetAll(self): On 2013/09/26 17:26:51, epoger wrote: > Replaced the old OfType(result_type) method with GetAll() , that returns results > of *all* types and lets the client filter/sort them as desired. > > And generates that set of results ONCE, during __init__(), rather than waiting > until the client requests them. If the client is going to filter by result type, would it be smarter to go ahead and index by result type, so that the client's work is really fast? https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/serv... File gm/rebaseline_server/server.py (right): https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/serv... gm/rebaseline_server/server.py:95: actuals_repo.Checkout(ACTUALS_SVN_REPO, '.') Does this just check out into '.'? We haven't chdir'd to self._actuals_dir... https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/serv... gm/rebaseline_server/server.py:97: actuals_repo.Update('.') Same question here as above: do we need to os.chdir(self._actuals_dir) in order to use '.'? https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/serv... gm/rebaseline_server/server.py:147: For now, we ignore the remaining path info, because we only know how to On 2013/09/26 17:26:51, epoger wrote: > This is a little funny... the client now requests /results/all instead of > /results/$type . So we have this "vestigial" subpath which is currently unused. Looks like I could request /results/pleasenoresults and still get "all". Is there a plan to add back result types, or can we go ahead and remove the subpath? https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/serv... gm/rebaseline_server/server.py:206: 'defaults to %(default)s .'), I think adding the period here will be confusing...
Please see small adjustments in patchset 6... https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/resu... File gm/rebaseline_server/results.py (right): https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/resu... gm/rebaseline_server/results.py:49: def GetAll(self): On 2013/09/26 19:02:01, borenet wrote: > On 2013/09/26 17:26:51, epoger wrote: > > Replaced the old OfType(result_type) method with GetAll() , that returns > results > > of *all* types and lets the client filter/sort them as desired. > > > > And generates that set of results ONCE, during __init__(), rather than waiting > > until the client requests them. > > If the client is going to filter by result type, would it be smarter to go ahead > and index by result type, so that the client's work is really fast? The short answer: "Maybe, but I don't think we know yet. I think we should commit it as-is and then make adjustments." The long answer: - Currently, view.html has a dropdown at the very top of the page within which the user chooses exactly one result type to see. But, as noted in a TODO there, that will change very soon; we want to allow more flexibility in this (for example, show a mix of both FAILED and FAILUREIGNORED in the same table). - Each result record includes the resultType, so it should be fairly straightforward for the client view to be filtered with things exactly as they are. - I think we need to add lazy loading of images, so we can see how much of the performance problems of long lists is due to image loading (as opposed to processing the list itself). https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/serv... File gm/rebaseline_server/server.py (right): https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/serv... gm/rebaseline_server/server.py:95: actuals_repo.Checkout(ACTUALS_SVN_REPO, '.') On 2013/09/26 19:02:01, borenet wrote: > Does this just check out into '.'? We haven't chdir'd to self._actuals_dir... The svn.Svn() constructor takes care of this for us. I have just checked the directory where I am running the server to confirm. Yes, it checked out into .gm_actuals. https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/serv... gm/rebaseline_server/server.py:97: actuals_repo.Update('.') On 2013/09/26 19:02:01, borenet wrote: > Same question here as above: do we need to os.chdir(self._actuals_dir) in order > to use '.'? See above. It works as-is. https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/serv... gm/rebaseline_server/server.py:147: For now, we ignore the remaining path info, because we only know how to On 2013/09/26 19:02:01, borenet wrote: > On 2013/09/26 17:26:51, epoger wrote: > > This is a little funny... the client now requests /results/all instead of > > /results/$type . So we have this "vestigial" subpath which is currently > unused. > > Looks like I could request /results/pleasenoresults and still get "all". Is > there a plan to add back result types, or can we go ahead and remove the > subpath? Again, I file this under "we don't know yet, let's let this develop a bit and see." I added a TODO here. https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/serv... gm/rebaseline_server/server.py:206: 'defaults to %(default)s .'), On 2013/09/26 19:02:01, borenet wrote: > I think adding the period here will be confusing... Removed all trailing periods from the command-line option descriptions. My high school English teachers will be disappointed, but my co-workers will probably be happier. Seems like a reasonable tradeoff.
LGTM https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/resu... File gm/rebaseline_server/results.py (right): https://codereview.chromium.org/24274003/diff/28001/gm/rebaseline_server/resu... gm/rebaseline_server/results.py:49: def GetAll(self): On 2013/09/26 21:22:21, epoger wrote: > On 2013/09/26 19:02:01, borenet wrote: > > On 2013/09/26 17:26:51, epoger wrote: > > > Replaced the old OfType(result_type) method with GetAll() , that returns > > results > > > of *all* types and lets the client filter/sort them as desired. > > > > > > And generates that set of results ONCE, during __init__(), rather than > waiting > > > until the client requests them. > > > > If the client is going to filter by result type, would it be smarter to go > ahead > > and index by result type, so that the client's work is really fast? > > The short answer: "Maybe, but I don't think we know yet. I think we should > commit it as-is and then make adjustments." > > The long answer: > > - Currently, view.html has a dropdown at the very top of the page within which > the user chooses exactly one result type to see. But, as noted in a TODO there, > that will change very soon; we want to allow more flexibility in this (for > example, show a mix of both FAILED and FAILUREIGNORED in the same table). > > - Each result record includes the resultType, so it should be fairly > straightforward for the client view to be filtered with things exactly as they > are. > > - I think we need to add lazy loading of images, so we can see how much of the > performance problems of long lists is due to image loading (as opposed to > processing the list itself). All of the above SGTM
Message was sent while issue was closed.
Committed patchset #7 manually as r11500 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
