Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2157953002:
Change filter quality when scaling-down in drawImage (Closed)
|
Created:
4 years, 5 months ago by xidachen Modified:
4 years, 5 months ago Reviewers:
Justin Novosad CC:
chromium-reviews, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange filter quality when scaling-down in drawImage
When calling drawImage with imageSmoothingEnabled = false, we use nearest
neighbor as the filter. In the spec, it doesn't specify that we need to
use nearest neighbor when scaling down, so this CL changes to use
kLow_FitlerQuality when scaling down.
BUG=269089
Committed: https://crrev.com/6ea454313e099cb68ea91d31ef8f2f54ac4c393c
Cr-Commit-Position: refs/heads/master@{#406824}
Patch Set 1 #
Total comments: 3
Patch Set 2 : no need to change Image::draw API #
Total comments: 3
Patch Set 3 : account for transformation, layout test added #Patch Set 4 : change expected.txt #
Total comments: 2
Patch Set 5 : account for rotation and update layout test #
Total comments: 2
Patch Set 6 : address comments #Patch Set 7 : update test expectation #
Messages
Total messages: 33 (15 generated)
Description was changed from ========== Change filter quality when scaling-down in drawImage When calling drawImage with imageSmoothingEnabled = false, we use nearest neighbor as the filter. In the spec, it doesn't specify that we need to use nearest neighbor when scaling down, so this CL changes to use kLow_FitlerQuality when scaling down. BUG=269089 ========== to ========== Change filter quality when scaling-down in drawImage When calling drawImage with imageSmoothingEnabled = false, we use nearest neighbor as the filter. In the spec, it doesn't specify that we need to use nearest neighbor when scaling down, so this CL changes to use kLow_FitlerQuality when scaling down. BUG=269089 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
xidachen@chromium.org changed reviewers: + junov@chromium.org
PTAL
On 2016/07/18 15:30:26, xidachen wrote: > PTAL The crbug and and the spec change referenced by the bug are only about 2D canvas. a) file separate bugs and submit separate CLs for each API that you intend to change b) For each crbug, reference and quote the parts of the spec that show that the behaviour change is spec-compliant.
https://codereview.chromium.org/2157953002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/Image.h (right): https://codereview.chromium.org/2157953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/Image.h:153: virtual void draw(SkCanvas*, const SkPaint&, const FloatRect& dstRect, const FloatRect& srcRect, bool imageSmoothingEnabled, RespectImageOrientationEnum, ImageClampingMode) = 0; Instead of plumbing imageSmoothingEnabled in the Image API, why not adjust the SkPaint filter quality before calling draw(...)?
https://codereview.chromium.org/2157953002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2157953002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:261: adjustedPaint.setFilterQuality(kLow_SkFilterQuality); This should not have any effect. Ditto below, and likely for other cases like GradientGeneratedImage. https://codereview.chromium.org/2157953002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/Image.h (right): https://codereview.chromium.org/2157953002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/Image.h:153: virtual void draw(SkCanvas*, const SkPaint&, const FloatRect& dstRect, const FloatRect& srcRect, bool imageSmoothingEnabled, RespectImageOrientationEnum, ImageClampingMode) = 0; On 2016/07/18 at 15:46:38, f(malita) wrote: > Instead of plumbing imageSmoothingEnabled in the Image API, why not adjust the SkPaint filter quality before calling draw(...)? I was about to make a note about the same thing =). If it's about the "adjusted*Rect"s being different, I don't think that's the case, since (IIRC) the canvas code already clips the source rect and doesn't use image-orientation.
On 2016/07/18 15:46:38, f(malita) wrote: > https://codereview.chromium.org/2157953002/diff/1/third_party/WebKit/Source/p... > File third_party/WebKit/Source/platform/graphics/Image.h (right): > > https://codereview.chromium.org/2157953002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/graphics/Image.h:153: virtual void > draw(SkCanvas*, const SkPaint&, const FloatRect& dstRect, const FloatRect& > srcRect, bool imageSmoothingEnabled, RespectImageOrientationEnum, > ImageClampingMode) = 0; > Instead of plumbing imageSmoothingEnabled in the Image API, why not adjust the > SkPaint filter quality before calling draw(...)? That's a good suggestion, I thought about it, but I am not sure whether that would be correct or not, because I saw some kind of adjust srcRect and dstRect inside the draw() function call. That's why I thought whether it is scaling down or not may have to be done on the adjusted srcRect and dstRect. junov@: comments?
On 2016/07/18 15:57:00, xidachen wrote: > On 2016/07/18 15:46:38, f(malita) wrote: > > > https://codereview.chromium.org/2157953002/diff/1/third_party/WebKit/Source/p... > > File third_party/WebKit/Source/platform/graphics/Image.h (right): > > > > > https://codereview.chromium.org/2157953002/diff/1/third_party/WebKit/Source/p... > > third_party/WebKit/Source/platform/graphics/Image.h:153: virtual void > > draw(SkCanvas*, const SkPaint&, const FloatRect& dstRect, const FloatRect& > > srcRect, bool imageSmoothingEnabled, RespectImageOrientationEnum, > > ImageClampingMode) = 0; > > Instead of plumbing imageSmoothingEnabled in the Image API, why not adjust the > > SkPaint filter quality before calling draw(...)? > > That's a good suggestion, I thought about it, but I am not sure whether that > would be correct or not, because I saw some kind of adjust srcRect and dstRect > inside the draw() function call. That's why I thought whether it is scaling down > or not may have to be done on the adjusted srcRect and dstRect. > > junov@: comments? Yeah, the way things are currently implemented, it is not convenient to determine the filter upstream. Ideally, SkPaint would have min/mag filters like OpenGL does, and then this would be trivial, but it doesn't.
On 2016/07/18 15:56:55, fs wrote: > https://codereview.chromium.org/2157953002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): > > https://codereview.chromium.org/2157953002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:261: > adjustedPaint.setFilterQuality(kLow_SkFilterQuality); > This should not have any effect. Ditto below, and likely for other cases like > GradientGeneratedImage. > > https://codereview.chromium.org/2157953002/diff/1/third_party/WebKit/Source/p... > File third_party/WebKit/Source/platform/graphics/Image.h (right): > > https://codereview.chromium.org/2157953002/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/graphics/Image.h:153: virtual void > draw(SkCanvas*, const SkPaint&, const FloatRect& dstRect, const FloatRect& > srcRect, bool imageSmoothingEnabled, RespectImageOrientationEnum, > ImageClampingMode) = 0; > On 2016/07/18 at 15:46:38, f(malita) wrote: > > Instead of plumbing imageSmoothingEnabled in the Image API, why not adjust the > SkPaint filter quality before calling draw(...)? > > I was about to make a note about the same thing =). If it's about the > "adjusted*Rect"s being different, I don't think that's the case, since (IIRC) > the canvas code already clips the source rect and doesn't use image-orientation. Right, the adjustment is just for mapping the clipping, which does not affect the transform. So I retract my previous comment. We can determine the filter upfront.
Description was changed from ========== Change filter quality when scaling-down in drawImage When calling drawImage with imageSmoothingEnabled = false, we use nearest neighbor as the filter. In the spec, it doesn't specify that we need to use nearest neighbor when scaling down, so this CL changes to use kLow_FitlerQuality when scaling down. BUG=269089 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Change filter quality when scaling-down in drawImage When calling drawImage with imageSmoothingEnabled = false, we use nearest neighbor as the filter. In the spec, it doesn't specify that we need to use nearest neighbor when scaling down, so this CL changes to use kLow_FitlerQuality when scaling down. BUG=269089 ==========
https://codereview.chromium.org/2157953002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/canvas-imageSmoothingQuality.html (right): https://codereview.chromium.org/2157953002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/canvas-imageSmoothingQuality.html:88: shouldBe("sampleAlpha(noFilterData)", "sampleAlpha(lowData)"); Would this test enough? should I add a new test doing drawImage()?
https://codereview.chromium.org/2157953002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp (right): https://codereview.chromium.org/2157953002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:946: return dstRect.width() < srcRect.width() && dstRect.height() < srcRect.height(); This is not good enough. You also have to take into account the transform matrix. An the test need to cover cases like rotation+scale correctly.
https://codereview.chromium.org/2157953002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp (right): https://codereview.chromium.org/2157953002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:946: return dstRect.width() < srcRect.width() && dstRect.height() < srcRect.height(); On 2016/07/19 17:17:16, Justin Novosad wrote: > This is not good enough. You also have to take into account the transform > matrix. An the test need to cover cases like rotation+scale correctly. Done.
https://codereview.chromium.org/2157953002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp (right): https://codereview.chromium.org/2157953002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:980: if (!imageSmoothingEnabled() && isDrawScalingDown(srcRect, state().transform().mapRect(dstRect))) Using mapRect is not correct. For example if you scale by 0.9 and rotate by 45deg. The image is being scaled-down, but map rect s going to give you the bounding box of the tranformed rect, which will be bigger than the original rect by a factor of 0.9*sqrt(2) = 1.27.
https://codereview.chromium.org/2157953002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp (right): https://codereview.chromium.org/2157953002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:980: if (!imageSmoothingEnabled() && isDrawScalingDown(srcRect, state().transform().mapRect(dstRect))) On 2016/07/20 17:36:36, Justin Novosad wrote: > Using mapRect is not correct. > For example if you scale by 0.9 and rotate by 45deg. The image is being > scaled-down, but map rect s going to give you the bounding box of the tranformed > rect, which will be bigger than the original rect by a factor of 0.9*sqrt(2) = > 1.27. Done.
https://codereview.chromium.org/2157953002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp (right): https://codereview.chromium.org/2157953002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:946: return dstRect.width() * xScale < srcRect.width() && dstRect.height() * yScale < srcRect.height(); I had an idea: you can optimize out the expensive sqrt call inside xScale/yScale by squaring both sides of the inequality. The expression becomes: dstRect.width() * dstRect.width() * xScaleSquared < srcRect.width() * srcRect.width() Then you do the same for height. The rects are stored in floats and the transform is in double. The way it is written now, the floats will be promoted to double in the expression. We wight as well do all the arithmetic in float (like in the rest of the canvas implementation), so you should make the args float.
https://codereview.chromium.org/2157953002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp (right): https://codereview.chromium.org/2157953002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp:946: return dstRect.width() * xScale < srcRect.width() && dstRect.height() * yScale < srcRect.height(); On 2016/07/20 19:43:56, Justin Novosad wrote: > I had an idea: you can optimize out the expensive sqrt call inside xScale/yScale > by squaring both sides of the inequality. > The expression becomes: dstRect.width() * dstRect.width() * xScaleSquared < > srcRect.width() * srcRect.width() > Then you do the same for height. > > The rects are stored in floats and the transform is in double. The way it is > written now, the floats will be promoted to double in the expression. We wight > as well do all the arithmetic in float (like in the rest of the canvas > implementation), so you should make the args float. Done.
lgtm
The CQ bit was checked by xidachen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xidachen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xidachen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org Link to the patchset: https://codereview.chromium.org/2157953002/#ps120001 (title: "update test expectation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Change filter quality when scaling-down in drawImage When calling drawImage with imageSmoothingEnabled = false, we use nearest neighbor as the filter. In the spec, it doesn't specify that we need to use nearest neighbor when scaling down, so this CL changes to use kLow_FitlerQuality when scaling down. BUG=269089 ========== to ========== Change filter quality when scaling-down in drawImage When calling drawImage with imageSmoothingEnabled = false, we use nearest neighbor as the filter. In the spec, it doesn't specify that we need to use nearest neighbor when scaling down, so this CL changes to use kLow_FitlerQuality when scaling down. BUG=269089 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Change filter quality when scaling-down in drawImage When calling drawImage with imageSmoothingEnabled = false, we use nearest neighbor as the filter. In the spec, it doesn't specify that we need to use nearest neighbor when scaling down, so this CL changes to use kLow_FitlerQuality when scaling down. BUG=269089 ========== to ========== Change filter quality when scaling-down in drawImage When calling drawImage with imageSmoothingEnabled = false, we use nearest neighbor as the filter. In the spec, it doesn't specify that we need to use nearest neighbor when scaling down, so this CL changes to use kLow_FitlerQuality when scaling down. BUG=269089 Committed: https://crrev.com/6ea454313e099cb68ea91d31ef8f2f54ac4c393c Cr-Commit-Position: refs/heads/master@{#406824} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/6ea454313e099cb68ea91d31ef8f2f54ac4c393c Cr-Commit-Position: refs/heads/master@{#406824} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
