Bug 97991 - image-rendering: -webkit-optimize-contrast not working for background images
Summary: image-rendering: -webkit-optimize-contrast not working for background images
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Simon Fraser (smfr)
URL: http://cavestory.org/irtest.php
Keywords: InRadar
Depends on: 153556
  Show dependency treegraph
Reported: 2012-09-30 21:44 PDT by realmmaster26
Modified: 2016-05-07 22:26 PDT (History)
17 users (show)

See Also:

Patch (40.86 KB, patch)
2016-01-27 19:57 PST, Simon Fraser (smfr)
darin: review+
Details | Formatted Diff | Diff
Patch (42.89 KB, patch)
2016-01-29 13:54 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description realmmaster26 2012-09-30 21:44:32 PDT
The css property image-rendering:-webkit-optimize-contrast works on img elements but fails to work on background images when using either the browser's zoom feature or the background-size property.

See an example of the issue here:

Tested in r129988.
Comment 1 Simon Fraser (smfr) 2012-10-01 13:08:48 PDT
I don't think we ever implemented it for CSS images.
Comment 2 Tab Atkins 2012-10-01 15:37:35 PDT
According to what I've specced <http://dev.w3.org/csswg/css4-images/#the-image-rendering>, it should apply to all images on that element, both content and CSS.
Comment 3 realmmaster26 2012-10-01 16:29:05 PDT
Webkit's image-rendering was based on the image-rendering property shown in this draft:

It specified:
When specified on an element, it applies to all images given in properties for the element, such as background images, list-style images, or the content of replaced elements when they represent an image that must be scaled.

So even back then it specified background images, and as Tab mentioned this is the same case with the current draft. Webkit's is the only implementation that is broken in this regard.
Comment 4 Radar WebKit Bug Importer 2016-01-27 09:50:12 PST
Comment 5 Simon Fraser (smfr) 2016-01-27 11:42:35 PST
This is a bit of a mess.

WebKit supports:
auto, optimizeSpeed, optimizeQuality, -webkit-optimize-contrast, -webkit-crisp-edges

We'll continue to parse, but deprecate all of these, and I'll add support for:
crisp-edges, pixelated
optimizeSpeed will be treated like pixelated
optimizeQuality will be treated like auto

Note that -webkit-optimize-contrast has no unprefixed version.
Comment 6 Simon Fraser (smfr) 2016-01-27 19:57:56 PST
Created attachment 270079 [details]
Comment 7 Darin Adler 2016-01-28 08:55:17 PST
Comment on attachment 270079 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=270079&action=review

Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:217:6: error: prototype for 'void WebCore::ImageBuffer::drawConsuming(std::unique_ptr<WebCore::ImageBuffer>, WebCore::GraphicsContext&, const WebCore::FloatRect&, const WebCore::FloatRect&, WebCore::CompositeOperator, WebCore::BlendMode, bool)' does not match any in class 'WebCore::ImageBuffer'
Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:222:6: error: prototype for 'void WebCore::ImageBuffer::draw(WebCore::GraphicsContext&, const WebCore::FloatRect&, const WebCore::FloatRect&, WebCore::CompositeOperator, WebCore::BlendMode, bool)' does not match any in class 'WebCore::ImageBuffer'

Error above indicate the Cairo code is still trying to pass the "useLowQualityScale" boolean.

C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\graphics\cg\ImageBufferCG.cpp(286): error C2065: 'useLowQualityScale': undeclared identifier [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]

Above is something similar on Windows.

> Source/WebCore/platform/graphics/GraphicsContext.h:680
> +class InterpolationQualityMaintainer {
> +public:
> +    explicit InterpolationQualityMaintainer(GraphicsContext& graphicsContext, InterpolationQuality interpolationQualityToUse)
> +        : m_graphicsContext(graphicsContext)
> +        , m_currentInterpolationQuality(graphicsContext.imageInterpolationQuality())
> +        , m_interpolationQualityChanged(interpolationQualityToUse != InterpolationDefault && m_currentInterpolationQuality != interpolationQualityToUse)
> +    {
> +        if (m_interpolationQualityChanged)
> +            m_graphicsContext.setImageInterpolationQuality(interpolationQualityToUse);
> +    }
> +
> +    explicit InterpolationQualityMaintainer(GraphicsContext& graphicsContext, Optional<InterpolationQuality> interpolationQuality)
> +        : InterpolationQualityMaintainer(graphicsContext, interpolationQuality ? interpolationQuality.value() : graphicsContext.imageInterpolationQuality())
> +    {
> +    }
> +
> +    ~InterpolationQualityMaintainer()
> +    {
> +        if (m_interpolationQualityChanged)
> +            m_graphicsContext.setImageInterpolationQuality(m_currentInterpolationQuality);
> +    }
> +
> +private:
> +    GraphicsContext& m_graphicsContext;
> +    InterpolationQuality m_currentInterpolationQuality;
> +    bool m_interpolationQualityChanged;
> +};

Could make this class less wordy and easier to read by calling it context instead of graphicsContext (because inside this class it’s clear that it’s a graphics context) and quality instead of interpolationQuality (because inside this class it’s clear that it’s interpolation quality).

> Source/WebCore/rendering/ImageQualityController.cpp:120
> +    // If the image is not a bitmap image, then none of this is relevant and we just paint at high quality.
> +    if (!(image.isBitmapImage() || image.isPDFDocumentImage()) || context.paintingDisabled())
> +        return InterpolationDefault;

Comment says "not a bitmap image", code says "not a bitmap image or PDF document". That’s an annoying difference that would be easier to understand with a more precise comment that focuses a bit more on why rather than what the code does, since we can read the code.

> Source/WebCore/rendering/ImageQualityController.cpp:123
> +    if (Optional<InterpolationQuality> styleInterpolation = interpolationQualityFromStyle(object->style()))
> +        return styleInterpolation.value();

I think this would read better with auto instead of the long type name.

> Source/WebCore/rendering/ImageQualityController.h:41
>  class RenderView;
> +class RenderStyle;

Should be sorted alphabetically.

> Source/WebCore/rendering/ImageQualityController.h:49
> +    InterpolationQuality chooseInterpolationQuality(GraphicsContext&, RenderBoxModelObject*, Image&, const void* layer, const LayoutSize&);

Why RenderBoxModelObject* instead of RenderBoxModelObject&?

Is it really acceptable to have the layer argument type be const void*!?

> Source/WebCore/rendering/RenderImage.cpp:551
> +    // FIXME: Document when image != img.get().

I don’t understand this comment. Not clear what “document” means in this context, or what action someone is supposed to take. Maybe you can reword it so others will understand the meaning?
Comment 8 Simon Fraser (smfr) 2016-01-28 22:35:29 PST
Yes, I need to fix the non-cocoa platforms.
Comment 9 Simon Fraser (smfr) 2016-01-29 13:54:22 PST
Created attachment 270251 [details]
Comment 10 WebKit Commit Bot 2016-01-29 15:16:06 PST
Comment on attachment 270251 [details]

Clearing flags on attachment: 270251

Committed r195848: <http://trac.webkit.org/changeset/195848>
Comment 11 icherevkov 2016-05-07 16:32:27 PDT
Not sure whether it's fixed, but it still not working for me in Safari 9.0.3 (latest) and  I ve registered here specifically to let you know
Comment 12 Simon Fraser (smfr) 2016-05-07 22:26:20 PDT
Please try Safari Technology Preview or a WebKit nightly build.