Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1236363003:
Drop WebCoreInterpolationQualityToSkFilterQuality (Closed)
|
Created:
5 years, 5 months ago by fs Modified:
5 years, 5 months ago CC:
blink-reviews, krit, drott+blinkwatch_chromium.org, Rik, dshwang, jbroman, Justin Novosad, danakj, pdr+graphicswatchlist_chromium.org, Stephen Chennney, rwlbuis, reed1 Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionDrop WebCoreInterpolationQualityToSkFilterQuality
Except for GraphicsContextState, this function is only used in one
location - SVGPaintContext. In that one instance it's hard-wired to 'low',
so make that even more obvious.
Move the function from SkiaUtils.h to GraphicsContextState.cpp and rename.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199092
Patch Set 1 #Patch Set 2 : Drop instead. #
Total comments: 2
Patch Set 3 : Space #
Messages
Total messages: 18 (4 generated)
fs@opera.com changed reviewers: + fmalita@chromium.org, senorblanco@chromium.org
While looking into the various interpolation tweaks I noticed this one, and subsequently had to see what breakage there would be with it "fixed". Not a lot it turns out - some minor (indiscernible) changes (probably due to the wider filter support of the higher quality filters I'm guessing?) This one is clearly worse though: https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... looks like a Skia issue to me though.
On 2015/07/15 16:16:03, fs wrote: > While looking into the various interpolation tweaks I noticed this one, and > subsequently had to see what breakage there would be with it "fixed". > Not a lot it turns out - some minor (indiscernible) changes (probably due to the > wider filter support of the higher quality filters I'm guessing?) > > This one is clearly worse though: > > https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... > > looks like a Skia issue to me though. I'm guessing that's because the higher-quality filters don't support shear or rotation matrices. It looks like it's dropping to nearest-neighbour in that case, though, which seems unfortunate. We'd definitely have to fix that in Skia before switching this over (by having it drop to linear). There will also likely be a performance penalty for using higher quality in these cases. Note that <canvas> also only supports either Low or None, so there's some precedent for what SVG patterns and whatever else is using this function is doing.
On 2015/07/15 16:48:28, Stephen White wrote: > On 2015/07/15 16:16:03, fs wrote: > > While looking into the various interpolation tweaks I noticed this one, and > > subsequently had to see what breakage there would be with it "fixed". > > Not a lot it turns out - some minor (indiscernible) changes (probably due to > the > > wider filter support of the higher quality filters I'm guessing?) > > > > This one is clearly worse though: > > > > > https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... > > > > looks like a Skia issue to me though. > > I'm guessing that's because the higher-quality filters don't support shear or > rotation matrices. It looks like it's dropping to nearest-neighbour in that > case, though, which seems > unfortunate. We'd definitely have to fix that in Skia before switching this over > (by having it drop to linear). Not sure it's strictly tied to shear/rotation - some of the other differences use a similar pattern transform (skew, made popular by the SVG W3C testsuite...) and didn't show this effect. Maybe "luck" (or lack thereof) or some very minor difference. > There will also likely be a performance penalty > for using higher quality in these cases. Quite possibly. For almost all drawImage calls though, this function isn't used, so computeFilterQuality - which uses GC::imageInterpolationQuality() - will often return "High" because essentially all bitmaps are of the immutable kind these days, and the default interpolation for non-Android is "High". So arguably the penalty is there already for the most part (SVG, <canvas> and possibly something else excluded it seems.) The good news though is that removing the "none" -> "low" coercion should pose less of the problem (because of the "essentially all bitmaps are of the immutable" mostly I suspect.) > Note that <canvas> also only supports either Low or None, so there's some > precedent for what SVG patterns and whatever else is using this function is > doing. All the SVG differences is actually attempting to use InterpolationDefault (+ this function to map to SkFilterQuality directly), so they could quite trivially be coerced into something more restricted if we see fit. (I actually wondering if that isn't what we want...) Florin, any opinion on that?
reed@google.com changed reviewers: + reed@google.com
FYI : skia mostly respects HQ for all transformations. If there is rotation/skew, we decompose the matrix into a Scale + Remainder, and apply HQ using the Scale, and then apply the remainder w/ bilerp.
On 2015/07/15 17:07:11, fs wrote: > On 2015/07/15 16:48:28, Stephen White wrote: > > On 2015/07/15 16:16:03, fs wrote: > > > While looking into the various interpolation tweaks I noticed this one, and > > > subsequently had to see what breakage there would be with it "fixed". > > > Not a lot it turns out - some minor (indiscernible) changes (probably due to > > the > > > wider filter support of the higher quality filters I'm guessing?) > > > > > > This one is clearly worse though: > > > > > > > > > https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... > > > > > > looks like a Skia issue to me though. > > > > I'm guessing that's because the higher-quality filters don't support shear or > > rotation matrices. It looks like it's dropping to nearest-neighbour in that > > case, though, which seems > > unfortunate. We'd definitely have to fix that in Skia before switching this > over > > (by having it drop to linear). > > Not sure it's strictly tied to shear/rotation - some of the other differences > use a similar pattern transform (skew, made popular by the SVG W3C testsuite...) > and didn't show this effect. Maybe "luck" (or lack thereof) or some very minor > difference. > > > There will also likely be a performance penalty > > for using higher quality in these cases. > > Quite possibly. For almost all drawImage calls though, this function isn't used, > so computeFilterQuality - which uses GC::imageInterpolationQuality() - will > often return "High" because essentially all bitmaps are of the immutable kind > these days, and the default interpolation for non-Android is "High". So arguably > the penalty is there already for the most part (SVG, <canvas> and possibly > something else excluded it seems.) The good news though is that removing the > "none" -> "low" coercion should pose less of the problem (because of the > "essentially all bitmaps are of the immutable" mostly I suspect.) I was just trying to find what (if anything) is still depending on GC imageInterpolationQuality(), and couldn't find much. As you pointed out, all image related draw call ignore this GC state and compute a different filter level, so all setters seem to be wasting their time. Although, since we use this filter level for all other Skia draw call paints, it's still possible it may affect the results when using shaders (patterns?). Not sure if that's a problem, I would think not. So I guess I'm trying to figure whether we can just get rid of GC::imageInterpolationQuality(). I can take a stab myself if y'all don't know any immediate roadblocks. > > Note that <canvas> also only supports either Low or None, so there's some > > precedent for what SVG patterns and whatever else is using this function is > > doing. > > All the SVG differences is actually attempting to use InterpolationDefault (+ > this function to map to SkFilterQuality directly), so they could quite trivially > be coerced into something more restricted if we see fit. (I actually wondering > if that isn't what we want...) Florin, any opinion on that? What happens if we request InterpolationNone in SVGPaintContext::paintForLayoutObject (previous coerced behavior)? I'm hoping all the diffs go away?
On 2015/07/16 13:44:20, f(malita) wrote: > On 2015/07/15 17:07:11, fs wrote: > > On 2015/07/15 16:48:28, Stephen White wrote: > > > On 2015/07/15 16:16:03, fs wrote: > > > > While looking into the various interpolation tweaks I noticed this one, > and > > > > subsequently had to see what breakage there would be with it "fixed". > > > > Not a lot it turns out - some minor (indiscernible) changes (probably due > to > > > the > > > > wider filter support of the higher quality filters I'm guessing?) > > > > > > > > This one is clearly worse though: > > > > > > > > > > > > > > https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... > > > > > > > > looks like a Skia issue to me though. > > > > > > I'm guessing that's because the higher-quality filters don't support shear > or > > > rotation matrices. It looks like it's dropping to nearest-neighbour in that > > > case, though, which seems > > > unfortunate. We'd definitely have to fix that in Skia before switching this > > over > > > (by having it drop to linear). > > > > Not sure it's strictly tied to shear/rotation - some of the other differences > > use a similar pattern transform (skew, made popular by the SVG W3C > testsuite...) > > and didn't show this effect. Maybe "luck" (or lack thereof) or some very minor > > difference. > > > > > There will also likely be a performance penalty > > > for using higher quality in these cases. > > > > Quite possibly. For almost all drawImage calls though, this function isn't > used, > > so computeFilterQuality - which uses GC::imageInterpolationQuality() - will > > often return "High" because essentially all bitmaps are of the immutable kind > > these days, and the default interpolation for non-Android is "High". So > arguably > > the penalty is there already for the most part (SVG, <canvas> and possibly > > something else excluded it seems.) The good news though is that removing the > > "none" -> "low" coercion should pose less of the problem (because of the > > "essentially all bitmaps are of the immutable" mostly I suspect.) > > I was just trying to find what (if anything) is still depending on GC > imageInterpolationQuality(), and couldn't find much. As you pointed out, all > image related draw call ignore this GC state and compute a different filter > level, so all setters seem to be wasting their time. The only user of imageInterpolationQuality (that _I_ know of...) is limitInterpolationQuality in SkiaUtils.cpp (yeah, good job whoever did the hiding ,-)) > Although, since we use this filter level for all other Skia draw call paints, > it's still possible it may affect the results when using shaders (patterns?). > Not sure if that's a problem, I would think not. Yeah... Patterns aren't used a lot though - I even dropped the RefPtr from GCState if you remember. They're used in some GeneratedImage code-paths and SVG I think - the latter explicitly sets filter quality though, so not affected. Suspect the GCState SkPaint filter qualities could be hard-wired to either of the two current values and be fine in general. Would like to revisit the usage within the Image hierarchy - which should probably use the "image" (unclamped) value. > So I guess I'm trying to figure whether we can just get rid of > GC::imageInterpolationQuality(). I can take a stab myself if y'all don't know > any immediate roadblocks. I wouldn't think so - likely to pass CQ though since I suspect only Android would be affected (since they has 'low' as InterpolationDefault IIRC.) > > > Note that <canvas> also only supports either Low or None, so there's some > > > precedent for what SVG patterns and whatever else is using this function is > > > doing. > > > > All the SVG differences is actually attempting to use InterpolationDefault (+ > > this function to map to SkFilterQuality directly), so they could quite > trivially > > be coerced into something more restricted if we see fit. (I actually wondering > > if that isn't what we want...) Florin, any opinion on that? > > What happens if we request InterpolationNone in > SVGPaintContext::paintForLayoutObject (previous coerced behavior)? I'm hoping > all the diffs go away? Did you mean InterpolationLow (which is what we'll always request there ATM.) just prior to this comment I uploaded a new version of the CL, that instead drops said functions and does the suggested hard-coding, so the answer to that should be forthcoming...
On 2015/07/16 14:16:44, fs wrote: > Did you mean InterpolationLow (which is what we'll always request there ATM.) > just prior to this comment I uploaded a new version of the CL, that instead > drops said functions and does the suggested hard-coding, so the answer to that > should be forthcoming... Yup, meant InterpolationLow, thanks!
On 2015/07/16 14:19:47, f(malita) wrote: > On 2015/07/16 14:16:44, fs wrote: > > Did you mean InterpolationLow (which is what we'll always request there ATM.) > > just prior to this comment I uploaded a new version of the CL, that instead > > drops said functions and does the suggested hard-coding, so the answer to that > > should be forthcoming... > > Yup, meant InterpolationLow, thanks! It appears green (at least the Mac bot, and the Linux one looked like a flake.)
LGTM
On 2015/07/15 17:10:00, reed1 wrote: > FYI : skia mostly respects HQ for all transformations. If there is > rotation/skew, we decompose the matrix into a Scale + Remainder, and apply HQ > using the Scale, and then apply the remainder w/ bilerp. Interesting. I wonder why the high quality version of https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... looks so much worse. Note the patterned sample on the left, third from the top: the blue/yellow transitions have much worse jaggies. Maybe something about the tiling seams? Not a big deal, since fs@ has restored the old behaviour in his new patch, I'm just curious.
lgtm https://codereview.chromium.org/1236363003/diff/20001/Source/platform/graphic... File Source/platform/graphics/GraphicsContextState.cpp (right): https://codereview.chromium.org/1236363003/diff/20001/Source/platform/graphic... Source/platform/graphics/GraphicsContextState.cpp:12: // The filter quality "selected" here will primarily be used when paintinga nit: paintinga spacing
https://codereview.chromium.org/1236363003/diff/20001/Source/platform/graphic... File Source/platform/graphics/GraphicsContextState.cpp (right): https://codereview.chromium.org/1236363003/diff/20001/Source/platform/graphic... Source/platform/graphics/GraphicsContextState.cpp:12: // The filter quality "selected" here will primarily be used when paintinga On 2015/07/16 15:33:27, f(malita) wrote: > nit: paintinga spacing Done.
The CQ bit was checked by fs@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from senorblanco@chromium.org, fmalita@chromium.org Link to the patchset: https://codereview.chromium.org/1236363003/#ps40001 (title: "Space")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1236363003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=199092 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
