Bug 56627 - implement image-rendering: optimize-contrast (with a vendor prefix) as defined in CSS3 image values
: implement image-rendering: optimize-contrast (with a vendor prefix) as define...
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
: http://www.grantgalitz.org/gameboy/
:
: 58495 61173
:
  Show dependency treegraph
 
Reported: 2011-03-17 22:55 PST by
Modified: 2012-01-24 22:12 PST (History)


Attachments
Patch (41.34 KB, patch)
2011-04-11 23:56 PST, Mike Lawther
no flags Review Patch | Details | Formatted Diff | Diff
Patch (38.17 KB, patch)
2011-04-28 18:44 PST, Mike Lawther
no flags Review Patch | Details | Formatted Diff | Diff
Patch (39.35 KB, patch)
2011-05-10 18:37 PST, Mike Lawther
no flags Review Patch | Details | Formatted Diff | Diff
Patch (44.53 KB, patch)
2011-05-17 21:03 PST, Mike Lawther
no flags Review Patch | Details | Formatted Diff | Diff
Patch (47.02 KB, patch)
2011-05-17 23:47 PST, Mike Lawther
no flags Review Patch | Details | Formatted Diff | Diff
Patch (47.11 KB, patch)
2011-05-18 06:51 PST, Mike Lawther
no flags Review Patch | Details | Formatted Diff | Diff
Patch (47.79 KB, patch)
2011-05-23 15:41 PST, Mike Lawther
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-03-17 22:55:34 PST
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 From 2011-03-17 22:56:39 PST -------
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 From 2011-03-17 23:33:40 PST -------
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 From 2011-03-17 23:37:39 PST -------
James, what are the rendering changes needed?
------- Comment #4 From 2011-03-17 23:58:20 PST -------
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 From 2011-04-11 23:56:30 PST -------
Created an attachment (id=89165) [details]
Patch
------- Comment #6 From 2011-04-11 23:59:47 PST -------
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 From 2011-04-12 00:03:47 PST -------
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 #8 From 2011-04-12 16:30:59 PST -------
Updated my CSS file: https://github.com/grantgalitz/GameBoy-Online/commit/e14b2c590a627f12bdff971321c84b4a7ddb680d
------- Comment #9 From 2011-04-12 16:54:53 PST -------
(From update of attachment 89165 [details])
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 From 2011-04-12 19:18:27 PST -------
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 From 2011-04-13 17:00:45 PST -------
@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 From 2011-04-13 17:02:18 PST -------
Sounds like a plan!
------- Comment #13 From 2011-04-28 18:44:41 PST -------
Created an attachment (id=91614) [details]
Patch
------- Comment #14 From 2011-04-28 18:51:12 PST -------
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 From 2011-04-28 18:56:34 PST -------
(From update of attachment 91614 [details])
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 From 2011-04-28 23:06:00 PST -------
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 From 2011-05-10 18:37:00 PST -------
Created an attachment (id=93059) [details]
Patch
------- Comment #18 From 2011-05-11 18:04:34 PST -------
(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

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

I think this should move to StyleRareInheritedData.
------- Comment #19 From 2011-05-17 21:03:38 PST -------
Created an attachment (id=93865) [details]
Patch
------- Comment #20 From 2011-05-17 21:11:17 PST -------
(In reply to comment #18)
> (From update of attachment 93059 [details] [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 From 2011-05-17 21:28:51 PST -------
(From update of attachment 93865 [details])
r+ based on Simon's comments.
------- Comment #22 From 2011-05-17 23:47:59 PST -------
Created an attachment (id=93874) [details]
Patch
------- Comment #23 From 2011-05-17 23:50:36 PST -------
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 From 2011-05-17 23:58:29 PST -------
(From update of attachment 93874 [details])
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 From 2011-05-18 06:51:47 PST -------
Created an attachment (id=93907) [details]
Patch
------- Comment #26 From 2011-05-18 06:53:18 PST -------
(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 From 2011-05-19 19:01:18 PST -------
(From update of attachment 93907 [details])
Clearing flags on attachment: 93907

Committed r86920: <http://trac.webkit.org/changeset/86920>
------- Comment #28 From 2011-05-19 19:01:26 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #29 From 2011-05-23 15:39:26 PST -------
Reopening due to rollout in https://bugs.webkit.org/show_bug.cgi?id=61173 (broke Chromium valgrind builder with enable_svg=0).
------- Comment #30 From 2011-05-23 15:41:41 PST -------
Created an attachment (id=94506) [details]
Patch
------- Comment #31 From 2011-05-23 15:46:57 PST -------
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 From 2011-05-23 15:47:19 PST -------
(From update of attachment 94506 [details])
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 From 2011-05-23 16:09:51 PST -------
(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 From 2011-05-23 16:56:31 PST -------
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 From 2011-05-23 19:25:55 PST -------
(From update of attachment 94506 [details])
Clearing flags on attachment: 94506

Committed r87121: <http://trac.webkit.org/changeset/87121>
------- Comment #36 From 2011-05-23 19:26:03 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #37 From 2011-05-23 21:08:51 PST -------
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 From 2011-05-24 09:04:42 PST -------
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 From 2011-05-24 09:21:14 PST -------
(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 From 2011-05-24 19:14:47 PST -------
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 From 2011-05-24 19:21:02 PST -------
Test URL with a public domain that prestarts - http://www.grantgalitz.org/CrazyZone/
------- Comment #42 From 2011-05-24 19:43:21 PST -------
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 From 2011-05-24 19:44:57 PST -------
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 From 2011-05-24 20:57:19 PST -------
http://www.grantgalitz.org/canvastestcase.html uses less CPU with this change than without it. I don't see anything untoward.
------- Comment #45 From 2011-05-24 20:59:30 PST -------
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 From 2011-10-03 04:27:06 PST -------
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 From 2012-01-24 22:12:48 PST -------
*** Bug 40881 has been marked as a duplicate of this bug. ***