Bug 113405 - Respect image-rendering setting for determing image-rendering quality
Summary: Respect image-rendering setting for determing image-rendering quality
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords: WebExposed
Depends on:
Blocks: 74600
  Show dependency treegraph
 
Reported: 2013-03-27 07:06 PDT by Allan Sandfeld Jensen
Modified: 2013-04-23 01:16 PDT (History)
13 users (show)

See Also:


Attachments
Patch (2.81 KB, patch)
2013-03-27 07:10 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (3.89 KB, patch)
2013-03-27 07:30 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (9.53 KB, patch)
2013-04-04 08:04 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (9.82 KB, patch)
2013-04-04 10:01 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (10.11 KB, patch)
2013-04-05 02:29 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (11.64 KB, patch)
2013-04-17 02:54 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (27.26 KB, patch)
2013-04-19 03:14 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (868.74 KB, application/zip)
2013-04-19 05:07 PDT, Build Bot
no flags Details
Patch (31.64 KB, patch)
2013-04-22 08:02 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (31.03 KB, patch)
2013-04-22 08:05 PDT, Allan Sandfeld Jensen
benjamin: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (547.19 KB, application/zip)
2013-04-22 10:10 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Allan Sandfeld Jensen 2013-03-27 07:06:01 PDT
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).
Comment 1 Allan Sandfeld Jensen 2013-03-27 07:10:45 PDT
Created attachment 195309 [details]
Patch
Comment 2 Allan Sandfeld Jensen 2013-03-27 07:30:45 PDT
Created attachment 195317 [details]
Patch
Comment 3 Simon Fraser (smfr) 2013-03-28 11:11:59 PDT
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.
Comment 4 Allan Sandfeld Jensen 2013-03-28 11:20:49 PDT
(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.
Comment 5 Simon Fraser (smfr) 2013-03-29 14:52:38 PDT
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-
Comment 6 Allan Sandfeld Jensen 2013-03-30 05:27:49 PDT
(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.
Comment 7 Simon Fraser (smfr) 2013-03-30 09:12:03 PDT
I still think it's worth fixing this bug though. See
http://lists.w3.org/Archives/Public/www-style/2013Mar/0750.html
Comment 8 Allan Sandfeld Jensen 2013-04-04 08:04:55 PDT
Created attachment 196478 [details]
Patch
Comment 9 Simon Fraser (smfr) 2013-04-04 08:34:19 PDT
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).
Comment 10 Allan Sandfeld Jensen 2013-04-04 10:01:55 PDT
Created attachment 196487 [details]
Patch
Comment 11 Simon Fraser (smfr) 2013-04-04 10:09:50 PDT
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.
Comment 12 Allan Sandfeld Jensen 2013-04-05 01:20:21 PDT
(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.
Comment 13 Allan Sandfeld Jensen 2013-04-05 02:29:34 PDT
Created attachment 196603 [details]
Patch
Comment 14 Allan Sandfeld Jensen 2013-04-11 02:17:39 PDT
Simon, any insight?
Comment 15 Simon Fraser (smfr) 2013-04-11 09:05:34 PDT
Comment on attachment 196603 [details]
Patch

Ithink we might want something more fine-grained than ENABLE_CSS4_IMAGES, but we can tweak that later.
Comment 16 Allan Sandfeld Jensen 2013-04-11 09:18:25 PDT
Committed r148208: <http://trac.webkit.org/changeset/148208>
Comment 17 Benjamin Poulain 2013-04-15 01:01:16 PDT
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?
Comment 18 Allan Sandfeld Jensen 2013-04-15 06:18:33 PDT
(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?
Comment 19 Benjamin Poulain 2013-04-15 11:28:09 PDT
> > 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
Comment 20 Allan Sandfeld Jensen 2013-04-17 02:00:16 PDT
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.
Comment 21 Allan Sandfeld Jensen 2013-04-17 02:06:37 PDT
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
Comment 22 Allan Sandfeld Jensen 2013-04-17 02:54:19 PDT
Created attachment 198489 [details]
Patch

Disabled the CSS4 values again. They are not ready for implementation.
Comment 23 Simon Fraser (smfr) 2013-04-17 09:29:39 PDT
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.
Comment 24 Allan Sandfeld Jensen 2013-04-18 07:11:57 PDT
(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.
Comment 25 Benjamin Poulain 2013-04-18 13:14:01 PDT
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.
Comment 26 Allan Sandfeld Jensen 2013-04-19 03:14:09 PDT
Created attachment 198837 [details]
Patch
Comment 27 Build Bot 2013-04-19 05:07:24 PDT
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
Comment 28 Build Bot 2013-04-19 05:07:27 PDT
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
Comment 29 Benjamin Poulain 2013-04-20 01:10:04 PDT
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').
Comment 30 Allan Sandfeld Jensen 2013-04-20 03:55:59 PDT
(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.
Comment 31 Benjamin Poulain 2013-04-20 14:01:31 PDT
(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.
Comment 32 Allan Sandfeld Jensen 2013-04-20 14:24:57 PDT
(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.
Comment 33 Allan Sandfeld Jensen 2013-04-22 08:02:51 PDT
Created attachment 199028 [details]
Patch

Added JS test of image-rendering values
Comment 34 Allan Sandfeld Jensen 2013-04-22 08:05:07 PDT
Created attachment 199029 [details]
Patch

Removed accidentially uploaded difference
Comment 35 Build Bot 2013-04-22 10:10:10 PDT
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
Comment 36 Build Bot 2013-04-22 10:10:18 PDT
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
Comment 37 Benjamin Poulain 2013-04-22 15:35:47 PDT
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?
Comment 38 Allan Sandfeld Jensen 2013-04-23 01:16:04 PDT
Committed r148949: <http://trac.webkit.org/changeset/148949>