Bug 56627

Summary: implement image-rendering: optimize-contrast (with a vendor prefix) as defined in CSS3 image values
Product: WebKit Reporter: James Robinson <jamesr>
Component: CSSAssignee: Mike Lawther <mikelawther>
Status: RESOLVED FIXED    
Severity: Normal CC: adamk, cmsdew, commit-queue, eric, eyereight, grantgalitz, macpherson, mikelawther, ojan, peter, senorblanco, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
URL: http://www.grantgalitz.org/gameboy/
Bug Depends on: 58495, 61173    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description James Robinson 2011-03-17 22:55:34 PDT
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.
Comment 1 James Robinson 2011-03-17 22:56:39 PDT
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.
Comment 2 Ojan Vafai 2011-03-17 23:33:40 PDT
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).
Comment 3 Ojan Vafai 2011-03-17 23:37:39 PDT
James, what are the rendering changes needed?
Comment 4 James Robinson 2011-03-17 23:58:20 PDT
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.
Comment 5 Mike Lawther 2011-04-11 23:56:30 PDT
Created attachment 89165 [details]
Patch
Comment 6 WebKit Review Bot 2011-04-11 23:59:47 PDT
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.
Comment 7 Mike Lawther 2011-04-12 00:03:47 PDT
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.
Comment 9 James Robinson 2011-04-12 16:54:53 PDT
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.
Comment 10 Grant Galitz 2011-04-12 19:18:27 PDT
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.
Comment 11 Mike Lawther 2011-04-13 17:00:45 PDT
@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.
Comment 12 James Robinson 2011-04-13 17:02:18 PDT
Sounds like a plan!
Comment 13 Mike Lawther 2011-04-28 18:44:41 PDT
Created attachment 91614 [details]
Patch
Comment 14 Mike Lawther 2011-04-28 18:51:12 PDT
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.
Comment 15 Simon Fraser (smfr) 2011-04-28 18:56:34 PDT
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.
Comment 16 Mike Lawther 2011-04-28 23:06:00 PDT
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.
Comment 17 Mike Lawther 2011-05-10 18:37:00 PDT
Created attachment 93059 [details]
Patch
Comment 18 Simon Fraser (smfr) 2011-05-11 18:04:34 PDT
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.
Comment 19 Mike Lawther 2011-05-17 21:03:38 PDT
Created attachment 93865 [details]
Patch
Comment 20 Mike Lawther 2011-05-17 21:11:17 PDT
(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.
Comment 21 Geoffrey Garen 2011-05-17 21:28:51 PDT
Comment on attachment 93865 [details]
Patch

r+ based on Simon's comments.
Comment 22 Mike Lawther 2011-05-17 23:47:59 PDT
Created attachment 93874 [details]
Patch
Comment 23 Mike Lawther 2011-05-17 23:50:36 PDT
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.
Comment 24 Eric Seidel (no email) 2011-05-17 23:58:29 PDT
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?
Comment 25 Mike Lawther 2011-05-18 06:51:47 PDT
Created attachment 93907 [details]
Patch
Comment 26 Mike Lawther 2011-05-18 06:53:18 PDT
(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.
Comment 27 WebKit Commit Bot 2011-05-19 19:01:18 PDT
Comment on attachment 93907 [details]
Patch

Clearing flags on attachment: 93907

Committed r86920: <http://trac.webkit.org/changeset/86920>
Comment 28 WebKit Commit Bot 2011-05-19 19:01:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Mike Lawther 2011-05-23 15:39:26 PDT
Reopening due to rollout in https://bugs.webkit.org/show_bug.cgi?id=61173 (broke Chromium valgrind builder with enable_svg=0).
Comment 30 Mike Lawther 2011-05-23 15:41:41 PDT
Created attachment 94506 [details]
Patch
Comment 31 Grant Galitz 2011-05-23 15:46:57 PDT
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.
Comment 32 Eric Seidel (no email) 2011-05-23 15:47:19 PDT
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).
Comment 33 James Robinson 2011-05-23 16:09:51 PDT
(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.
Comment 34 Mike Lawther 2011-05-23 16:56:31 PDT
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
Comment 35 WebKit Commit Bot 2011-05-23 19:25:55 PDT
Comment on attachment 94506 [details]
Patch

Clearing flags on attachment: 94506

Committed r87121: <http://trac.webkit.org/changeset/87121>
Comment 36 WebKit Commit Bot 2011-05-23 19:26:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 37 WebKit Commit Bot 2011-05-23 21:08:51 PDT
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: alice.liu@apple.com)
http/tests/misc/uncacheable-script-repeated.html bug 51009 (author: koivisto@iki.fi)
http/tests/media/media-can-load-when-hidden.html bug 61341 (author: hausmann@webkit.org)
The commit-queue is continuing to process your patch.
Comment 38 Adam Klein 2011-05-24 09:04:42 PDT
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.
Comment 39 Adam Klein 2011-05-24 09:21:14 PDT
(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
Comment 40 Grant Galitz 2011-05-24 19:14:47 PDT
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.
Comment 41 Grant Galitz 2011-05-24 19:21:02 PDT
Test URL with a public domain that prestarts - http://www.grantgalitz.org/CrazyZone/
Comment 42 Grant Galitz 2011-05-24 19:43:21 PDT
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.
Comment 43 Grant Galitz 2011-05-24 19:44:57 PDT
http://www.grantgalitz.org/canvastestcase.html definitely is not keeping up for sure and is way below the target fps of 1000/16.
Comment 44 Simon Fraser (smfr) 2011-05-24 20:57:19 PDT
http://www.grantgalitz.org/canvastestcase.html uses less CPU with this change than without it. I don't see anything untoward.
Comment 45 Grant Galitz 2011-05-24 20:59:30 PDT
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
Comment 46 Chris Dew 2011-10-03 04:27:06 PDT
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.
Comment 47 Adam Barth 2012-01-24 22:12:48 PST
*** Bug 40881 has been marked as a duplicate of this bug. ***