Bug 58495 - Change useLowQualityScale boolean in GraphicsContext to be an enum
Summary: Change useLowQualityScale boolean in GraphicsContext to be an enum
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Mike Lawther
URL:
Keywords:
Depends on:
Blocks: 56627
  Show dependency treegraph
 
Reported: 2011-04-13 16:58 PDT by Mike Lawther
Modified: 2011-07-28 15:05 PDT (History)
6 users (show)

See Also:


Attachments
Patch (26.41 KB, patch)
2011-04-13 23:50 PDT, Mike Lawther
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Lawther 2011-04-13 16:58:33 PDT
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
Comment 1 Mike Lawther 2011-04-13 23:50:01 PDT
Created attachment 89540 [details]
Patch
Comment 2 Mike Lawther 2011-04-13 23:58:07 PDT
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.
Comment 3 Collabora GTK+ EWS bot 2011-04-14 07:38:43 PDT
Attachment 89540 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8402585
Comment 4 Stephen White 2011-04-14 07:52:47 PDT
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?
Comment 5 Mike Lawther 2011-04-14 23:22:47 PDT
@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 6 Eric Seidel (no email) 2011-04-26 16:37:27 PDT
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.
Comment 7 Mike Lawther 2011-04-28 06:20:03 PDT
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?