Respect image-rendering setting for determing image-rendering quality

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Allan Sandfeld Jensen]

WebKit has been parsing -webkit-image-rendering optimizeSpeed and optimizeQuality for a long time, but has never actually used them.

I propose letting optimizeSpeed select faster image scaling, currently this can be somewhat counterintuitively also be done using optimizeContrast, but we may want to add more filters to the latter later.

Also I want optimizeQuality to ensure lowQuality scaling is never used. Specifically this will make it possible to disable it during animations if the author desires so (bug 74600).

[Allan Sandfeld Jensen]

Created attachment 195309 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 195317 [details]
Patch

[Simon Fraser (smfr)]

The CSS property was landed in http://trac.webkit.org/changeset/87121, and ImageQualityController::shouldPaintAtLowQuality() consults object->style()->imageRendering() so the CSS property does do something.

[Allan Sandfeld Jensen]

(In reply to comment #3)
> The CSS property was landed in http://trac.webkit.org/changeset/87121, and ImageQualityController::shouldPaintAtLowQuality() consults object->style()->imageRendering() so the CSS property does do something.

Yes, but only for the value -webkit-optimize-contrast which enforces low quality scaling for the purpose of disabling blur.

[Simon Fraser (smfr)]

Comment on attachment 195317 [details]
Patch

http://dev.w3.org/csswg/css-images-4/#the-image-rendering says "This property previously accepted the values ‘optimizeSpeed’ and ‘optimizeQuality’. These are now deprecated; a user agent must accept them as valid values but must treat them as having the same behavior as ‘auto’, and authors must not use them." so r-

[Allan Sandfeld Jensen]

(In reply to comment #5)
> (From update of attachment 195317 [details])
> http://dev.w3.org/csswg/css-images-4/#the-image-rendering says "This property previously accepted the values ‘optimizeSpeed’ and ‘optimizeQuality’. These are now deprecated; a user agent must accept them as valid values but must treat them as having the same behavior as ‘auto’, and authors must not use them." so r-

I will rethink my approach and consult www-style. Thanks for link.

[Simon Fraser (smfr)]

I still think it's worth fixing this bug though. See
http://lists.w3.org/Archives/Public/www-style/2013Mar/0750.html

[Allan Sandfeld Jensen]

Created attachment 196478 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 196478 [details]
Patch

I think image-rendering needs to continue to accept optimizeSpeed/optimizeQuality for compatibility with SVG. optimizeQuality should map to the new -webkit-smooth value (the draft will be updated to reflect this at some point).

[Allan Sandfeld Jensen]

Created attachment 196487 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 196487 [details]
Patch

You should add a parsing test for the new values in image-rendering, and also fix getComputedStyle(). I think the latter should round-trip both old and new values, so I'm not sure that mapping ImageRenderingSmooth to CSSValueOptimizequality will work here.

[Allan Sandfeld Jensen]

(In reply to comment #11)
> (From update of attachment 196487 [details])
> You should add a parsing test for the new values in image-rendering, and also fix getComputedStyle(). I think the latter should round-trip both old and new values, so I'm not sure that mapping ImageRenderingSmooth to CSSValueOptimizequality will work here.

Right. I didn't see any need to the round-trip for the CSS value in HTML content, but of course it is needed for SVG content.

[Allan Sandfeld Jensen]

Created attachment 196603 [details]
Patch

[Allan Sandfeld Jensen]

Simon, any insight?

[Simon Fraser (smfr)]

Comment on attachment 196603 [details]
Patch

Ithink we might want something more fine-grained than ENABLE_CSS4_IMAGES, but we can tweak that later.

[Allan Sandfeld Jensen]

Committed r148208: <http://trac.webkit.org/changeset/148208>

[Benjamin Poulain]

Comment on attachment 196603 [details]
Patch

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

This is a weird way to have an Enable flag. If we were to remove the feature, instead of tracking CSS4_IMAGES, we would have to track all the values one by one.

That is not the way the other CSS properties are contained.

> Source/WebCore/css/CSSValueKeywords.in:929
> +crisp-edges
> +pixelated
> +-webkit-smooth

Why isn't this behind ENABLE_CSS4_IMAGES?

> Tools/Scripts/webkitperl/FeatureList.pm:203
> +    { option => "css4-images", desc => "Toggle CSS4 Images support",
> +      define => "ENABLE_CSS4_IMAGES", default => 0, value => \$css4ImagesSupport },
> +

Why aren't the Xcode project files updated accordingly?

[Allan Sandfeld Jensen]

(In reply to comment #17)
> (From update of attachment 196603 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196603&action=review
> 
> > Source/WebCore/css/CSSValueKeywords.in:929
> > +crisp-edges
> > +pixelated
> > +-webkit-smooth
> 
> Why isn't this behind ENABLE_CSS4_IMAGES?
> 
I only used the flag to decide if the values should be parsed or not. I prefer not to litter ifdefs all over the places when only one or two places are necessary to enable/disable the feature.

Are you concerned if we were to remove or rename stuff, that it would be hard to find all the affected areas of the code?

> > Tools/Scripts/webkitperl/FeatureList.pm:203
> > +    { option => "css4-images", desc => "Toggle CSS4 Images support",
> > +      define => "ENABLE_CSS4_IMAGES", default => 0, value => \$css4ImagesSupport },
> > +
> 
> Why aren't the Xcode project files updated accordingly?

I had no idea Xcode needed to be update when we add a new experimental feature. Isn't FeatureList.pm the central file for that?

[Benjamin Poulain]

> > Why isn't this behind ENABLE_CSS4_IMAGES?
> > 
> I only used the flag to decide if the values should be parsed or not. I prefer not to litter ifdefs all over the places when only one or two places are necessary to enable/disable the feature.
> 
> Are you concerned if we were to remove or rename stuff, that it would be hard to find all the affected areas of the code?

The rule of thumb is, when practical, if it is not enabled it should not be compiled.

If you want to have a glimpse of how not doing this causes problem, look at the world of pain of the chromium cleanup.

> > > Tools/Scripts/webkitperl/FeatureList.pm:203
> > > +    { option => "css4-images", desc => "Toggle CSS4 Images support",
> > > +      define => "ENABLE_CSS4_IMAGES", default => 0, value => \$css4ImagesSupport },
> > > +
> > 
> > Why aren't the Xcode project files updated accordingly?
> 
> I had no idea Xcode needed to be update when we add a new experimental feature. Isn't FeatureList.pm the central file for that?

Here is what you have to modify for a new ENABLE flag http://trac.webkit.org/wiki/AddingFeatures

[Allan Sandfeld Jensen]

Reopening. The CSS4 part was not fully introduces as a new feature, and I am actually opposed to introducing as an enablable feature since it is too early in the standardization. This means it should not have been included at all.

[Allan Sandfeld Jensen]

Mozilla and old Opera have the feature enabled with vendor-prefix. I still strongly dislike the disparity between SVG CSS/attributes and HTML CSS.
https://developer.mozilla.org/en-US/docs/CSS/image-rendering
https://developer.mozilla.org/en-US/docs/SVG/Attribute/image-rendering

[Allan Sandfeld Jensen]

Created attachment 198489 [details]
Patch

Disabled the CSS4 values again. They are not ready for implementation.

[Simon Fraser (smfr)]

Given that the moz docs say:

                   image-rendering: -moz-crisp-edges;         /* Firefox */
                   image-rendering:   -o-crisp-edges;         /* Opera */
                   image-rendering: -webkit-optimize-contrast;/* Webkit (non-standard naming) */

I think it would be preferable to keep the new code, but use -webkit prefixes.

[Allan Sandfeld Jensen]

(In reply to comment #23)
> Given that the moz docs say:
> 
>                    image-rendering: -moz-crisp-edges;         /* Firefox */
>                    image-rendering:   -o-crisp-edges;         /* Opera */
>                    image-rendering: -webkit-optimize-contrast;/* Webkit (non-standard naming) */
> 
> I think it would be preferable to keep the new code, but use -webkit prefixes.

The latest patch keeps the -webkit-crisp-edges value, but removes -webkit-smooth and pixelated values. They provided nothing yet that couldn't be done using optimizeQuality of -webkit-crisp-edges.

[Benjamin Poulain]

Comment on attachment 198489 [details]
Patch

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

I like the new direction this is taking.

Some comments bellow:

> Source/WebCore/rendering/style/RenderStyleConstants.h:484
>      ImageRenderingAuto, ImageRenderingOptimizeSpeed, ImageRenderingOptimizeQuality,
> -    ImageRenderingCrispEdges, ImageRenderingPixelated, ImageRenderingSmooth
> +    ImageRenderingCrispEdges

I think it would be better to either split the enum on one value per line, or put them all on the same line.

> Source/WebCore/rendering/style/RenderStyleConstants.h:485
> +    // For CSS4 Images we will also need ImageRenderingPixelated and ImageRenderingSmooth

I think we should not have this comment. There is no way to keep those kind of comments in sync with spec updates.

> LayoutTests/ChangeLog:12
> +        * fast/css/image-rendering-pre-css4.html: Added.

The name of the test is not good enough in my opinion.

I think you should also have the following coverage:
-Tests for CSSOM. Both checking the style and computed style when using image-rendering.
-A ref test verifying "-webkit-crisp-edges" and "-webkit-optimize-contrast" render the exact same thing.

Look for the current coverage of image-rendering, as it should be a good start.

[Allan Sandfeld Jensen]

Created attachment 198837 [details]
Patch

[Build Bot]

Comment on attachment 198837 [details]
Patch

Attachment 198837 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/25231

New failing tests:
fast/css/image-rendering.html

[Build Bot]

Created attachment 198844 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Benjamin Poulain]

Comment on attachment 198837 [details]
Patch

So you replaced the coverage of -webkit-optimize-contrast by -webkit-crisp-edges then verify they are equivalent with a ref-test? Make sense.

Can you please add one more? A text test checking the style and computed style for images with -webkit-optimize-contrast, -webkit-crisp-edges, and the default value of image-rendering? With js-test-pre, you can do stuff like shouldBeEqualToString('element.imageRender', '-webkit-crip-edges').

[Allan Sandfeld Jensen]

(In reply to comment #29)
> (From update of attachment 198837 [details])
> So you replaced the coverage of -webkit-optimize-contrast by -webkit-crisp-edges then verify they are equivalent with a ref-test? Make sense.
> 
> Can you please add one more? A text test checking the style and computed style for images with -webkit-optimize-contrast, -webkit-crisp-edges, and the default value of image-rendering? With js-test-pre, you can do stuff like shouldBeEqualToString('element.imageRender', '-webkit-crip-edges').

Note that the improved optimize-contrast-image.html test also tests the computed value of -webkit-optimize-contrast, and specifically that it gets the same computed value as -webkit-crisp-edges..

If anything is missing it would be that optimizeSpeed and optimizeQuality are preserved and not converted to auto.  I could make image-rendering.html print the value of getComputedStyle above each image instead of using a fixed title.

[Benjamin Poulain]

(In reply to comment #30)
> Note that the improved optimize-contrast-image.html test also tests the computed value of -webkit-optimize-contrast, and specifically that it gets the same computed value as -webkit-crisp-edges..

Yep, I have noticed, I think it is great.

> If anything is missing it would be that optimizeSpeed and optimizeQuality are preserved and not converted to auto.  I could make image-rendering.html print the value of getComputedStyle above each image instead of using a fixed title.

Pixel tests and text tests have different values:
Pixel tests are not run on EWS. Pixel test are also basically only maintained for Mac.
Good text tests are easy to interpret when they are failing. They are run by all the ports.

I think having additional text tests is desirable here.

[Allan Sandfeld Jensen]

(In reply to comment #31)
> (In reply to comment #30)
> > Note that the improved optimize-contrast-image.html test also tests the computed value of -webkit-optimize-contrast, and specifically that it gets the same computed value as -webkit-crisp-edges..
> 
> Yep, I have noticed, I think it is great.
> 
> > If anything is missing it would be that optimizeSpeed and optimizeQuality are preserved and not converted to auto.  I could make image-rendering.html print the value of getComputedStyle above each image instead of using a fixed title.
> 
> Pixel tests and text tests have different values:
> Pixel tests are not run on EWS. Pixel test are also basically only maintained for Mac.
> Good text tests are easy to interpret when they are failing. They are run by all the ports.
> 
> I think having additional text tests is desirable here.

Pixel tests? I think we call them layout tests, and they also have a text expectation even without being in dumpAsText mode.

[Allan Sandfeld Jensen]

Created attachment 199028 [details]
Patch

Added JS test of image-rendering values

[Allan Sandfeld Jensen]

Created attachment 199029 [details]
Patch

Removed accidentially uploaded difference

[Build Bot]

Comment on attachment 199029 [details]
Patch

Attachment 199029 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/81694

New failing tests:
fast/css/image-rendering.html

[Build Bot]

Created attachment 199041 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Benjamin Poulain]

Comment on attachment 199029 [details]
Patch

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

Great new tests, thanks for updating.

> LayoutTests/ChangeLog:12
> +        Baselines for fast/css/image-rendering.html will be uploaded later when the bots have generated them.

Can't you add the results provided by EWS?

[Allan Sandfeld Jensen]

Committed r148949: <http://trac.webkit.org/changeset/148949>