Bug 150967 – Remove ColorSpace argument to all the drawing calls

• Component: Platform
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Remove ColorSpace argument to all the drawing calls

[Simon Fraser (smfr)]

Created attachment 264917 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 264919 [details]
Patch

[Tim Horton]

I thought you could switch SVG filters into linearized sRGB? Is this ok because we don't ever paint with GraphicsContext into those buffers, or because we only paint regular sRGB colors into them? (I guess it's probably that)

[Simon Fraser (smfr)]

I don't want to change any ImageBuffer or filter stuff at this point. This is just removing input params that are always ColorSpaceSRGB.

[Simon Fraser (smfr)]

...always ColorSpaceSRGB (hard-coded in style) or ColorSpaceDeviceRGB (hardcoded at many call sites).

[Darin Adler]

Comment on attachment 264919 [details]
Patch

One hard part here is getting all the platforms to build. GTK, EFL, and iOS all failed on EWS.

[Simon Fraser (smfr)]

Created attachment 264939 [details]
Patch

[Darin Adler]

Comment on attachment 264939 [details]
Patch

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

Looking forward to merging this with my mega-patch after you land it ;)

> Source/WebCore/platform/graphics/ImageBuffer.h:72
> +    static std::unique_ptr<ImageBuffer> create(const FloatSize& size, RenderingMode renderingMode, float resolutionScale = 1, ColorSpace colorSpace = ColorSpaceDeviceRGB /* FIXME sRGB */)

I don’t think you should land this change as part of this patch. The comment isn’t all that valuable. We’re going to remove the ColorSpaceDeviceRGB constant entirely, soon enough.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:989
>          if (m_uncorrectedContentsImage && m_uncorrectedContentsImage.get() == newImage)

Someone should come back here and figure out:

1) Why we need to null check m_uncorrectedContentsImage. Doesn’t seem we need to.
2) Why we have to say .get() here; that should not be required.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:85
>      static CGColorSpaceRef sRGBSpace = CGColorSpaceCreateWithName(kCGColorSpaceSRGB);

Are you sure this works on iOS? Did you test it? I seem to recall having a problem with this. Should make sure we also run tests on iOS, not just Mac.

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:188
> +        RetainPtr<CGContextRef> context = adoptCF(CGBitmapContextCreate(0, logicalSize().width(), logicalSize().height(), 8, 4 * logicalSize().width(), deviceRGBColorSpaceRef() /* FIXME */, kCGImageAlphaPremultipliedLast));

I don’t think this FIXME is valuable.

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:233
>      UNUSED_PARAM(useLowQualityScale);

We should leave out the argument name rather than using UNUSED_PARAM.

> Source/WebCore/platform/graphics/win/GraphicsContextCGWin.cpp:147
> +    CGColorRef colorRef = color.isValid() ? cachedCGColor(color) : 0;

Surprised you didn’t do the nullptr thing here.

[Simon Fraser (smfr)]

(In reply to comment #8)
> Comment on attachment 264939 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264939&action=review
> 
> Looking forward to merging this with my mega-patch after you land it ;)
> 
> > Source/WebCore/platform/graphics/ImageBuffer.h:72
> > +    static std::unique_ptr<ImageBuffer> create(const FloatSize& size, RenderingMode renderingMode, float resolutionScale = 1, ColorSpace colorSpace = ColorSpaceDeviceRGB /* FIXME sRGB */)
> 
> I don’t think you should land this change as part of this patch. The comment
> isn’t all that valuable. We’re going to remove the ColorSpaceDeviceRGB
> constant entirely, soon enough.

Agreed; I did't mean to include the ImageBuffer creation changes as part of the patch. I'll do them separately (possibly landing first).

> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:989
> >          if (m_uncorrectedContentsImage && m_uncorrectedContentsImage.get() == newImage)
> 
> Someone should come back here and figure out:

Yes!

> 1) Why we need to null check m_uncorrectedContentsImage. Doesn’t seem we
> need to.
> 2) Why we have to say .get() here; that should not be required.
> 
> > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:85
> >      static CGColorSpaceRef sRGBSpace = CGColorSpaceCreateWithName(kCGColorSpaceSRGB);
> 
> Are you sure this works on iOS? Did you test it? I seem to recall having a
> problem with this. Should make sure we also run tests on iOS, not just Mac.

I will test.

> > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:188
> > +        RetainPtr<CGContextRef> context = adoptCF(CGBitmapContextCreate(0, logicalSize().width(), logicalSize().height(), 8, 4 * logicalSize().width(), deviceRGBColorSpaceRef() /* FIXME */, kCGImageAlphaPremultipliedLast));
> 
> I don’t think this FIXME is valuable.

I'll revert this change.

> > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:233
> >      UNUSED_PARAM(useLowQualityScale);
> 
> We should leave out the argument name rather than using UNUSED_PARAM.
> 
> > Source/WebCore/platform/graphics/win/GraphicsContextCGWin.cpp:147
> > +    CGColorRef colorRef = color.isValid() ? cachedCGColor(color) : 0;
> 
> Surprised you didn’t do the nullptr thing here.

Will do.

[Simon Fraser (smfr)]

Created attachment 264991 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 264992 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 264994 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 264995 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 264995 [details]
Patch

Darin reviewed this.

[Csaba Osztrogonác]

It broke the Windows build.

[David Kilzer (:ddkilzer)]

(In reply to comment #15)
> It broke the Windows build.

Simon, was there a reason why you couldn't let the commit queue land this?  It would have prevented breaking the Windows build.

[David Kilzer (:ddkilzer)]

Via <https://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/91957/steps/compile-webkit/logs/stdio>:

c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderThemeWin.cpp(895): error C2039: 'colorSpace': is not a member of 'WebCore::RenderStyle' (compiling source file C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\rendering\RenderingAllInOne.cpp) [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj]
  c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderMediaControls.h(42): note: see declaration of 'WebCore::RenderStyle' (compiling source file C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\rendering\RenderingAllInOne.cpp)
c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderThemeWin.cpp(943): error C2039: 'colorSpace': is not a member of 'WebCore::RenderStyle' (compiling source file C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\rendering\RenderingAllInOne.cpp) [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj]
  c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderMediaControls.h(42): note: see declaration of 'WebCore::RenderStyle' (compiling source file C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\rendering\RenderingAllInOne.cpp)
c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderThemeWin.cpp(978): error C2039: 'colorSpace': is not a member of 'WebCore::RenderStyle' (compiling source file C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\rendering\RenderingAllInOne.cpp) [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj]
  c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderMediaControls.h(42): note: see declaration of 'WebCore::RenderStyle' (compiling source file C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\rendering\RenderingAllInOne.cpp)

C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\platform\graphics\ca\win\PlatformCALayerWin.cpp(930): error C2660: 'WebCore::GraphicsContext::setFillColor': function does not take 2 arguments [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj]
  JSWebKitPlaybackTargetAvailabilityEvent.cpp

C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\platform\graphics\win\ImageCGWin.cpp(97): error C2065: 'styleColorSpace': undeclared identifier [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-debug\build\Source\WebCore\platform\graphics\win\ImageCGWin.cpp(105): error C2065: 'styleColorSpace': undeclared identifier [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj]

[David Kilzer (:ddkilzer)]

Attempted build fix in r192143:  <http://trac.webkit.org/r192143>

[Simon Fraser (smfr)]

And another in https://trac.webkit.org/changeset/192144

This is what happens when Windows EWS is broken.

[Alexey Proskuryakov]

In the future, please do tell when you see EWS not processing patches. 

Is it right that this bug is currently open?

[David Kilzer (:ddkilzer)]

Last build fix in r192145:  <http://trac.webkit.org/r192145>

[Csaba Osztrogonác]

(In reply to comment #16)
> Simon, was there a reason why you couldn't let the commit queue land this? 
> It would have prevented breaking the Windows build.

Commit queue guarantees only the patch builds and tests pass on Apple Mac
platform. But if you land your patch manually, nobody prevents the author
to check https://build.webkit.org/waterfall if everything works fine.

( just to document, the original patch landed in http://trac.webkit.org/changeset/192140 )