Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
210043004:
Draw thin slices of an image w/ anti-aliasing for 2D <canvas> (Closed)
|
Created:
6 years, 9 months ago by fs Modified:
6 years, 8 months ago CC:
blink-reviews, jamesr, krit, dsinclair, jbroman, danakj, pdr., rwlbuis Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionDraw thin slices of an image w/ anti-aliasing for 2D <canvas>
Add a GraphicsContext-scoped flag that allows anti-aliasing of
(all) image-geometry to be enabled in a selective manner.
Enable this behavior for CanvasRenderingContext2D.
When this flag is set, image geometry that would drawn as very
thin (< 1 device pixel wide/high) will be anti-aliased.
BUG=352611
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170671
Patch Set 1 #Patch Set 2 : Add fabs(). #Patch Set 3 : TC. #
Total comments: 3
Patch Set 4 : Revised solution; Add simple perftest. #Patch Set 5 : Update TestExpectations. #Patch Set 6 : Rebase. #Patch Set 7 : Combined solution. #
Total comments: 2
Patch Set 8 : Add 'hairline' to names. #Patch Set 9 : Rebase; Rename some more. #
Total comments: 2
Patch Set 10 : Clarifications. #
Messages
Total messages: 32 (0 generated)
I'd hoped to get a discussion going, but it seemed that didn't even trigger a "meh", so here's forcing the discussion a bit... =)
Querying the CTM at "draw" time inside blink is not reliable. It may not predict the actual scale/transform that will be used when the drawing is actually rasterized. I think we need to move Blink in the direction of *never* querying the context for the current matrix. For this particular problem, does your antialiasing trick always draw the right thing? If so, perhaps we should just always set antialiasing on. If it doesn't always "do the right thing", then I'd like to understand what is going on more deeply, to see if we can come up with a device-independent drawing semantic for you that will work.
I totally agree with this change. lgtm with nits. https://codereview.chromium.org/210043004/diff/60001/LayoutTests/canvas/thin-... File LayoutTests/canvas/thin-drawImages.html (right): https://codereview.chromium.org/210043004/diff/60001/LayoutTests/canvas/thin-... LayoutTests/canvas/thin-drawImages.html:8: break; Nit: I think it would be more readable if you just returned false here and true at the end. https://codereview.chromium.org/210043004/diff/60001/Source/platform/graphics... File Source/platform/graphics/skia/NativeImageSkia.cpp (right): https://codereview.chromium.org/210043004/diff/60001/Source/platform/graphics... Source/platform/graphics/skia/NativeImageSkia.cpp:344: // the relevant components of the matrix to compute the transformed vector. Reformulate for clarity. Suggestion : Since we know that kRectStaysRect_Mask is set, the matrix either has scale and no skew or vice versa. We can query the kAffine_Mask flag to determine which case it is.
Oh yeah, not lgtm because of what mike said. We need to do this on the skia side.
On 2014/03/25 14:24:55, reed1 wrote: > Querying the CTM at "draw" time inside blink is not reliable. It may not predict > the actual scale/transform that will be used when the drawing is actually > rasterized. I think we need to move Blink in the direction of *never* querying > the context for the current matrix. Yes. We really need a plan to solve this problem because it bites us regularly. But it's hard to see what to do without having conditionals in SkPicture, or callbacks when rasterizing, or always assuming that we have the "bad" case. The problem is that we need to do logically different things based on the transform, and that conditional logic needs to be there at rasterization if that is the only time we can rely on the CTM. None of those options is even slightly appealing. > For this particular problem, does your antialiasing trick always draw the right > thing? If so, perhaps we should just always set antialiasing on. If it doesn't > always "do the right thing", then I'd like to understand what is going on more > deeply, to see if we can come up with a device-independent drawing semantic for > you that will work. Mike, what's the performance impact of always turning on anti-aliasing? Does it only affect border pixels of the image? If the performance cost is not big, then I would be in favor of always anti-aliasing because it reduces the chance of artifacts when we turn anti-aliasing on and off at different sizes/scales. Failing that, I agree that the real fix may be in Skia at rasterization time, where all the transform state is known.
My understanding is that this is a 2D canvas only bug because in regular HTML, boxes are snapped to integer pixel coordinates (except maybe with CSS 3D transforms?) In the context of a 2D canvas, the CTM is reliable with respect to the canvas's backing store. Perhaps a possible solution would be to move this logic up the stack into CanvasRenderingContext2D.
On 2014/03/25 14:24:55, reed1 wrote: > Querying the CTM at "draw" time inside blink is not reliable. It may not predict > the actual scale/transform that will be used when the drawing is actually > rasterized. I think we need to move Blink in the direction of *never* querying > the context for the current matrix. Agreed, but it's the same story as with the current "is it rotated/rect-to-rect" predicate... > For this particular problem, does your antialiasing trick always draw the right > thing? If so, perhaps we should just always set antialiasing on. If it doesn't > always "do the right thing", then I'd like to understand what is going on more > deeply, to see if we can come up with a device-independent drawing semantic for > you that will work. Could expand on what you mean by "the right thing" here? I mean it certainly doesn't draw "the right thing" for larger geometry (> 1 device pixel) with a sub-pixel offset for instance - but that's unchanged from without this fix/tweak. I certainly would like to always enable anti-aliasing, and just let Skia deal with it - I guess I was assuming that this was the way it was because of some reason or another (likely performance - there'll be an additional cost for the potentially sub-pixel positioned edges after all.)
FWIW, I think we should avoid enabling anti-aliasing globally. I am concerned that this could break existing apps that rely on the absence of seams in quad strips (or grids) that are not perfectly aligned with pixels.
On 2014/03/25 14:42:10, junov wrote: > My understanding is that this is a 2D canvas only bug because in regular HTML, > boxes are snapped to integer pixel coordinates (except maybe with CSS 3D > transforms?) > In the context of a 2D canvas, the CTM is reliable with respect to the canvas's > backing store. Perhaps a possible solution would be to move this logic up the > stack into CanvasRenderingContext2D. I suppose that optionally it could be achieved by something like: paint.setAntiAlias(context->shouldAntialias() /* || oldPredicate */); and then have CRC2D set that to true (always)?
On 2014/03/25 14:56:49, fs wrote: > On 2014/03/25 14:24:55, reed1 wrote: > > Querying the CTM at "draw" time inside blink is not reliable. It may not > predict > > the actual scale/transform that will be used when the drawing is actually > > rasterized. I think we need to move Blink in the direction of *never* querying > > the context for the current matrix. > > Agreed, but it's the same story as with the current "is it rotated/rect-to-rect" > predicate... FWIW, I think in practice we only tweak the scale/translation at playback time (device scale factor, pinch zoom) - so rotation/skew based decisions should hold. But that's still fragile and the agreement is we should try to avoid (and remove) any CTM-based decisions in Blink.
On 2014/03/25 15:39:17, Florin Malita wrote: > FWIW, I think in practice we only tweak the scale/translation at playback time > (device scale factor, pinch zoom) - so rotation/skew based decisions should > hold. But that's still fragile and the agreement is we should try to avoid (and > remove) any CTM-based decisions in Blink. It is actually a scale based-decision. It is just that when rotated +/-90 degrees, the scale is in the skew component.
On 2014/03/25 15:11:10, fs wrote: > On 2014/03/25 14:42:10, junov wrote: > > My understanding is that this is a 2D canvas only bug because in regular HTML, > > boxes are snapped to integer pixel coordinates (except maybe with CSS 3D > > transforms?) > > In the context of a 2D canvas, the CTM is reliable with respect to the > canvas's > > backing store. Perhaps a possible solution would be to move this logic up the > > stack into CanvasRenderingContext2D. > > I suppose that optionally it could be achieved by something like: > > paint.setAntiAlias(context->shouldAntialias() /* || oldPredicate */); > > and then have CRC2D set that to true (always)? ...and it seems shouldAntialias() is default to 'true' already, which makes a change like the above essentially equal to setting it globally - unless a new flag is added for images of course... (but that smells a bit)
1. antialiasing only has a cost on the edges. it does not scale up with the area. 2. if we are worried about seaming (which sometimes is the intent), then disappearing may be precisely the correct behavior. I don't have a magical solution in hand, but it imagine that part of a better solution involves knowing more from the client: do they want seaming (no-aa) or smooth edges/animations (aa). I know this doesn't help us here, but I would like to be part of a larger discussion along these lines.
On 2014/03/25 16:14:12, reed1 wrote: > 1. antialiasing only has a cost on the edges. it does not scale up with the > area. > 2. if we are worried about seaming (which sometimes is the intent), then > disappearing may be precisely the correct behavior. > > I don't have a magical solution in hand, but it imagine that part of a better > solution involves knowing more from the client: do they want seaming (no-aa) or > smooth edges/animations (aa). I know this doesn't help us here, but I would like > to be part of a larger discussion along these lines. Every once in a while the idea of adding an option to enable/disable AA in 2D canvas re-surfaces, and gets dismissed as an implementation decision. The spec is deliberately (and annoyingly IMHO) anti-aliasing agnostic. Knowing more from the client definitely makes sense, but we need to make a solid case to get the necessary API it into the web standard. Previous attempts have failed.
On 2014/03/25 16:14:12, reed1 wrote: > 2. if we are worried about seaming (which sometimes is the intent), then > disappearing may be precisely the correct behavior. > > I don't have a magical solution in hand, but it imagine that part of a better > solution involves knowing more from the client: do they want seaming (no-aa) or > smooth edges/animations (aa). I know this doesn't help us here, but I would like > to be part of a larger discussion along these lines. Well, "seaming" will not only be a problem between images and images - but also between image (geometry) and other geometry - and AFAICS the latter will be painted with AA enabled. And with MSAA or similar you shouldn't need to worry though so it most definitely is a decision better based on higher level information... I think I'll try the "ugly flag" version mentioned above, and see how that goes.
Thanks for the feedback so far. Uploaded a new patchset w/ the GC-global enabling flag. Also added a simple perftest (minor modification of the existing drawimage.html) to measure the impact. Locally on my machine there was a different of ~8% between w/ and w/o AA (software rendering - I'd expect accelerated code-paths to affect less - if at all.) https://codereview.chromium.org/210043004/diff/60001/LayoutTests/canvas/thin-... File LayoutTests/canvas/thin-drawImages.html (right): https://codereview.chromium.org/210043004/diff/60001/LayoutTests/canvas/thin-... LayoutTests/canvas/thin-drawImages.html:8: break; On 2014/03/25 14:35:07, junov wrote: > Nit: I think it would be more readable if you just returned false here and true > at the end. Done.
On 2014/03/25 14:07:38, fs wrote: > I'd hoped to get a discussion going, but it seemed that didn't even trigger a > "meh", so here's forcing the discussion a bit... =) Is this related to this: http://lists.w3.org/Archives/Public/public-whatwg-archive/2014Mar/0195.html Looks like Blink doesn't look outside the source rect at drawImage time. Maybe it's the same problem?
On 2014/03/26 00:18:25, Rik wrote: > On 2014/03/25 14:07:38, fs wrote: > > I'd hoped to get a discussion going, but it seemed that didn't even trigger a > > "meh", so here's forcing the discussion a bit... =) > > Is this related to this: > http://lists.w3.org/Archives/Public/public-whatwg-archive/2014Mar/0195.html > Looks like Blink doesn't look outside the source rect at drawImage time. Maybe > it's the same problem? No, this has nothing to do with sampling from the source image. What's described there is likely what shouldClampToSourceRect is supposed to achieve.
On 2014/03/25 17:18:50, fs wrote: > Thanks for the feedback so far. Uploaded a new patchset w/ the GC-global > enabling flag. > > Also added a simple perftest (minor modification of the existing drawimage.html) > to measure the impact. Locally on my machine there was a different of ~8% > between w/ and w/o AA (software rendering - I'd expect accelerated code-paths to > affect less - if at all.) Given the 8% number above I feel somewhat inclined to take the safer route and have both the "kill-switch" from the latest PS, and the heuristic (from PS < 4). Presumably accompanied about a comment that reading the CTM (for scale) isn't a great idea. That ought to dodge the 8% while fixing the bug, and only affect Canvas. Any other ideas on how to move this forward?
Uploaded a version which combines the "thin slice" detection with a flag to allow only enabling it for <canvas> (i.e. "the best of both worlds"). PTAL.
https://codereview.chromium.org/210043004/diff/440001/Source/platform/graphic... File Source/platform/graphics/GraphicsContext.h (right): https://codereview.chromium.org/210043004/diff/440001/Source/platform/graphic... Source/platform/graphics/GraphicsContext.h:143: bool shouldAntialiasImages() const { return m_antialiasImages && shouldAntialias(); } I think the name is a bit misleading. Perhaps: shouldAntialiasHairline Images? or thin images? something like that. The fact is that rotated/skewed images will still be draw anti-aliased even if this bit is not set.
https://codereview.chromium.org/210043004/diff/440001/Source/platform/graphic... File Source/platform/graphics/GraphicsContext.h (right): https://codereview.chromium.org/210043004/diff/440001/Source/platform/graphic... Source/platform/graphics/GraphicsContext.h:143: bool shouldAntialiasImages() const { return m_antialiasImages && shouldAntialias(); } On 2014/03/28 18:20:38, junov wrote: > I think the name is a bit misleading. Perhaps: shouldAntialiasHairline Images? > or thin images? something like that. The fact is that rotated/skewed images > will still be draw anti-aliased even if this bit is not set. Makes sense. Renamed accordingly. It also occurred to me that it might make sense to reverse the logic (and hence settings) to always enable AA, but then selectively disable it for the rectilinear cases (except the "hairline" case, which would then still have a flag I suppose - so no major improvement in cleanliness.) If that sounds reasonable, I'd be happy to do that change here or as a follow up CL.
On 2014/03/31 08:21:11, fs wrote: > https://codereview.chromium.org/210043004/diff/440001/Source/platform/graphic... > File Source/platform/graphics/GraphicsContext.h (right): > > https://codereview.chromium.org/210043004/diff/440001/Source/platform/graphic... > Source/platform/graphics/GraphicsContext.h:143: bool shouldAntialiasImages() > const { return m_antialiasImages && shouldAntialias(); } > On 2014/03/28 18:20:38, junov wrote: > > I think the name is a bit misleading. Perhaps: shouldAntialiasHairline Images? > > or thin images? something like that. The fact is that rotated/skewed images > > will still be draw anti-aliased even if this bit is not set. > > Makes sense. Renamed accordingly. > > It also occurred to me that it might make sense to reverse the logic (and hence > settings) to always enable AA, but then selectively disable it for the > rectilinear cases (except the "hairline" case, which would then still have a > flag I suppose - so no major improvement in cleanliness.) If that sounds > reasonable, I'd be happy to do that change here or as a follow up CL. Ah, the reversed logic actually makes a lot of sense I think. And it will make this code less confusing. Then, the disabling of AA for axis-aligned rects could be seen as an optimization option which could be set to include or exclude hairline rects.
On 2014/03/31 15:14:45, junov wrote: > On 2014/03/31 08:21:11, fs wrote: > > > https://codereview.chromium.org/210043004/diff/440001/Source/platform/graphic... > > File Source/platform/graphics/GraphicsContext.h (right): > > > > > https://codereview.chromium.org/210043004/diff/440001/Source/platform/graphic... > > Source/platform/graphics/GraphicsContext.h:143: bool shouldAntialiasImages() > > const { return m_antialiasImages && shouldAntialias(); } > > On 2014/03/28 18:20:38, junov wrote: > > > I think the name is a bit misleading. Perhaps: shouldAntialiasHairline > Images? > > > or thin images? something like that. The fact is that rotated/skewed images > > > will still be draw anti-aliased even if this bit is not set. > > > > Makes sense. Renamed accordingly. > > > > It also occurred to me that it might make sense to reverse the logic (and > hence > > settings) to always enable AA, but then selectively disable it for the > > rectilinear cases (except the "hairline" case, which would then still have a > > flag I suppose - so no major improvement in cleanliness.) If that sounds > > reasonable, I'd be happy to do that change here or as a follow up CL. > > Ah, the reversed logic actually makes a lot of sense I think. And it will make > this code less confusing. Then, the disabling of AA for axis-aligned rects could > be seen as an optimization option which could be set to include or exclude > hairline rects. Yepp, that would be the interpretation seen from that "angle".
On 2014/03/31 15:30:20, fs wrote: > On 2014/03/31 15:14:45, junov wrote: > > On 2014/03/31 08:21:11, fs wrote: > > > > > > https://codereview.chromium.org/210043004/diff/440001/Source/platform/graphic... > > > File Source/platform/graphics/GraphicsContext.h (right): > > > > > > > > > https://codereview.chromium.org/210043004/diff/440001/Source/platform/graphic... > > > Source/platform/graphics/GraphicsContext.h:143: bool shouldAntialiasImages() > > > const { return m_antialiasImages && shouldAntialias(); } > > > On 2014/03/28 18:20:38, junov wrote: > > > > I think the name is a bit misleading. Perhaps: shouldAntialiasHairline > > Images? > > > > or thin images? something like that. The fact is that rotated/skewed > images > > > > will still be draw anti-aliased even if this bit is not set. > > > > > > Makes sense. Renamed accordingly. > > > > > > It also occurred to me that it might make sense to reverse the logic (and > > hence > > > settings) to always enable AA, but then selectively disable it for the > > > rectilinear cases (except the "hairline" case, which would then still have a > > > flag I suppose - so no major improvement in cleanliness.) If that sounds > > > reasonable, I'd be happy to do that change here or as a follow up CL. > > > > Ah, the reversed logic actually makes a lot of sense I think. And it will make > > this code less confusing. Then, the disabling of AA for axis-aligned rects > could > > be seen as an optimization option which could be set to include or exclude > > hairline rects. > > Yepp, that would be the interpretation seen from that "angle". Uploaded something like that (very similar to the prevous patchset.) PTAL.
Ping?
https://codereview.chromium.org/210043004/diff/480001/Source/platform/graphic... File Source/platform/graphics/GraphicsContext.h (right): https://codereview.chromium.org/210043004/diff/480001/Source/platform/graphic... Source/platform/graphics/GraphicsContext.h:139: // Disable the optimization that disables anti-aliasing for Double negation: Disabling the disabling is more confusing than necessary. Also, there should be some warning that determining whether an image is hairline is only reliable when the device pixel scale is fixed (e.g. when drawing to an ImageBuffer). Therefore, this option should not be used on GraphicsContexts that are backed by a recording canvas. You could even add a safety check: ASSERT(!isRecording());
https://codereview.chromium.org/210043004/diff/480001/Source/platform/graphic... File Source/platform/graphics/GraphicsContext.h (right): https://codereview.chromium.org/210043004/diff/480001/Source/platform/graphic... Source/platform/graphics/GraphicsContext.h:139: // Disable the optimization that disables anti-aliasing for On 2014/04/01 17:02:01, junov wrote: > Double negation: Disabling the disabling is more confusing than necessary. Made this read more like the name of the method. > Also, there should be some warning that determining whether an image is hairline > is only reliable when the device pixel scale is fixed (e.g. when drawing to an > ImageBuffer). Therefore, this option should not be used on GraphicsContexts > that are backed by a recording canvas. You could even add a safety check: > ASSERT(!isRecording()); Added a "Note" and the ASSERT.
On 2014/04/02 08:31:37, fs wrote: > https://codereview.chromium.org/210043004/diff/480001/Source/platform/graphic... > File Source/platform/graphics/GraphicsContext.h (right): > > https://codereview.chromium.org/210043004/diff/480001/Source/platform/graphic... > Source/platform/graphics/GraphicsContext.h:139: // Disable the optimization that > disables anti-aliasing for > On 2014/04/01 17:02:01, junov wrote: > > Double negation: Disabling the disabling is more confusing than necessary. > > Made this read more like the name of the method. > > > Also, there should be some warning that determining whether an image is > hairline > > is only reliable when the device pixel scale is fixed (e.g. when drawing to an > > ImageBuffer). Therefore, this option should not be used on GraphicsContexts > > that are backed by a recording canvas. You could even add a safety check: > > ASSERT(!isRecording()); > > Added a "Note" and the ASSERT. lgtm
The CQ bit was checked by fs@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fs@opera.com/210043004/500001
Message was sent while issue was closed.
Change committed as 170671 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
