Bug 56627

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.

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

James, what are the rendering changes needed?

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.

Created attachment 89165 [details] Patch

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.

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.

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

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.

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.

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

Sounds like a plan!

Created attachment 91614 [details] Patch

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

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.

Created attachment 93059 [details] Patch

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.

Created attachment 93865 [details] Patch

(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 on attachment 93865 [details] Patch r+ based on Simon's comments.

Created attachment 93874 [details] Patch

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 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?

Created attachment 93907 [details] Patch

(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 on attachment 93907 [details] Patch Clearing flags on attachment: 93907 Committed r86920: <http://trac.webkit.org/changeset/86920>

All reviewed patches have been landed. Closing bug.

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

Created attachment 94506 [details] Patch

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

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

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 on attachment 94506 [details] Patch Clearing flags on attachment: 94506 Committed r87121: <http://trac.webkit.org/changeset/87121>

All reviewed patches have been landed. Closing bug.

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.

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.

(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

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.

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

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.

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

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

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

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.

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