Bug 97991 – image-rendering: -webkit-optimize-contrast not working for background images

• Component: CSS
• Status: RESOLVED FIXED

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:
http://cavestory.org/irtest.php

Tested in r129988.

[Simon Fraser (smfr)]

I don't think we ever implemented it for CSS images.

[Tab Atkins]

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.

Webkit's image-rendering was based on the image-rendering property shown in this draft:
http://www.w3.org/TR/2011/WD-css3-images-20110217/#image-rendering

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.

[Radar WebKit Bug Importer]

<rdar://problem/24370136>

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

Created attachment 270079 [details]
Patch

[Darin Adler]

Comment on attachment 270079 [details]
Patch

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?

[Simon Fraser (smfr)]

Yes, I need to fix the non-cocoa platforms.

[Simon Fraser (smfr)]

Created attachment 270251 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 270251 [details]
Patch

Clearing flags on attachment: 270251

Committed r195848: <http://trac.webkit.org/changeset/195848>

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

[Simon Fraser (smfr)]

Please try Safari Technology Preview or a WebKit nightly build.