Bug 150967

Summary: Remove ColorSpace argument to all the drawing calls
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: PlatformAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bdakin, darin, ddkilzer, ossy, simon.fraser, thorton
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 150305    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch simon.fraser: review+

Description Simon Fraser (smfr) 2015-11-05 22:30:20 PST
Remove ColorSpace argument to all the drawing calls
Comment 1 Simon Fraser (smfr) 2015-11-05 22:34:17 PST
Created attachment 264917 [details]
Patch
Comment 2 Simon Fraser (smfr) 2015-11-05 23:24:13 PST
Created attachment 264919 [details]
Patch
Comment 3 Tim Horton 2015-11-06 05:24:55 PST
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)
Comment 4 Simon Fraser (smfr) 2015-11-06 08:11:03 PST
I don't want to change any ImageBuffer or filter stuff at this point. This is just removing input params that are always ColorSpaceSRGB.
Comment 5 Simon Fraser (smfr) 2015-11-06 08:11:39 PST
...always ColorSpaceSRGB (hard-coded in style) or ColorSpaceDeviceRGB (hardcoded at many call sites).
Comment 6 Darin Adler 2015-11-06 08:16:00 PST
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.
Comment 7 Simon Fraser (smfr) 2015-11-06 08:29:41 PST
Created attachment 264939 [details]
Patch
Comment 8 Darin Adler 2015-11-06 08:39:38 PST
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.
Comment 9 Simon Fraser (smfr) 2015-11-06 08:46:27 PST
(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.
Comment 10 Simon Fraser (smfr) 2015-11-06 19:23:40 PST
Created attachment 264991 [details]
Patch
Comment 11 Simon Fraser (smfr) 2015-11-06 20:03:28 PST
Created attachment 264992 [details]
Patch
Comment 12 Simon Fraser (smfr) 2015-11-06 20:20:43 PST
Created attachment 264994 [details]
Patch
Comment 13 Simon Fraser (smfr) 2015-11-06 20:46:02 PST
Created attachment 264995 [details]
Patch
Comment 14 Simon Fraser (smfr) 2015-11-07 10:16:34 PST
Comment on attachment 264995 [details]
Patch

Darin reviewed this.
Comment 15 Csaba Osztrogonác 2015-11-08 01:11:46 PST
It broke the Windows build.
Comment 16 David Kilzer (:ddkilzer) 2015-11-08 07:03:25 PST
(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.
Comment 17 David Kilzer (:ddkilzer) 2015-11-08 07:05:23 PST
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]
Comment 18 David Kilzer (:ddkilzer) 2015-11-08 07:34:00 PST
Attempted build fix in r192143:  <http://trac.webkit.org/r192143>
Comment 19 Simon Fraser (smfr) 2015-11-08 09:11:59 PST
And another in https://trac.webkit.org/changeset/192144

This is what happens when Windows EWS is broken.
Comment 20 Alexey Proskuryakov 2015-11-08 09:37:53 PST
In the future, please do tell when you see EWS not processing patches. 

Is it right that this bug is currently open?
Comment 21 David Kilzer (:ddkilzer) 2015-11-08 17:54:58 PST
Last build fix in r192145:  <http://trac.webkit.org/r192145>
Comment 22 Csaba Osztrogonác 2015-11-09 07:57:46 PST
(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 )