Bug 31196

Created attachment 42622 [details] Patch

Comment on attachment 42622 [details] Patch > +CSSColorSpace GraphicsContext::cgToCSSColorSpace(ColorSpace colorSpace) > +{ > + if (colorSpace == sRGBColorSpace) > + return sRGB; > + return Default; > +} > + > +ColorSpace GraphicsContext::cssToCGColorSpace(CSSColorSpace colorSpace) > +{ > + if (colorSpace == sRGB) > + return sRGBColorSpace; > + return DeviceRGBColorSpace; > +} Using switch() statements here would be good because then if someone added new color space enum values, it would give a warning. > Index: WebCore/platform/graphics/GraphicsContext.h > =================================================================== > // a fill or stroke is using a gradient or pattern instead of a solid color. > // https://bugs.webkit.org/show_bug.cgi?id=20558 > enum ColorSpace { > - SolidColorSpace, > + DeviceRGBColorSpace, > + sRGBColorSpace, > PatternColorSpace, > GradientColorSpace This is a bit confusing; why is sRGBColorSpace a peer of PatternColorSpace and GradientColorSpace? Wouldn't you want to be able to say that a pattern or gradient is in sRGB or deviceRGB? I think the existing ColorSpace enum is misnamed; it's more like a "brush". You may need Device/sRGB in a new enum. > - void setStrokeColor(const Color&); > + void setStrokeColor(const Color&, CSSColorSpace colorSpace = Default); Rather than all these changes to GraphicsContext methods, maybe it make more sense to push a color space onto the graphics context and have it be part of the state? > Index: WebCore/platform/graphics/cg/GraphicsContextCG.cpp > =================================================================== > +static void setCGFillColor(CGContextRef context, const Color& color, ColorSpace colorSpace) > +{ > + CGFloat components[4]; > + color.getRGBA(components[0], components[1], components[2], components[3]); > + > + CGColorRef cgColor; > + if (colorSpace == sRGBColorSpace) > + cgColor = CGColorCreate(sRGBColorSpaceRef(), components); > + else > + cgColor = CGColorCreate(deviceRGBColorSpaceRef(), components); > + > + CGContextSetFillColorWithColor(context, cgColor); This is leaking a CGColorRef. > +static void setCGStrokeColor(CGContextRef context, const Color& color, ColorSpace colorSpace) > +{ > + CGFloat components[4]; > + color.getRGBA(components[0], components[1], components[2], components[3]); > + > + CGColorRef cgColor; > + if (colorSpace == sRGBColorSpace) > + cgColor = CGColorCreate(sRGBColorSpaceRef(), components); > + else > + cgColor = CGColorCreate(deviceRGBColorSpaceRef(), components); > + > + CGContextSetStrokeColorWithColor(context, cgColor); Ditto. I may have missed them, but: * Where are untagged images tagged with sRGB? * What about <canvas>, WebGL, gradients, etc? r- for the leaks, for now, but this is a good start. Maybe talked to dhyatt about the graphics context changes.

(In reply to comment #2) > (From update of attachment 42622 [details]) > > +CSSColorSpace GraphicsContext::cgToCSSColorSpace(ColorSpace colorSpace) > > +{ > > + if (colorSpace == sRGBColorSpace) > > + return sRGB; > > + return Default; > > +} > > + > > +ColorSpace GraphicsContext::cssToCGColorSpace(CSSColorSpace colorSpace) > > +{ > > + if (colorSpace == sRGB) > > + return sRGBColorSpace; > > + return DeviceRGBColorSpace; > > +} > > Using switch() statements here would be good because then if someone added new > color space enum values, it would give a warning. > I wrote these as if-statements since ColorSpace has some values (Gradient and Pattern) that are ignored here. More on that later. > > Index: WebCore/platform/graphics/GraphicsContext.h > > =================================================================== > > > // a fill or stroke is using a gradient or pattern instead of a solid color. > > // https://bugs.webkit.org/show_bug.cgi?id=20558 > > enum ColorSpace { > > - SolidColorSpace, > > + DeviceRGBColorSpace, > > + sRGBColorSpace, > > PatternColorSpace, > > GradientColorSpace > > This is a bit confusing; why is sRGBColorSpace a peer of PatternColorSpace and > GradientColorSpace? Wouldn't you want to be able to say that a pattern or > gradient is in sRGB or deviceRGB? > > I think the existing ColorSpace enum is misnamed; it's more like a "brush". You > may need Device/sRGB in a new enum. I have to admit that I found this confusing too, and really I still do. But it seems like something confusing about CG that is just replicated in our code. The following is an actual line from GraphicsContextCG.cpp: CGContextSetStrokeColorSpace(cgContext, patternSpace.get()); I don't understand why CG treats patterns and spaces like sRGB the same, but it appears they do. Maybe someone on the team who knows more about CG could enlighten us? But given that fact that CG treats patternSpace as a ColorSpace, this enum doesn't seem crazy. Mostly I was just trying to honor the existing code and trust that there was logic to it. Of course it's possible that is too trusting. > > > - void setStrokeColor(const Color&); > > + void setStrokeColor(const Color&, CSSColorSpace colorSpace = Default); > > Rather than all these changes to GraphicsContext methods, maybe it make more > sense to push a color space onto the graphics context and have it be part of > the state? > The color space -- just like color itself -- was already a part of the state and remains so with this patch, if you look at GraphicsContext::setFillColor(), you will see: - m_common->state.fillColorSpace = SolidColorSpace; + m_common->state.fillColorSpace = cssToCGColorSpace(colorSpace); Even though color is a part of the state as well, many functions on GraphicsContext accept a Color as a parameter and ignore the color on the state. All of those functions now need to also take a ColorSpace and ignore the state's color space. I think this part of my patch is correct. > > Index: WebCore/platform/graphics/cg/GraphicsContextCG.cpp > > =================================================================== > > > +static void setCGFillColor(CGContextRef context, const Color& color, ColorSpace colorSpace) > > +{ > > + CGFloat components[4]; > > + color.getRGBA(components[0], components[1], components[2], components[3]); > > + > > + CGColorRef cgColor; > > + if (colorSpace == sRGBColorSpace) > > + cgColor = CGColorCreate(sRGBColorSpaceRef(), components); > > + else > > + cgColor = CGColorCreate(deviceRGBColorSpaceRef(), components); > > + > > + CGContextSetFillColorWithColor(context, cgColor); > > This is leaking a CGColorRef. Will fix and re-post a patch tomorrow. > > > +static void setCGStrokeColor(CGContextRef context, const Color& color, ColorSpace colorSpace) > > +{ > > + CGFloat components[4]; > > + color.getRGBA(components[0], components[1], components[2], components[3]); > > + > > + CGColorRef cgColor; > > + if (colorSpace == sRGBColorSpace) > > + cgColor = CGColorCreate(sRGBColorSpaceRef(), components); > > + else > > + cgColor = CGColorCreate(deviceRGBColorSpaceRef(), components); > > + > > + CGContextSetStrokeColorWithColor(context, cgColor); > > Ditto. > Will fix and re-post a patch tomorrow. > I may have missed them, but: > * Where are untagged images tagged with sRGB? > * What about <canvas>, WebGL, gradients, etc? This patch is big enough as it is that I wanted to keep it as simple as possible. As you see, I titled the bug "Implement -webkit-color-correction for CSS colors." The Radar version tracks the whole property, and I won't move it to "integrate" until we have more implemented. I plan to file follow-up bugs here for the remaining features.

See also bug 20558.

(In reply to comment #1) > Created an attachment (id=42622) [details] > Patch > Index: WebCore/css/CSSValueKeywords.in > =================================================================== > --- WebCore/css/CSSValueKeywords.in (revision 50570) > +++ WebCore/css/CSSValueKeywords.in (working copy) > @@ -638,3 +638,7 @@ subpixel-antialiased > optimizeSpeed > optimizeLegibility > geometricPrecision > + > +# -webkit-color-correction > +#default > +sRGB > Index: WebCore/css/SVGCSSValueKeywords.in > =================================================================== > --- WebCore/css/SVGCSSValueKeywords.in (revision 50570) > +++ WebCore/css/SVGCSSValueKeywords.in (working copy) > @@ -183,7 +183,7 @@ new > # CSS_PROP_STOP_OPACITY > # CSS_PROP_COLOR_INTERPOLATION > #auto > -sRGB > +#sRGB > linearRGB Does switching the color-space on SVG via CSS still work after this changes? (We'll need the switching at least for masking and filtering)

(In reply to comment #1) > Created an attachment (id=42622) [details] > Patch Can we find another name for cssToCGColorSpace and cgToCSSColorSpace since GraphicsContext is the platform-independent code? :-) Mabe default or deviceColorSpace like on ImageBuffer.h.

Hi Dirk, It will still work for SVG. Also, I noticed that I had named those functions in a very CG-centric way myself, and I changed them in my working patch. Hopefully I'll have it uploaded today. -Beth

Created attachment 42846 [details] New Patch New and improved patch! This patch fixes the leaks Simon found, is built on top of the newly cleaned-up GraphicsContext, uses a single enum stored in it's own header for CSS and GraphicsContext (platform/graphics/ColorSpace.h), requires callers of GraphicsContext functions to provide ColorSpaces rather than sneaking in using defaults (per Darin's request), and attempts to keep other ports building.

Comment on attachment 42846 [details] New Patch Have you tested the performance impact of this yet? > case CSSPropertyWebkitBoxSizing: > valid_primitive = id == CSSValueBorderBox || id == CSSValueContentBox; > break; > + case CSSPropertyWebkitColorCorrection: > + if (id == CSSValueSrgb || id == CSSValueDefault) > + valid_primitive = true; > + break; I notice that the previous case uses just an assignment, no if statement. Is there agood reason you chose a different size? > + case CSSPropertyWebkitColorCorrection: { > + if (isInherit) > + m_style->setColorSpace(m_parentStyle->colorSpace()); > + else if (isInitial) > + m_style->setColorSpace(DeviceColorSpace); > + else { > + if (!primitiveValue) > + return; > + m_style->setColorSpace(*primitiveValue); > + } > + return; > + } The outer braces are not required. > Color caretColor = Color::black; > + ColorSpace colorSpace = DeviceColorSpace; > Element* element = rootEditableElement(); > - if (element && element->renderer()) > + if (element && element->renderer()) { > caretColor = element->renderer()->style()->color(); > + colorSpace = element->renderer()->style()->colorSpace(); > + } > > - p->fillRect(caret, caretColor); > + p->fillRect(caret, caretColor, colorSpace); Code like this really cries out for a single structure that contains both color and color space. > - ctxt->fillRect(dstRect, color); > + ctxt->fillRect(dstRect, color, DeviceColorSpace); Seems to me that this already does the wrong thing for images that have a color profile. In that case, the solid color optimization needs to be color-corrected, not device. A bug, not caused by your change. > - if (mode & cTextFill && fillColor != context->fillColor()) > - context->setFillColor(fillColor); > + if (mode & cTextFill && (fillColor != context->fillColor() > + || colorSpace != context->fillColorSpace())) > + context->setFillColor(fillColor, colorSpace); I think this would read better all on one line. And also, I think a single struct for color and color space would make this so much more readable! > static void paintTextWithShadows(GraphicsContext* context, const Font& font, const TextRun& textRun, int startOffset, int endOffset, const IntPoint& textOrigin, int x, int y, int w, int h, ShadowData* shadow, bool stroked) > { > Color fillColor = context->fillColor(); > + ColorSpace fillColorSpace = context->fillColorSpace(); > bool opaque = fillColor.alpha() == 255; > if (!opaque) > - context->setFillColor(Color::black); > + context->setFillColor(Color::black, DeviceColorSpace); I'm not sure DeviceColorSpace is right for this; maybe fillColorSpace would be right. What test case would demonstrate the difference? > // Helper functions shared by InlineTextBox / SVGRootInlineBox > -void updateGraphicsContext(GraphicsContext* context, const Color& fillColor, const Color& strokeColor, float strokeThickness); > +void updateGraphicsContext(GraphicsContext* context, const Color& fillColor, const Color& strokeColor, float strokeThickness, ColorSpace colorSpace); The context and colorSpace argument names should not be here. > - context->fillRoundedRect(fillRect, topLeft, topRight, bottomLeft, bottomRight, Color::black); > + context->fillRoundedRect(fillRect, topLeft, topRight, bottomLeft, bottomRight, Color::black, DeviceColorSpace); I think we might want to use the color space from the style here, not DeviceColorSpace. > @@ -1258,7 +1258,7 @@ void RenderBoxModelObject::paintBoxShado > > if (!rectToClipOut.isEmpty()) > context->clipOut(rectToClipOut); > - context->fillRect(fillRect, Color::black); > + context->fillRect(fillRect, Color::black, DeviceColorSpace); Ditto. > // Draw an outline rect where the image should be. > context->setStrokeStyle(SolidStroke); > - context->setStrokeColor(Color::lightGray); > - context->setFillColor(Color::transparent); > + context->setStrokeColor(Color::lightGray, DeviceColorSpace); > + context->setFillColor(Color::transparent, DeviceColorSpace); I think we want the style of the image element to control this rather than hard-coding DeviceColorSpace. > - context->fillRect(absRect, Color::white); > + context->fillRect(absRect, Color::white, DeviceColorSpace); These corners may need to respect the color space setting for the element they are for rather than DeviceColorSpace. > + ColorSpace colorSpace = element->renderStyle() ? element->renderStyle()->colorSpace() : DeviceColorSpace; When would element->renderStyle() be 0? In that case, why are we sure DeviceColorSpace is right? > [[textColor colorUsingColorSpaceName:NSDeviceRGBColorSpace] getRed:&red green:&green blue:&blue alpha:&alpha]; > - graphicsContext.setFillColor(makeRGBA(red * 255, green * 255, blue * 255, alpha * 255)); > + graphicsContext.setFillColor(makeRGBA(red * 255, green * 255, blue * 255, alpha * 255), DeviceColorSpace); Eventually, this code should change to respect the color space in the NSColor. If it's device, then stay with device, but if it’s not, then color-correct to sRGB values. We've got to find a way to have more test coverage. We should have a separate test for each call site where we are passing in a color space, and some easy way to construct the tests. This might include some DumpRenderTree enhancements. r=me as-is -- this clearly all will work fine when nobody uses the new feature But lets add a lot more test cases, and use the test cases to drive more clarity on whether these explicit DeviceColorSpace call sites are correct. I suspect at least half of them are not.

Comment on attachment 42846 [details] New Patch I wonder if the GraphicsContext methods that now take a ColorSpace could have a default value for the parameter, since it's almost always the same.

(In reply to comment #9) > (From update of attachment 42846 [details]) > Have you tested the performance impact of this yet? > > > case CSSPropertyWebkitBoxSizing: > > valid_primitive = id == CSSValueBorderBox || id == CSSValueContentBox; > > break; > > + case CSSPropertyWebkitColorCorrection: > > + if (id == CSSValueSrgb || id == CSSValueDefault) > > + valid_primitive = true; > > + break; > > I notice that the previous case uses just an assignment, no if statement. Is > there agood reason you chose a different size? > No good reason. I fixed this. > > + case CSSPropertyWebkitColorCorrection: { > > + if (isInherit) > > + m_style->setColorSpace(m_parentStyle->colorSpace()); > > + else if (isInitial) > > + m_style->setColorSpace(DeviceColorSpace); > > + else { > > + if (!primitiveValue) > > + return; > > + m_style->setColorSpace(*primitiveValue); > > + } > > + return; > > + } > > The outer braces are not required. > Fixed. > > Color caretColor = Color::black; > > + ColorSpace colorSpace = DeviceColorSpace; > > Element* element = rootEditableElement(); > > - if (element && element->renderer()) > > + if (element && element->renderer()) { > > caretColor = element->renderer()->style()->color(); > > + colorSpace = element->renderer()->style()->colorSpace(); > > + } > > > > - p->fillRect(caret, caretColor); > > + p->fillRect(caret, caretColor, colorSpace); > > Code like this really cries out for a single structure that contains both color > and color space. I agree. Did not fix for now though. > > > - ctxt->fillRect(dstRect, color); > > + ctxt->fillRect(dstRect, color, DeviceColorSpace); > > Seems to me that this already does the wrong thing for images that have a color > profile. In that case, the solid color optimization needs to be > color-corrected, not device. A bug, not caused by your change. > Huh. Good observation. I will look into this more when I work on getting this property to work with untagged images. > > - if (mode & cTextFill && fillColor != context->fillColor()) > > - context->setFillColor(fillColor); > > + if (mode & cTextFill && (fillColor != context->fillColor() > > + || colorSpace != context->fillColorSpace())) > > + context->setFillColor(fillColor, colorSpace); > > I think this would read better all on one line. And also, I think a single > struct for color and color space would make this so much more readable! > Fixed the first part, but not the second (yet). > > static void paintTextWithShadows(GraphicsContext* context, const Font& font, const TextRun& textRun, int startOffset, int endOffset, const IntPoint& textOrigin, int x, int y, int w, int h, ShadowData* shadow, bool stroked) > > { > > Color fillColor = context->fillColor(); > > + ColorSpace fillColorSpace = context->fillColorSpace(); > > bool opaque = fillColor.alpha() == 255; > > if (!opaque) > > - context->setFillColor(Color::black); > > + context->setFillColor(Color::black, DeviceColorSpace); > > I'm not sure DeviceColorSpace is right for this; maybe fillColorSpace would be > right. What test case would demonstrate the difference? > Changed to fillColorSpace. I agree that is more correct. > > // Helper functions shared by InlineTextBox / SVGRootInlineBox > > -void updateGraphicsContext(GraphicsContext* context, const Color& fillColor, const Color& strokeColor, float strokeThickness); > > +void updateGraphicsContext(GraphicsContext* context, const Color& fillColor, const Color& strokeColor, float strokeThickness, ColorSpace colorSpace); > > The context and colorSpace argument names should not be here. Fixed. > > > - context->fillRoundedRect(fillRect, topLeft, topRight, bottomLeft, bottomRight, Color::black); > > + context->fillRoundedRect(fillRect, topLeft, topRight, bottomLeft, bottomRight, Color::black, DeviceColorSpace); > > I think we might want to use the color space from the style here, not > DeviceColorSpace. Fixed. > > > @@ -1258,7 +1258,7 @@ void RenderBoxModelObject::paintBoxShado > > > > if (!rectToClipOut.isEmpty()) > > context->clipOut(rectToClipOut); > > - context->fillRect(fillRect, Color::black); > > + context->fillRect(fillRect, Color::black, DeviceColorSpace); > > Ditto. Fixed. > > > // Draw an outline rect where the image should be. > > context->setStrokeStyle(SolidStroke); > > - context->setStrokeColor(Color::lightGray); > > - context->setFillColor(Color::transparent); > > + context->setStrokeColor(Color::lightGray, DeviceColorSpace); > > + context->setFillColor(Color::transparent, DeviceColorSpace); > > I think we want the style of the image element to control this rather than > hard-coding DeviceColorSpace. Fixed. > > > - context->fillRect(absRect, Color::white); > > + context->fillRect(absRect, Color::white, DeviceColorSpace); > > These corners may need to respect the color space setting for the element they > are for rather than DeviceColorSpace. Fixed. > > > + ColorSpace colorSpace = element->renderStyle() ? element->renderStyle()->colorSpace() : DeviceColorSpace; > > When would element->renderStyle() be 0? In that case, why are we sure > DeviceColorSpace is right? Not sure when it would happen, but this code if nested inside an if-statement that reads: if (!element->renderStyle() || element->renderStyle()->visibility() != HIDDEN) So it definitely seems possible. That being said, I looked around the file, and it looks like the renderer's style is used whenever the element's style is null, so I did that instead. > > > [[textColor colorUsingColorSpaceName:NSDeviceRGBColorSpace] getRed:&red green:&green blue:&blue alpha:&alpha]; > > - graphicsContext.setFillColor(makeRGBA(red * 255, green * 255, blue * 255, alpha * 255)); > > + graphicsContext.setFillColor(makeRGBA(red * 255, green * 255, blue * 255, alpha * 255), DeviceColorSpace); > > Eventually, this code should change to respect the color space in the NSColor. > If it's device, then stay with device, but if it’s not, then color-correct to > sRGB values. > Agreed. > We've got to find a way to have more test coverage. We should have a separate > test for each call site where we are passing in a color space, and some easy > way to construct the tests. This might include some DumpRenderTree > enhancements. > > r=me as-is -- this clearly all will work fine when nobody uses the new feature > > But lets add a lot more test cases, and use the test cases to drive more > clarity on whether these explicit DeviceColorSpace call sites are correct. I > suspect at least half of them are not. Thanks Darin! I agree about the tests. Committed the fix: http://trac.webkit.org/changeset/50760

(In reply to comment #10) > (From update of attachment 42846 [details]) > I wonder if the GraphicsContext methods that now take a ColorSpace could have a > default value for the parameter, since it's almost always the same. I originally had it implemented with defaults, but Darin thought we should get rid of them. His argument was that all call sites -- old and especially new -- should be forced to consider color spaces now that we support them. This sounded reasonable to me, so I got rid of the defaults even though that inflated the patch a lot.

Follow-ups: https://bugs.webkit.org/show_bug.cgi?id=31321 Make -webkit-color-correction work with untagged images https://bugs.webkit.org/show_bug.cgi?id=31319 Make -webkit-color-correction work with canvas https://bugs.webkit.org/show_bug.cgi?id=31316 Make sure -webkit-color-correction is doing the right thing on Windows

FWIW, according to the message posted by D. Baron on www-style the 2 values for the property are 'auto' and 'sRGB'. The description (in comment 0) in this bug, and as far as I can tell, the patch implements 'default' and 'sRGB' http://lists.w3.org/Archives/Public/www-style/2009Nov/0226.html

Thanks for the heads up. I am pretty confident that we settled on default at the face-to-face. Either I am confused or dbaron is. I will ping him shortly.

My recollection of the CSS-WG F2F was that we decided on 'default' after input from Tantek.

Ah, dbaron changed his mind: <http://lists.w3.org/Archives/Public/www-style/2009Nov/0226.html>