Bug 56627 – implement image-rendering: optimize-contrast (with a vendor prefix) as defined in CSS3 image values

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[James Robinson]

http://dev.w3.org/csswg/css3-images/#image-rendering defines the image-rendering property and a new value 'optimize-contrast' to use for image like data where a nearest-neighbor style scaling is desirable.  This is similar to the -moz-crisp-edges value that firefox exposes (https://developer.mozilla.org/en/css/image-rendering).  Implementing this would be a big performance and quality improvements for pages like the GameBoy emulator in the URL field - that page scales the <canvas> element in CSS. Currently we waste a lot of CPU doing a bilinear filter for the upscale and as a result the pixels look blurry.

There was a bit of controversy in the working group about the name for the optimize-contrast value, so I think we should vendor-prefix the optimize-contrast value for now.  The image-rendering property already exists for SVG content and so we don't need to prefix that.

[James Robinson]

FYI I think the actual rendering changes here are trivial but I'm hoping someone else can take a look at the css parsing/property list stuff since SVGCSSPropertyNames.in scares me.  I also suspect there are some other bugs on file about this (40881 looks related) but couldn't find them all through searches.

[Ojan Vafai]

I think you just need to # comment out the line in SVGCSSPropertyNames.in, add it to CSSPropertyNames.in and then add the appropriate case statement to CSSParser::parseValue (look at http://codesearch.google.com/codesearch?q=textRendering+file%3AWebKit.*cpp%24&exact_package=chromium&hl=en&vert=chromium for an example of all the places that need updating for the new property name).

[Ojan Vafai]

James, what are the rendering changes needed?

[James Robinson]

When the value is set, we want to specify the proper resampling mode in the various platform-specific Image files so that a nearest neighbor scale is done instead of bilinear or better.

[Mike Lawther]

Created attachment 89165 [details]
Patch

[WebKit Review Bot]

Attachment 89165 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1

Source/WebCore/rendering/style/RenderStyle.cpp:553:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mike Lawther]

Here is a first cut at this patch. Would like some feedback on the rendering side of things: 
 - I'm a bit hesitant to rely on the 'lowQualityScaling' param to drawImage - it does what I want, but it doesn't feel like what I mean. I can replace this bool with the InterpolationQuality enum, using 'InterpolationDefault' to mean 'don't change the currently set interpolation', the same as useLowQualityScale=false does now (I'd do this as a separate pre-cleanup patch if so).
 - Have I hooked into the right place with the ImageQualityController::shouldPaintAtLowQuality function?

btw - the style failing is because most of RenderStyle.cpp has the boolean operator on the right, so I left it like that.

[Grant Galitz]

Updated my CSS file: https://github.com/grantgalitz/GameBoy-Online/commit/e14b2c590a627f12bdff971321c84b4a7ddb680d

[James Robinson]

Comment on attachment 89165 [details]
Patch

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

License header is wrong (sent you an email with directions for what the right header for Googlers is), but otherwise I think this looks good.

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).

> LayoutTests/css3/images/optimize-contrast-canvas.html:23
> +<script> 
> +	var canvas;

layoutTestController.dumpAsText(true); please - that way it'll generate a .png but no render tree.  That should help make the expectations more cross-platform.

[Grant Galitz]

Scaling with bilinear in chrome in fullscreen is slow with approx. maximum fps at around 20. When implementing a nearest-neighbor scaling algorithm purely in JavaScript, I can get the same fps rate, so something is seriously wrong here with this picture. This gives more reason to optimize-contrast thankfully.

[Mike Lawther]

@jamesr - thanks for the header and dumpAsText(true) comments - I've addressed those.

I've filed https://bugs.webkit.org/show_bug.cgi?id=58495 to track the useLowQualityScale->enum change, as it's orthogonal to this one (smaller patches and all :) ). Once that goes in, I'll reupload this one.

[James Robinson]

Sounds like a plan!

[Mike Lawther]

Created attachment 91614 [details]
Patch

[Mike Lawther]

I've addressed James' comments from above in this patch.

Although https://bugs.webkit.org/show_bug.cgi?id=58495 (changing useLowQuality bool -> enum) hasn't been closed yet, this one is still landable as-is.

[Simon Fraser (smfr)]

Comment on attachment 91614 [details]
Patch

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

What platform did you generate the image results on? Did you check Mac with CG?

> Source/WebCore/rendering/style/RenderStyle.h:214
>          // 45 bits

No longer 45, right?

> Source/WebCore/rendering/style/RenderStyleConstants.h:429
> +enum EImageRendering { IR_AUTO, IR_OPTIMIZESPEED, IR_OPTIMIZEQUALITY, IR_OPTIMIZECONTRAST };

UGLIEST enum values ever. Can you prettify them? No need for all caps.

[Mike Lawther]

Hey Simon - thanks for the review!

(In reply to comment #15)
> What platform did you generate the image results on? Did you check Mac with CG?

Yes - the results were generated on Snow Leopard using Safari.

> > Source/WebCore/rendering/style/RenderStyle.h:214
> >          // 45 bits
> 
> No longer 45, right?

Yes - nice catch!

> > Source/WebCore/rendering/style/RenderStyleConstants.h:429
> > +enum EImageRendering { IR_AUTO, IR_OPTIMIZESPEED, IR_OPTIMIZEQUALITY, IR_OPTIMIZECONTRAST };
> 
> UGLIEST enum values ever. Can you prettify them? No need for all caps.

(It was originally like that from the SVG code). I've CamelCased them, but I found an existing typo that I'm cleaning up first in https://bugs.webkit.org/show_bug.cgi?id=59779.

[Mike Lawther]

Created attachment 93059 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 93059 [details]
Patch

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

r=me but please moves the bits to StyleRareInheritedData

> Source/WebCore/rendering/style/RenderStyle.h:212
> +        unsigned m_imageRendering : 2; // EImageRendering */

I think this should move to StyleRareInheritedData.

[Mike Lawther]

Created attachment 93865 [details]
Patch

[Mike Lawther]

(In reply to comment #18)
> (From update of attachment 93059 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93059&action=review
> 
> r=me but please moves the bits to StyleRareInheritedData

Thanks for the review. Done.

[Geoffrey Garen]

Comment on attachment 93865 [details]
Patch

r+ based on Simon's comments.

[Mike Lawther]

Created attachment 93874 [details]
Patch

[Mike Lawther]

Previous patch failed to land because of a failing SVG CSS test. A simple rebaseline was all that was needed. "run-webkit-tests LayoutTests/css* LayoutTests/fast/css* LayoutTests/svg/css/" now all passes.

[Eric Seidel (no email)]

Comment on attachment 93874 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:102
> +    CSSPropertyImageRendering,

I don't understand the move here?

> Source/WebCore/css/CSSPrimitiveValueMappings.h:2952
> +    case ImageRenderingAuto:

Thank you for fixing the bad enum naming.

> Source/WebCore/css/CSSStyleSelector.cpp:4653
> +    case CSSPropertyImageRendering:
> +        if (!primitiveValue)
> +            return;
> +        m_style->setImageRendering(*primitiveValue);
> +        return;

We should ask Luke if this is the modern way.

> Source/WebCore/css/CSSValueKeywords.in:809
> +optimizeQuality

Are we intentionally adding this unprefixed?

> Source/WebCore/css/SVGCSSComputedStyleDeclaration.cpp:-107
> -        case CSSPropertyImageRendering:
> -            return CSSPrimitiveValue::create(svgStyle->imageRendering());

So we're moving this from being SVG-only to core css?

> Source/WebCore/html/HTMLCanvasElement.cpp:265
> +void HTMLCanvasElement::paint(GraphicsContext* context, const IntRect& r, bool useLowQualityScale)

Much better to use enums instead of bools.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:142
> +    if (object->style()->imageRendering() == WebCore::ImageRenderingOptimizeContrast)

This shouldn't need WebCore::

> Source/WebCore/rendering/RenderHTMLCanvas.cpp:61
> +    bool useLowQualityScale = style()->imageRendering() == WebCore::ImageRenderingOptimizeContrast;

Or here. Again, an enum would be better than a bool.

> Source/WebCore/rendering/style/RenderStyle.cpp:564
> +        || rareInheritedData->m_imageRendering != other->rareInheritedData->m_imageRendering)

Thank you for following modern style.

> Source/WebCore/rendering/style/RenderStyleConstants.h:429
> +enum EImageRendering { ImageRenderingAuto, ImageRenderingOptimizeSpeed, ImageRenderingOptimizeQuality, ImageRenderingOptimizeContrast };

We don't really have any style documentation for enum naming yet.  Which is horrible.  Thank you for picking something reasonable.  I'm not sure we need the leading "E" (I haven't seen that added in any recent enums), but if you like it that way or you feel it fits better with the files in question, that's fine.

Ah, now I see you're just moving this.

> Source/WebCore/rendering/style/StyleRareInheritedData.h:89
> +    unsigned m_imageRendering : 2; // EImageRendering */

Unfortunately we have to do this for Visual Studio. :)  Bitfields and enums don't get along with MSVC.   (enums are signed in MSVC).

> Source/WebCore/rendering/svg/SVGRenderTreeAsText.cpp:-324
> -    writeIfNotDefault(ts, "image rendering", svgStyle->imageRendering(), SVGRenderStyle::initialImageRendering());

Won't this affect a bunch of W3C svg results?

[Mike Lawther]

Created attachment 93907 [details]
Patch

[Mike Lawther]

(In reply to comment #24)
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:102
> > +    CSSPropertyImageRendering,
> 
> I don't understand the move here?

It was moved out of an ifdef'd ENABLE_SVG section.

> > Source/WebCore/css/CSSStyleSelector.cpp:4653
> > +    case CSSPropertyImageRendering:
> > +        if (!primitiveValue)
> > +            return;
> > +        m_style->setImageRendering(*primitiveValue);
> > +        return;
> 
> We should ask Luke if this is the modern way.

OK

> > Source/WebCore/css/CSSValueKeywords.in:809
> > +optimizeQuality
> 
> Are we intentionally adding this unprefixed?

Yes, so as not to break any existing SVG out there.

> > Source/WebCore/css/SVGCSSComputedStyleDeclaration.cpp:-107
> > -        case CSSPropertyImageRendering:
> > -            return CSSPrimitiveValue::create(svgStyle->imageRendering());
> 
> So we're moving this from being SVG-only to core css?

Yep.

> > Source/WebCore/html/HTMLCanvasElement.cpp:265
> > +void HTMLCanvasElement::paint(GraphicsContext* context, const IntRect& r, bool useLowQualityScale)
> 
> Much better to use enums instead of bools.

Agreed. https://bugs.webkit.org/show_bug.cgi?id=58495 has already been filed to track this.

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:142
> > +    if (object->style()->imageRendering() == WebCore::ImageRenderingOptimizeContrast)
> 
> This shouldn't need WebCore::

Thanks - fixed.

> > Source/WebCore/rendering/RenderHTMLCanvas.cpp:61
> > +    bool useLowQualityScale = style()->imageRendering() == WebCore::ImageRenderingOptimizeContrast;
> 
> Or here. Again, an enum would be better than a bool.

Fixed (the WebCore:: removing, not the bool :) ).

> > Source/WebCore/rendering/style/RenderStyle.cpp:564
> > +        || rareInheritedData->m_imageRendering != other->rareInheritedData->m_imageRendering)
> 
> Thank you for following modern style.

Modern style is with the m_ prefix right?

> > Source/WebCore/rendering/style/RenderStyleConstants.h:429
> > +enum EImageRendering { ImageRenderingAuto, ImageRenderingOptimizeSpeed, ImageRenderingOptimizeQuality, ImageRenderingOptimizeContrast };
> 
> We don't really have any style documentation for enum naming yet.  Which is horrible.  Thank you for picking something reasonable.  I'm not sure we need the leading "E" (I haven't seen that added in any recent enums), but if you like it that way or you feel it fits better with the files in question, that's fine.
> 
> Ah, now I see you're just moving this.

Yes, I kept the old 'E' prefix. Half of the enums in that file have the 'E' prefix, half don't :(

> > Source/WebCore/rendering/style/StyleRareInheritedData.h:89
> > +    unsigned m_imageRendering : 2; // EImageRendering */
> 
> Unfortunately we have to do this for Visual Studio. :)  Bitfields and enums don't get along with MSVC.   (enums are signed in MSVC).

You mean "we have to have the ''unsigned'"? 

> > Source/WebCore/rendering/svg/SVGRenderTreeAsText.cpp:-324
> > -    writeIfNotDefault(ts, "image rendering", svgStyle->imageRendering(), SVGRenderStyle::initialImageRendering());
> 
> Won't this affect a bunch of W3C svg results?

Good point - I've changed it to pull the value out of RenderStyle instead.

[WebKit Commit Bot]

Comment on attachment 93907 [details]
Patch

Clearing flags on attachment: 93907

Committed r86920: <http://trac.webkit.org/changeset/86920>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mike Lawther]

Reopening due to rollout in https://bugs.webkit.org/show_bug.cgi?id=61173 (broke Chromium valgrind builder with enable_svg=0).

[Mike Lawther]

Created attachment 94506 [details]
Patch

[Grant Galitz]

Why was scaling still slow when the last patch was applied and nearest-neighbor was seen being applied as the rendering algorithm? It felt only marginally better than the normal scaling algorithm.

[Eric Seidel (no email)]

Comment on attachment 94506 [details]
Patch

Assuming your'e just fixing the no-svg build, OK.  Chromium should have a no-svg builder on build.webkit.org if they expect the no-SVG build to work.  We've strongly considered disabling that option entirely (making SVG always on).

[James Robinson]

(In reply to comment #31)
> Why was scaling still slow when the last patch was applied and nearest-neighbor was seen being applied as the rendering algorithm? It felt only marginally better than the normal scaling algorithm.

Did you measure it exactly, or can you construct a test page where we can measure the scale time exactly?  I would expect this to speed up the scaling.

[Mike Lawther]

What browser/platform are you testing on, and are you testing image rendering, or canvas rendering?
I'd also expect it to be faster, but there might be something gnarly happening on the canvas path

[WebKit Commit Bot]

Comment on attachment 94506 [details]
Patch

Clearing flags on attachment: 94506

Committed r87121: <http://trac.webkit.org/changeset/87121>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 94506 [details]:

http/tests/misc/favicon-loads-with-icon-loading-override.html bug 58412 (author: [e-mail])
http/tests/misc/uncacheable-script-repeated.html bug 51009 (author: [e-mail])
http/tests/media/media-can-load-when-hidden.html bug 61341 (author: [e-mail])
The commit-queue is continuing to process your patch.

[Adam Klein]

The tests added in this patch seem to fail on the Chromium canary Windows and Linux bots:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win/builds/4776/steps/webkit_tests/logs/stdio
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/8068/steps/webkit_tests/logs/stdio

Please take a look and consider rolling out.

[Adam Klein]

(In reply to comment #38)
> The tests added in this patch seem to fail on the Chromium canary Windows and Linux bots:
> 
> http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win/builds/4776/steps/webkit_tests/logs/stdio
> http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/8068/steps/webkit_tests/logs/stdio
> 
> Please take a look and consider rolling out.

This is tracked in https://bugs.webkit.org/show_bug.cgi?id=61169

[Grant Galitz]

Jamesr: Something is holding up the entire JS thread when scaling is done. I'm not even seeing full CPU usage when it slows down, it just seems like webkit is delaying the system timers by doing something weird. For a 160x144 canvas being scaled via nearest-neighbor, it shouldn't slow down to 10 fps when it's scaled up to 400x300 by the fast nearest-neighbor algorithm. Something seriously "gnarly" is going on for sure.
WebKit Nightly for Mac OS X (On Snow Leopard 10.6.7)
Version 5.0.5 (6533.21.1, r87175)

URL listed for this bug still is slow on scaling up the canvas (I can see nearest-neighbor is being applied too now). There definitely seems to be something hanging up the timing.

[Grant Galitz]

Test URL with a public domain that prestarts - http://www.grantgalitz.org/CrazyZone/

[Grant Galitz]

Ok, made a testcase that putImageData's white noise to a canvas: http://www.grantgalitz.org/canvastestcase.html

Definitely eating up 2/3rds of one core on my mac.

[Grant Galitz]

http://www.grantgalitz.org/canvastestcase.html definitely is not keeping up for sure and is way below the target fps of 1000/16.

[Simon Fraser (smfr)]

http://www.grantgalitz.org/canvastestcase.html uses less CPU with this change than without it. I don't see anything untoward.

[Grant Galitz]

Load still seems a little high for a nn algorithm. Also my emulator slows down to half speed when you fullscreen the canvas. When it's 1:1 it's fullspeed. meh

[Chris Dew]

How do I tell when "image-rendering: -webkit-optimize-contrast;" will land in Chrome and Safari.  

Has it landed in either yet? (It doesn't seem to work in either.)

Thanks,

Chris.

[Adam Barth]

*** Bug 40881 has been marked as a duplicate of this bug. ***