Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
105823010:
resize mask canvas for viewer.html in skpdiff tool (Closed)
|
Created:
7 years ago by yunchao Modified:
7 years ago Reviewers:
djsollen CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionThe default size of mask canvas is not appropriate. its height is too small: 150, while its width is 300. As a result, for non-alphaMask canvas who doesn't update the mask canvas size, this tool just magnify the upper part of 'Expected Image' as well as 'Actual Image' when user open view.html to show skpdiff_output.json in browser for many cases. The other part can't be shown because it is out of the mask canvas. This CL update non-alphaMask canvas size according to baseline canvas for each case, then the tool can magnify anywhere you want for the whole image when you click and move mouse on the mask canvas.
Committed: http://code.google.com/p/skia/source/detail?r=12638
Patch Set 1 : set the height for mask canvas accordingly for each case #
Total comments: 8
Patch Set 2 : resize non-alphaMask canvas according to baseline canvas #
Total comments: 14
Patch Set 3 : updated code accordinding to derek's suggestions #Messages
Total messages: 11 (0 generated)
https://codereview.chromium.org/105823010/diff/20001/tools/skpdiff/diff_viewe... File tools/skpdiff/diff_viewer.js (right): https://codereview.chromium.org/105823010/diff/20001/tools/skpdiff/diff_viewe... tools/skpdiff/diff_viewer.js:29: scope.setHWRatio4MaskCanvas(canvas.height / canvas.width); I think you can remove this. The width and height of the canvas will not be that of the image until the image.onload completes so I don't see the benefit here. https://codereview.chromium.org/105823010/diff/20001/tools/skpdiff/diff_viewe... tools/skpdiff/diff_viewer.js:61: scope.setHWRatio4MaskCanvas(canvas.height / canvas.width); it seems like we only want to do this for the non-alphaMask canvas. I think your missing some kind of conditional check here. https://codereview.chromium.org/105823010/diff/20001/tools/skpdiff/diff_viewe... tools/skpdiff/diff_viewer.js:142: canvas.height = canvas.width * scope.HWRatio4MaskCanvas; why does the canvas height need to be set each time the magnifier is drawn. It seems like it should only need to be done one time. https://codereview.chromium.org/105823010/diff/20001/tools/skpdiff/diff_viewe... tools/skpdiff/diff_viewer.js:190: if (ratio > $scope.HWRatio4MaskCanvas) { I don't see why the setter should be conditional. I think you are just calling the setter too many times.
Hi Derek, thanks for your reviewing. I have update the code according to you suggestion. I resized both the height and the width for non-alphaMask Canvas. For some cases, the width of baseline/test canvas is smaller than 300. For example, if the image width is 820, the width of baseline/test canvas will be 205(divided by 2, and should be smaller than 400). So, If keep the same height width ratio for mask canvas, the mask canvas will bigger than baseline/test canvas. The bigger part of mask canvas will magnify nothing but blank. This is unnecessary. So the new patchset resize non-alphaMask canvas according to baseline canvas size. https://codereview.chromium.org/105823010/diff/20001/tools/skpdiff/diff_viewe... File tools/skpdiff/diff_viewer.js (right): https://codereview.chromium.org/105823010/diff/20001/tools/skpdiff/diff_viewe... tools/skpdiff/diff_viewer.js:29: scope.setHWRatio4MaskCanvas(canvas.height / canvas.width); On 2013/12/09 16:40:12, djsollen wrote: > I think you can remove this. The width and height of the canvas will not be > that of the image until the image.onload completes so I don't see the benefit > here. Agree! https://codereview.chromium.org/105823010/diff/20001/tools/skpdiff/diff_viewe... tools/skpdiff/diff_viewer.js:61: scope.setHWRatio4MaskCanvas(canvas.height / canvas.width); On 2013/12/09 16:40:12, djsollen wrote: > it seems like we only want to do this for the non-alphaMask canvas. I think > your missing some kind of conditional check here. Done. https://codereview.chromium.org/105823010/diff/20001/tools/skpdiff/diff_viewe... tools/skpdiff/diff_viewer.js:142: canvas.height = canvas.width * scope.HWRatio4MaskCanvas; On 2013/12/09 16:40:12, djsollen wrote: > why does the canvas height need to be set each time the magnifier is drawn. It > seems like it should only need to be done one time. Done, resize non-alphaMask canvas when loading image and resizing baseline canvas. https://codereview.chromium.org/105823010/diff/20001/tools/skpdiff/diff_viewe... tools/skpdiff/diff_viewer.js:190: if (ratio > $scope.HWRatio4MaskCanvas) { On 2013/12/09 16:40:12, djsollen wrote: > I don't see why the setter should be conditional. I think you are just calling > the setter too many times. Agree. have updated this patch accordingly.
https://codereview.chromium.org/105823010/diff/60001/tools/skpdiff/diff_viewe... File tools/skpdiff/diff_viewer.js (left): https://codereview.chromium.org/105823010/diff/60001/tools/skpdiff/diff_viewe... tools/skpdiff/diff_viewer.js:6: // TODO add support for a magnified scale factor this TODO was to track making the strength of the magnifier user configurable. https://codereview.chromium.org/105823010/diff/60001/tools/skpdiff/diff_viewe... File tools/skpdiff/diff_viewer.js (right): https://codereview.chromium.org/105823010/diff/60001/tools/skpdiff/diff_viewe... tools/skpdiff/diff_viewer.js:31: if (image.name == "") { This if check is unnecessary. I don't think the alphaMask element ever has a name assigned. https://codereview.chromium.org/105823010/diff/60001/tools/skpdiff/diff_viewe... tools/skpdiff/diff_viewer.js:66: scope.setNeedChangeMaskCanvasSize(false); you should be passing true if you really only want this to run 1x. https://codereview.chromium.org/105823010/diff/60001/tools/skpdiff/diff_viewe... tools/skpdiff/diff_viewer.js:82: }) this should be }); https://codereview.chromium.org/105823010/diff/60001/tools/skpdiff/diff_viewe... tools/skpdiff/diff_viewer.js:191: $scope.needChangeMaskCanvasSize = false; I think it may be more clear to use names like $scope.maskSizeUpdated = undefined; $scope.maskSizeInitialized = false; https://codereview.chromium.org/105823010/diff/60001/tools/skpdiff/diff_viewe... tools/skpdiff/diff_viewer.js:205: $scope.resizeMaskCanvas = function(newMaskCanvas) { function(updatedSize)
Thanks for you reviewing, Derek, I have one question below. https://codereview.chromium.org/105823010/diff/60001/tools/skpdiff/diff_viewe... File tools/skpdiff/diff_viewer.js (right): https://codereview.chromium.org/105823010/diff/60001/tools/skpdiff/diff_viewe... tools/skpdiff/diff_viewer.js:31: if (image.name == "") { On 2013/12/10 14:18:15, djsollen wrote: > This if check is unnecessary. I don't think the alphaMask element ever has a > name assigned. Hey derek, how to depart a non-alphaMask element from an alphaMask element? I found this error(can't magnify all part for baseline/test image) in GM results. But all cases of skpdiff_output.json from GM results have no alphaMask element. The differencePath for alphaMask element are all NULL. Then image.src is set to PATH/viewer.html here. I can't find a case has alphaMask element. https://codereview.chromium.org/105823010/diff/60001/tools/skpdiff/diff_viewe... tools/skpdiff/diff_viewer.js:66: scope.setNeedChangeMaskCanvasSize(false); On 2013/12/10 14:18:15, djsollen wrote: > you should be passing true if you really only want this to run 1x. I suppose line 31-33 can pass true. And update canvas size for non-alphaMask canvas when loading baseline image. Then set false here to prevent from resizing the mask canvas again when loading test image.
https://codereview.chromium.org/105823010/diff/60001/tools/skpdiff/diff_viewe... File tools/skpdiff/diff_viewer.js (right): https://codereview.chromium.org/105823010/diff/60001/tools/skpdiff/diff_viewe... tools/skpdiff/diff_viewer.js:31: if (image.name == "") { On 2013/12/10 14:35:00, yunchao wrote: > On 2013/12/10 14:18:15, djsollen wrote: > > This if check is unnecessary. I don't think the alphaMask element ever has a > > name assigned. > > Hey derek, how to depart a non-alphaMask element from an alphaMask element? I > found this error(can't magnify all part for baseline/test image) in GM results. > But all cases of skpdiff_output.json from GM results have no alphaMask element. > The differencePath for alphaMask element are all NULL. Then image.src is set to > PATH/viewer.html here. I can't find a case has alphaMask element. You need to run the skpdiff tool with the --alphaDir option to have it produce the alpha mask. https://codereview.chromium.org/105823010/diff/60001/tools/skpdiff/diff_viewe... tools/skpdiff/diff_viewer.js:66: scope.setNeedChangeMaskCanvasSize(false); On 2013/12/10 14:35:00, yunchao wrote: > On 2013/12/10 14:18:15, djsollen wrote: > > you should be passing true if you really only want this to run 1x. > > I suppose line 31-33 can pass true. And update canvas size for non-alphaMask > canvas when loading baseline image. Then set false here to prevent from resizing > the mask canvas again when loading test image. I don't think 31-33 needs to exist as the scope object will initialize that variable to false.
On 2013/12/10 14:44:20, djsollen wrote: > https://codereview.chromium.org/105823010/diff/60001/tools/skpdiff/diff_viewe... > File tools/skpdiff/diff_viewer.js (right): > > https://codereview.chromium.org/105823010/diff/60001/tools/skpdiff/diff_viewe... > tools/skpdiff/diff_viewer.js:31: if (image.name == "") { > On 2013/12/10 14:35:00, yunchao wrote: > > On 2013/12/10 14:18:15, djsollen wrote: > > > This if check is unnecessary. I don't think the alphaMask element ever has > a > > > name assigned. > > > > Hey derek, how to depart a non-alphaMask element from an alphaMask element? I > > found this error(can't magnify all part for baseline/test image) in GM > results. > > But all cases of skpdiff_output.json from GM results have no alphaMask > element. > > The differencePath for alphaMask element are all NULL. Then image.src is set > to > > PATH/viewer.html here. I can't find a case has alphaMask element. > > You need to run the skpdiff tool with the --alphaDir option to have it produce > the alpha mask. > > https://codereview.chromium.org/105823010/diff/60001/tools/skpdiff/diff_viewe... > tools/skpdiff/diff_viewer.js:66: scope.setNeedChangeMaskCanvasSize(false); > On 2013/12/10 14:35:00, yunchao wrote: > > On 2013/12/10 14:18:15, djsollen wrote: > > > you should be passing true if you really only want this to run 1x. > > > > I suppose line 31-33 can pass true. And update canvas size for non-alphaMask > > canvas when loading baseline image. Then set false here to prevent from > resizing > > the mask canvas again when loading test image. > > I don't think 31-33 needs to exist as the scope object will initialize that > variable to false. OK, I will test and update the code tomorrow.
Hi Derek, I have updated the code according to your suggestions. Please help to review it. https://codereview.chromium.org/105823010/diff/60001/tools/skpdiff/diff_viewe... File tools/skpdiff/diff_viewer.js (right): https://codereview.chromium.org/105823010/diff/60001/tools/skpdiff/diff_viewe... tools/skpdiff/diff_viewer.js:31: if (image.name == "") { On 2013/12/10 14:44:21, djsollen wrote: > On 2013/12/10 14:35:00, yunchao wrote: > > On 2013/12/10 14:18:15, djsollen wrote: > > > This if check is unnecessary. I don't think the alphaMask element ever has > a > > > name assigned. > > > > Hey derek, how to depart a non-alphaMask element from an alphaMask element? I > > found this error(can't magnify all part for baseline/test image) in GM > results. > > But all cases of skpdiff_output.json from GM results have no alphaMask > element. > > The differencePath for alphaMask element are all NULL. Then image.src is set > to > > PATH/viewer.html here. I can't find a case has alphaMask element. > > You need to run the skpdiff tool with the --alphaDir option to have it produce > the alpha mask. Yes. Then I found this CL is a corner case, because in most cases, alphaDir should be designated. https://codereview.chromium.org/105823010/diff/60001/tools/skpdiff/diff_viewe... tools/skpdiff/diff_viewer.js:66: scope.setNeedChangeMaskCanvasSize(false); On 2013/12/10 14:44:21, djsollen wrote: > On 2013/12/10 14:35:00, yunchao wrote: > > On 2013/12/10 14:18:15, djsollen wrote: > > > you should be passing true if you really only want this to run 1x. > > > > I suppose line 31-33 can pass true. And update canvas size for non-alphaMask > > canvas when loading baseline image. Then set false here to prevent from > resizing > > the mask canvas again when loading test image. > > I don't think 31-33 needs to exist as the scope object will initialize that > variable to false. > Yes, the original patch line 31-33 is unnecessary. But the code is executed after scope object initialization. I have traced this in devtools. https://codereview.chromium.org/105823010/diff/60001/tools/skpdiff/diff_viewe... tools/skpdiff/diff_viewer.js:82: }) On 2013/12/10 14:18:15, djsollen wrote: > this should be }); Done. https://codereview.chromium.org/105823010/diff/60001/tools/skpdiff/diff_viewe... tools/skpdiff/diff_viewer.js:191: $scope.needChangeMaskCanvasSize = false; On 2013/12/10 14:18:15, djsollen wrote: > I think it may be more clear to use names like > > $scope.maskSizeUpdated = undefined; > $scope.maskSizeInitialized = false; Agree, the original variable names are not good. The mask Canvas size is initialized to width=300, height=150 in line 18. So I didn't use variable maskSizeInitialized. I use variable maskSizeUpdated as you suggested to record the state(size updated or not), and use updatedMaskSize to record the new size when updating.
lgtm
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/yunchao.he@intel.com/105823010/100001
Message was sent while issue was closed.
Change committed as 12638 |
