Bug 58495 – Change useLowQualityScale boolean in GraphicsContext to be an enum

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: WebCore Misc.

[Mike Lawther]

In https://bugs.webkit.org/show_bug.cgi?id=56627, jamesr said: "As for useLowQualityScale, I think a better option would be to have an enum with the default value being "auto" to indicate that the paint() implementation is supposed to figure out the scaling algorithm to use and all non-default values will explicitly specify a scale (either nearestneighbor/bilinear/awesome or some other axis)."

Enums are more Webkitty than bools

[Mike Lawther]

Created attachment 89540 [details]
Patch

[Mike Lawther]

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.

[Collabora GTK+ EWS bot]

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

[Stephen White]

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?

[Mike Lawther]

@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?

[Eric Seidel (no email)]

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.

[Mike Lawther]

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?

[Kimmo Kinnunen]

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