Bug 58495

Created attachment 89540 [details] Patch

This is a pretty straightforward replacing of the bool with an enum. drawTiledImage and drawImage used to do different things if useLowQualityScaling was true, but they now both use nearestNeighbor (ie InterpolationQualityNone). I ran all of new-run-webkit-tests, and no pixel tests failed. I was unsure if this needed to be plumbed deeper at this stage. If we ever want to implement eg EPX scaling for image-rendering: optimize-contrast, then it definitely will need to go deeper. But for now, ScalingNearestNeighbor<=>InterpolationNone.

Attachment 89540 [details] did not build on gtk: Build output: http://queues.webkit.org/results/8402585

Hmm. Note that there already was an "InterpolationQuality" enum (which I believe the bool would override if set). This was somewhat confusing, but having two enums is also confusing. Is there some way we could clean this up?

@senorblanco: I completely agree. Having a 'quality' and an 'algorithm' axis implies a two dimensional space where we have this weird thing of what is a lowQuality/NearestNeighbor and a highQuality/NearestNeighbor. The InterpolationQuality stuff seems inherited from CG - there is a 1:1 mapping to CGInterpolationQuality. One possibility is extending this enum with specific algorithms so as to have (strawman names - I don't really like 'strategy', but 'quality' doesn't cover it anymore): enum InterpolationStrategy { Default, None, Low, Medium, High, NearestNeighbor, Bilinear, Bicubic, Lanczos }; Passing 'Default' into drawImage and friends (replacing the useLowQualityScaling param) means 'use whatever strategy has been set with setInterpolationStrategy()', and anything else will use that strategy for that call (same as useLowQualityScaling does now). Platforms can then choose to map any unsupported strategies to whatever they deem best (eg lanczos->bicubic). This makes me a little uncomfortable, but no more so than a platform having to decide what 'medium' means. What do you guys reckon?

Comment on attachment 89540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89540&action=review Need help understanding the low vs. none thing, otherwise this looks fine! > Source/WebCore/platform/graphics/GraphicsContext.cpp:479 > - setImageInterpolationQuality(InterpolationLow); > + setImageInterpolationQuality(InterpolationNone); Why? > Source/WebCore/platform/graphics/GraphicsContext.cpp:499 > - setImageInterpolationQuality(InterpolationLow); > + setImageInterpolationQuality(InterpolationNone); Why? > Source/WebCore/rendering/RenderBoxModelObject.cpp:756 > + ScalingAlgorithm scaleAlgorithm = > + shouldPaintAtLowQuality(context, image.get(), bgLayer, tileSize) ? ScalingNearestNeighbor : ScalingAuto; We dont' really have a collumn limit for webkit.

Hi Eric - thanks for the review! Basically None means nearest-neighbour, and Low means bilinear. The original useLowQualityScale boolean meant different things in different places (None or Low), so I normalised them to NearestNeighbour (ie quality=None). This does change the behaviour, but as I mentioned in comment #2, no tests broke. I think senorblanco has a point re this being confusing - what do you think about the suggestion in comment #5?

Closing this as configuration changed since the problem at hand, sueLowQualityScale seems to have moved to typed enum bitfield ImagePaintingOptions