Bug 158983 – <img> with a wide gamut PDF does not display using a wide gamut color space

• Status: RESOLVED
• Resolution: FIXED
• Component: PDF

[Antoine Quint]

If you have an <img> element pointing to a wide gamut PDF resource, the displayed PDF will not display the full color spectrum of the document.

[Antoine Quint]

<rdar://problem/25720247>

[Antoine Quint]

Created attachment 281731 [details]
Patch

[WebKit Commit Bot]

Attachment 281731 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/ImageBuffer.h:83:  The parameter name "colorSpace" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antoine Quint]

Created attachment 281734 [details]
Patch

[WebKit Commit Bot]

Attachment 281734 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/ImageBuffer.h:83:  The parameter name "colorSpace" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antoine Quint]

Created attachment 281735 [details]
Patch

[Build Bot]

Comment on attachment 281735 [details]
Patch

Attachment 281735 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1541858

New failing tests:
fast/css/webkit-mask-crash-td-2.html
fast/css/webkit-mask-crash-fieldset-legend.html
fast/css/webkit-mask-crash-td.html
fast/css/webkit-mask-crash-figure.html
fast/css/webkit-mask-crash-table.html

[Build Bot]

Created attachment 281739 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 281735 [details]
Patch

Attachment 281735 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1541861

New failing tests:
fast/css/webkit-mask-crash-td-2.html
fast/css/webkit-mask-crash-fieldset-legend.html
fast/css/webkit-mask-crash-td.html
fast/css/webkit-mask-crash-figure.html
fast/css/webkit-mask-crash-table.html

[Build Bot]

Created attachment 281740 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 281735 [details]
Patch

Attachment 281735 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1541860

New failing tests:
fast/css/webkit-mask-crash-td-2.html
fast/css/webkit-mask-crash-fieldset-legend.html
fast/css/webkit-mask-crash-td.html
fast/css/webkit-mask-crash-figure.html
fast/css/webkit-mask-crash-table.html

[Build Bot]

Created attachment 281741 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Antoine Quint]

I don't think these test failures have anything to do with my patch.

[Simon Fraser (smfr)]

Comment on attachment 281735 [details]
Patch

You can't do it this way. You need to get the colorspace from the target context; it's the screen colorspace, and may be something other than sRGB or P3.

[Antoine Quint]

(In reply to comment #14)
> Comment on attachment 281735 [details]
> Patch
> 
> You can't do it this way. You need to get the colorspace from the target
> context; it's the screen colorspace, and may be something other than sRGB or
> P3.

Which part of the code are you referring to exactly? Are you referring to this line?

    context.setSupportsExtendedColor(screenSupportsExtendedColor(this));

If so, the call to screenSupportsExtendedColor() does query the screen's color space.

[Dean Jackson]

Comment on attachment 281735 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:4141
> +#if PLATFORM(MAC)
> +    context.setSupportsExtendedColor(screenSupportsExtendedColor(this));
> +#endif

I don't think we want to call this every time we paint. screenSupportsExtendedColor on macOS copies the ICC profile from the screen's space, then creates a temporary colorsync profile to query the value. We should have some way to cache the value.

> Source/WebCore/platform/graphics/GraphicsContext.h:321
> +    void setSupportsExtendedColor(bool);

You should provide a stub implementation of this too.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:105
> +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200
> +    static CGColorSpaceRef displayP3Space = CGColorSpaceCreateWithName(kCGColorSpaceDisplayP3);
> +    if (!displayP3Space)
> +        displayP3Space = deviceRGBColorSpaceRef();
> +#else
> +    static CGColorSpaceRef displayP3Space = deviceRGBColorSpaceRef();
> +#endif
> +    return displayP3Space;
> +}

kCGColorSpaceDisplayP3 is available from 10.10 onwards, so I think you just need #if PLATFORM(MAC) (to exclude Windows). But why are you limiting it to MAC? It's been in iOS since 9.0, so I think you really want #if !PLATFORM(WINDOWS)

This would also avoid returning the deviceRGB space in most cases, which I don't like.

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:174
> +    ColorSpace colorSpace = context.supportsExtendedColor() ? ColorSpaceDisplayP3 : ColorSpaceSRGB;
> +    m_cachedImageBuffer = ImageBuffer::createCompatibleBuffer(FloatRect(enclosingIntRect(dstRect)).size(), context, true, colorSpace);

I think what Simon is getting at here is that we should create an ImageBuffer that matches the Screen, no matter what it is or what we support.

You could imagine someone with a display set to AdobeRGB, and a PDF that has content in AdobeRGB (which is very likely to happen, since AdobeRGB is better suited for print workflows than Display P3). We probably don't want to draw into a P3 space and then have is converted to AdobeRGB. 

What you have might be ok for now though, because (a) it's better than the existing situation, (b) modern Apple displays are P3, (c) we would have to add code to support a bunch of common color profiles into GraphicsContext

[Alexey Proskuryakov]

> I don't think these test failures have anything to do with my patch.

They are certainly caused by the patch. You can tell by looking inside the attached results archives:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00000001031b0ce7 WebCore::GraphicsContext::setSupportsExtendedColor(bool) + 7
1   com.apple.WebCore             	0x000000010317d8b4 WebCore::FrameView::paintContents(WebCore::GraphicsContext&, WebCore::IntRect const&) + 244
2   com.apple.WebCore             	0x000000010317d76f WebCore::FrameView::paintControlTints() + 175

[Antoine Quint]

Created attachment 281829 [details]
Patch

[Antoine Quint]

Created attachment 281831 [details]
Patch

[Antoine Quint]

Created attachment 281832 [details]
Patch

[Antoine Quint]

(In reply to comment #16)
> Comment on attachment 281735 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281735&action=review
> 
> > Source/WebCore/page/FrameView.cpp:4141
> > +#if PLATFORM(MAC)
> > +    context.setSupportsExtendedColor(screenSupportsExtendedColor(this));
> > +#endif
> 
> I don't think we want to call this every time we paint.
> screenSupportsExtendedColor on macOS copies the ICC profile from the
> screen's space, then creates a temporary colorsync profile to query the
> value. We should have some way to cache the value.

I'll look into this.

> > Source/WebCore/platform/graphics/GraphicsContext.h:321
> > +    void setSupportsExtendedColor(bool);
> 
> You should provide a stub implementation of this too.

There isn't one for GraphicsContext::setIsAcceleratedContext() so I didn't add one for 
setSupportsExtendedColor(). Is it necessary?
 
> > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:105
> > +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200
> > +    static CGColorSpaceRef displayP3Space = CGColorSpaceCreateWithName(kCGColorSpaceDisplayP3);
> > +    if (!displayP3Space)
> > +        displayP3Space = deviceRGBColorSpaceRef();
> > +#else
> > +    static CGColorSpaceRef displayP3Space = deviceRGBColorSpaceRef();
> > +#endif
> > +    return displayP3Space;
> > +}
> 
> kCGColorSpaceDisplayP3 is available from 10.10 onwards, so I think you just
> need #if PLATFORM(MAC) (to exclude Windows). But why are you limiting it to
> MAC? It's been in iOS since 9.0, so I think you really want #if
> !PLATFORM(WINDOWS)
> 
> This would also avoid returning the deviceRGB space in most cases, which I
> don't like.

I've changed it to #if PLATFORM(COCOA). Same for the call to context.setSupportsExtendedColor() in FrameView::paintContents().

> > Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:174
> > +    ColorSpace colorSpace = context.supportsExtendedColor() ? ColorSpaceDisplayP3 : ColorSpaceSRGB;
> > +    m_cachedImageBuffer = ImageBuffer::createCompatibleBuffer(FloatRect(enclosingIntRect(dstRect)).size(), context, true, colorSpace);
> 
> I think what Simon is getting at here is that we should create an
> ImageBuffer that matches the Screen, no matter what it is or what we support.
> 
> You could imagine someone with a display set to AdobeRGB, and a PDF that has
> content in AdobeRGB (which is very likely to happen, since AdobeRGB is
> better suited for print workflows than Display P3). We probably don't want
> to draw into a P3 space and then have is converted to AdobeRGB. 
> 
> What you have might be ok for now though, because (a) it's better than the
> existing situation, (b) modern Apple displays are P3, (c) we would have to
> add code to support a bunch of common color profiles into GraphicsContext

Yes, we would indeed have to add a bunch of new color spaces in ColorSpace.h, or fall back to sRGB if we didn't, which would essentially be the same as what this patch does in a simpler way.

[Antoine Quint]

(In reply to comment #17)
> > I don't think these test failures have anything to do with my patch.
> 
> They are certainly caused by the patch. You can tell by looking inside the
> attached results archives:
> 
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0   com.apple.WebCore             	0x00000001031b0ce7
> WebCore::GraphicsContext::setSupportsExtendedColor(bool) + 7
> 1   com.apple.WebCore             	0x000000010317d8b4
> WebCore::FrameView::paintContents(WebCore::GraphicsContext&,
> WebCore::IntRect const&) + 244
> 2   com.apple.WebCore             	0x000000010317d76f
> WebCore::FrameView::paintControlTints() + 175

I fixed this crash.

[Antoine Quint]

Created attachment 281833 [details]
Patch

[Antoine Quint]

Created attachment 281834 [details]
Patch

[Antoine Quint]

(In reply to comment #16)
> Comment on attachment 281735 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281735&action=review
> 
> kCGColorSpaceDisplayP3 is available from 10.10 onwards, so I think you just
> need #if PLATFORM(MAC) (to exclude Windows). But why are you limiting it to
> MAC? It's been in iOS since 9.0, so I think you really want #if
> !PLATFORM(WINDOWS)

Actually, kCGColorSpaceDisplayP3 is available on macOS 10.11.2 and iOS 9.3 onwards per the CGColorSpace.h header.

[Simon Fraser (smfr)]

Comment on attachment 281834 [details]
Patch

I still think this is the wrong approach. You need to get the destination context's colorSpace.

[Dean Jackson]

(In reply to comment #26)

> I still think this is the wrong approach. You need to get the destination
> context's colorSpace.

I don't think the destination's colorSpace matters, assuming that ImageBuffer is color managed correctly. What's more important is making sure we tag the ImageBuffer used for the PDF with the most appropriate color space.

We really need to know in advance if the PDF goes outside the sRGB gamut. If it doesn't, and we create an 8bpp P3 buffer, then we'll lose color accuracy painting sRGB into it. If it does go outside sRGB, we're probably better off creating a deeper buffer and tagging it with extended sRGB (again assuming that CG + PDF knows how to render into such a buffer). Maybe just an extended sRGB or P3 profile will be enough.

[Simon Fraser (smfr)]

(In reply to comment #27)
> (In reply to comment #26)
> 
> > I still think this is the wrong approach. You need to get the destination
> > context's colorSpace.
> 
> I don't think the destination's colorSpace matters, assuming that
> ImageBuffer is color managed correctly. What's more important is making sure
> we tag the ImageBuffer used for the PDF with the most appropriate color
> space.

But there's no point making a P3 buffer for the PDF if the final destination is sRGB.

In general, we want intermediate buffers to match the destination context. It should
behave as if those buffers weren't really in the rendering path.

> We really need to know in advance if the PDF goes outside the sRGB gamut. If
> it doesn't, and we create an 8bpp P3 buffer, then we'll lose color accuracy
> painting sRGB into it. If it does go outside sRGB, we're probably better off
> creating a deeper buffer and tagging it with extended sRGB (again assuming
> that CG + PDF knows how to render into such a buffer). Maybe just an
> extended sRGB or P3 profile will be enough.

If we just match the destination context, we always get it right.

[Dean Jackson]

(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #26)
> > 
> > > I still think this is the wrong approach. You need to get the destination
> > > context's colorSpace.
> > 
> > I don't think the destination's colorSpace matters, assuming that
> > ImageBuffer is color managed correctly. What's more important is making sure
> > we tag the ImageBuffer used for the PDF with the most appropriate color
> > space.
> 
> But there's no point making a P3 buffer for the PDF if the final destination
> is sRGB.

Right, sorry, I wasn't clear. We never need a P3 buffer on an sRGB device. Similarly, we don't need a P3 buffer if the PDF content is just sRGB. Aren't we just better off letting CG do the work in that case? Otherwise we're drawing a narrow image into a wider buffer, but at the same bit depth, ending up with reduced color accuracy. Instead if we passed the sRGB buffer to the window server (or CG), it might have a deeper backing store when it draws.

If the PDF content is outside sRGB, then we do want a P3 buffer (although maybe we want extended sRGB, since a color-managed xPDF is likely to be AdobeRGB). 

> 
> In general, we want intermediate buffers to match the destination context.
> It should
> behave as if those buffers weren't really in the rendering path.
> 
> > We really need to know in advance if the PDF goes outside the sRGB gamut. If
> > it doesn't, and we create an 8bpp P3 buffer, then we'll lose color accuracy
> > painting sRGB into it. If it does go outside sRGB, we're probably better off
> > creating a deeper buffer and tagging it with extended sRGB (again assuming
> > that CG + PDF knows how to render into such a buffer). Maybe just an
> > extended sRGB or P3 profile will be enough.
> 
> If we just match the destination context, we always get it right.

Do we? But we don't create P3 buffers for all our content on the Web, because it's supposed to be sRGB. If we have 8bpp, then we have 256 steps for each channel. Drawing sRGB content into an sRGB buffer means we keep all 256 steps. Drawing sRGB into a P3 buffer means we can only use 240-ish steps in the red channel, because the extra space covers the stuff outside of sRGB. Even if that P3 buffer goes through intermediate P3 buffers, and is drawn to a P3 display, we've lost data. If we knew in advance it was only sRGB, and used an sRGB buffer, and that buffer made it to the display, it could be color matched there, hopefully without loss.

But, honestly, I'm making all this up as I go along, so it could be all wrong.

[Dean Jackson]

My suggestion:

- Device is sRGB
  - all PDF buffers are sRGB, no matter what their content

- Device is P3 (supportsExtendedColor)
  - If PDF contains content outside of sRGB, create a P3 buffer (or an extended sRGB buffer, or whatever the target device is)
  - Otherwise, create an sRGB buffer

At the moment we only detect sRGB and P3 devices right? And we don't have other named ColorProfiles. What should we do if the target device is something else, like AdobeRGB?

Also, a followup patch might be to create a deeper buffer if the PDF has out-of-sRGB content.

[Dean Jackson]

Simon and I talked over IRC, and he convinced me to agree with him :)

For now, ignore what the PDF is and always create a buffer with the device colorspace. On iOS, that will be sRGB or P3. On Mac, that could be anything :) 

Ultimately we'll want to create deeper buffers for such content, at which time we might start being smarter about what spaces we use.

[Antoine Quint]

So, should all ImageBuffers be created with a color space matching the target display color space? We could remove the color space arguments at this stage.

[Antoine Quint]

Created attachment 281907 [details]
Patch

[Antoine Quint]

Created attachment 281912 [details]
Patch

[Dean Jackson]

Comment on attachment 281912 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:97
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000)

Sucks that we need that IPHONE MIN thing, since we'll only ship on iOS 10. It's just that we still have iOS 9 builders.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1682
> +    // FIXME

FIXME

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:206
> +        if (CGPDFPageContainsWideGamutContent(page))
> +            return true;

Is this SPI? Did you declare it in an SPI.h file for Open Source builders?

[Antoine Quint]

Created attachment 282577 [details]
Patch

[Antoine Quint]

Created attachment 282582 [details]
Patch

[Build Bot]

Comment on attachment 282582 [details]
Patch

Attachment 282582 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1609259

New failing tests:
fast/backgrounds/scaled-gradient-background.html
fast/canvas/2d.fillText.gradient.html

[Build Bot]

Created attachment 282589 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282582 [details]
Patch

Attachment 282582 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1609268

New failing tests:
fast/backgrounds/scaled-gradient-background.html
fast/canvas/2d.fillText.gradient.html

[Build Bot]

Created attachment 282592 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282582 [details]
Patch

Attachment 282582 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1609252

New failing tests:
fast/backgrounds/scaled-gradient-background.html
fast/canvas/2d.fillText.gradient.html

[Build Bot]

Created attachment 282593 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282582 [details]
Patch

Attachment 282582 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1609262

New failing tests:
fast/backgrounds/scaled-gradient-background.html
css3/masking/mask-luminance-gradient.html
css3/masking/mask-repeat-space-border.html
fast/css/cascade/background-clip-and-webkit-background-clip-cascade-order.html
fast/gradients/background-image-repeat-space.html
svg/repaint/buffered-rendering-static-image.html

[Build Bot]

Created attachment 282594 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Tim Horton]

Comment on attachment 282582 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageBuffer.cpp:174
> +#if USE(CG)

This is fairly gross. Also what happened to hasAlpha!

> Source/WebCore/platform/graphics/ImageBuffer.cpp:176
> +    auto buffer = ImageBuffer::createCompatibleBufferWithGraphicsContext(scaledSize, context);

Doesn't GraphicsContext already have a createCompatibleBuffer? Shouldn't you be using/fixing that?

> Source/WebCore/platform/graphics/ImageBuffer.h:177
> +    ImageBuffer(const FloatSize&, float resolutionScale, CGColorSpaceRef, RenderingMode, bool& success);

Sam was not a fan of this and would like a real ColorSpace wrapper (we've outgrown the enum, I think).

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:78
> +    CGColorSpaceRef colorSpace = CGContextCopyDeviceColorSpace(context.platformContext());

This looks like a leak?

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:80
> +    std::unique_ptr<ImageBuffer> buffer(new ImageBuffer(size, resolutionScale, colorSpace, renderingMode, success));

No a make_unique? Do we not do that anymore for some reason?

> Source/WebCore/platform/graphics/cocoa/IOSurface.h:111
> +    CGColorSpaceRef m_colorSpace;

Doesn't this need to be a RetainPtr now??

[Simon Fraser (smfr)]

Comment on attachment 282582 [details]
Patch

r- based on tim's comments.

[Antoine Quint]

(In reply to comment #46)
> Comment on attachment 282582 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282582&action=review
> 
> > Source/WebCore/platform/graphics/ImageBuffer.cpp:174
> > +#if USE(CG)
> 
> This is fairly gross. Also what happened to hasAlpha!

Maybe we should instead move the implementation of ImageBuffer::createCompatibleBuffer(const FloatSize& size, const GraphicsContext& context, bool hasAlpha) outside of ImageBuffer.cpp and make implementations platform-specific so that we didn't have to have a CG-specific code branch here.

As to hasAlpha, it's actually never used, if you look at ImageBuffer::createCompatibleBuffer(const FloatSize& size, float resolutionScale, ColorSpace colorSpace, const GraphicsContext& context, bool), hasAlpha is silently dropped too.

> > Source/WebCore/platform/graphics/ImageBuffer.cpp:176
> > +    auto buffer = ImageBuffer::createCompatibleBufferWithGraphicsContext(scaledSize, context);
> 
> Doesn't GraphicsContext already have a createCompatibleBuffer? Shouldn't you
> be using/fixing that?

Not that I'm aware. Which method are you thinking about?

> > Source/WebCore/platform/graphics/ImageBuffer.h:177
> > +    ImageBuffer(const FloatSize&, float resolutionScale, CGColorSpaceRef, RenderingMode, bool& success);
> 
> Sam was not a fan of this and would like a real ColorSpace wrapper (we've
> outgrown the enum, I think).

With this patch, we simply match the color space as configured by the user, which can be just about anything. I don't think it makes sense to keep going with named spaces in an enum.

I'd like to look at the introduction of a ColorSpace wrapper as a separate task as this would be quite involved.

I'll look at the rest of your comments as I rev up the patch, thanks Tim.

[Tim Horton]

(In reply to comment #48)
> (In reply to comment #46)
> > Comment on attachment 282582 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=282582&action=review
> > 
> > > Source/WebCore/platform/graphics/ImageBuffer.cpp:174
> > > +#if USE(CG)
> > 
> > This is fairly gross. Also what happened to hasAlpha!
> 
> Maybe we should instead move the implementation of
> ImageBuffer::createCompatibleBuffer(const FloatSize& size, const
> GraphicsContext& context, bool hasAlpha) outside of ImageBuffer.cpp and make
> implementations platform-specific so that we didn't have to have a
> CG-specific code branch here.

Maybe!

> As to hasAlpha, it's actually never used, if you look at
> ImageBuffer::createCompatibleBuffer(const FloatSize& size, float
> resolutionScale, ColorSpace colorSpace, const GraphicsContext& context,
> bool), hasAlpha is silently dropped too.

Heh.

> > > Source/WebCore/platform/graphics/ImageBuffer.cpp:176
> > > +    auto buffer = ImageBuffer::createCompatibleBufferWithGraphicsContext(scaledSize, context);
> > 
> > Doesn't GraphicsContext already have a createCompatibleBuffer? Shouldn't you
> > be using/fixing that?
> 
> Not that I'm aware. Which method are you thinking about?

It got removed recently! See: https://trac.webkit.org/changeset/201629/trunk/Source/WebCore/platform/graphics/GraphicsContext.h

> > > Source/WebCore/platform/graphics/ImageBuffer.h:177
> > > +    ImageBuffer(const FloatSize&, float resolutionScale, CGColorSpaceRef, RenderingMode, bool& success);
> > 
> > Sam was not a fan of this and would like a real ColorSpace wrapper (we've
> > outgrown the enum, I think).
> 
> With this patch, we simply match the color space as configured by the user,
> which can be just about anything. I don't think it makes sense to keep going
> with named spaces in an enum.

Right, of course.

> I'd like to look at the introduction of a ColorSpace wrapper as a separate
> task as this would be quite involved.

Agreed.

> I'll look at the rest of your comments as I rev up the patch, thanks Tim.

[Antoine Quint]

Created attachment 282772 [details]
Patch

[Build Bot]

Comment on attachment 282772 [details]
Patch

Attachment 282772 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1629806

New failing tests:
fast/backgrounds/scaled-gradient-background.html
fast/canvas/2d.fillText.gradient.html

[Build Bot]

Created attachment 282777 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282772 [details]
Patch

Attachment 282772 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1629814

New failing tests:
fast/backgrounds/scaled-gradient-background.html
fast/canvas/2d.fillText.gradient.html

[Build Bot]

Created attachment 282778 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282772 [details]
Patch

Attachment 282772 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1629820

New failing tests:
fast/backgrounds/scaled-gradient-background.html
css3/masking/mask-luminance-gradient.html
css3/masking/mask-repeat-space-border.html
fast/css/cascade/background-clip-and-webkit-background-clip-cascade-order.html
fast/gradients/background-image-repeat-space.html
svg/repaint/buffered-rendering-static-image.html

[Build Bot]

Created attachment 282780 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Build Bot]

Comment on attachment 282772 [details]
Patch

Attachment 282772 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1629813

New failing tests:
fast/backgrounds/scaled-gradient-background.html
fast/canvas/2d.fillText.gradient.html

[Build Bot]

Created attachment 282781 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Tim Horton]

Comment on attachment 282772 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:80
> +    RetainPtr<CGColorSpaceRef> colorSpace = CGContextCopyDeviceColorSpace(context.platformContext());

This is still a leak. Needs some adoption.

[Brent Fulgham]

Comment on attachment 282772 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:80
>> +    RetainPtr<CGColorSpaceRef> colorSpace = CGContextCopyDeviceColorSpace(context.platformContext());
> 
> This is still a leak. Needs some adoption.

Note: CG on Windows doesn't support this at present. You may just need to create an sRGB color space if building for Windows.

[Antoine Quint]

Created attachment 282871 [details]
Patch

[Build Bot]

Comment on attachment 282871 [details]
Patch

Attachment 282871 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1634988

New failing tests:
fast/backgrounds/scaled-gradient-background.html
fast/canvas/2d.fillText.gradient.html

[Build Bot]

Created attachment 282874 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282871 [details]
Patch

Attachment 282871 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1634993

New failing tests:
fast/backgrounds/scaled-gradient-background.html
fast/canvas/2d.fillText.gradient.html

[Build Bot]

Created attachment 282875 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282871 [details]
Patch

Attachment 282871 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1635001

New failing tests:
fast/backgrounds/scaled-gradient-background.html
fast/canvas/2d.fillText.gradient.html

[Build Bot]

Created attachment 282876 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 282871 [details]
Patch

Attachment 282871 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1635005

New failing tests:
fast/backgrounds/scaled-gradient-background.html
css3/masking/mask-luminance-gradient.html
css3/masking/mask-repeat-space-border.html
fast/css/cascade/background-clip-and-webkit-background-clip-cascade-order.html
fast/gradients/background-image-repeat-space.html
svg/repaint/buffered-rendering-static-image.html

[Build Bot]

Created attachment 282877 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Antoine Quint]

Created attachment 282890 [details]
Patch

[Antoine Quint]

Created attachment 282895 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 282895 [details]
Patch

Clearing flags on attachment: 282895

Committed r202867: <http://trac.webkit.org/changeset/202867>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 159491

[Ryan Haddad]

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

[Ryan Haddad]

Rolled out this change in https://trac.webkit.org/r202877.

Please see https://bugs.webkit.org/show_bug.cgi?id=159486 for crashlogs and links to test runs exhibiting the issue. 

It appears that ios-sim EWS caught this crash on one iteration of this patch before it was landed (https://bugs.webkit.org/show_bug.cgi?id=158983#c690), but it doesn't repro 100% of the time on release builds.

[Alexey Proskuryakov]

Two threads in vImage, perhaps manipulating the same data?

[Tim Horton]

(In reply to comment #77)
> Two threads in vImage, perhaps manipulating the same data?

I'm pretty sure vImage will happily use multiple threads, so I don't think that that is particularly alarming by itself (I see there are actually quite a number in some of the crash logs).

[Antoine Quint]

So the crash reproduces 100% of the time for me when using WebKitTester. However, it never reproduces when running the same debug WebCore with MobileSafari.

[Antoine Quint]

In WKT, we hit this assertion:

ASSERT(endx.unsafeGet() <= size.width())

[Antoine Quint]

Created attachment 283007 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 283007 [details]
Patch

Clearing flags on attachment: 283007

Committed r202927: <http://trac.webkit.org/changeset/202927>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

Comment on attachment 283007 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:88
> +    std::unique_ptr<ImageBuffer> buffer(new ImageBuffer(scaledSize, resolutionScale, colorSpace.get(), renderingMode, success));

Why are sending scaledSize to the ImageBuffer constructor? In ImageBuffer::ImageBuffer(), it is going to be scaled it with resolutionScale:

float scaledWidth = ceilf(resolutionScale * size.width());
float scaledHeight = ceilf(resolutionScale * size.height());

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:94
> +    buffer->context().scale(FloatSize(scaledSize.width() / size.width(), scaledSize.height() / size.height()));

Why do we need to do this scaling? The ImageBuffer is created with the scaledSize to avoid lower resolution with zooming. Also there are inconsistencies here:

In this function, scaledSize is calculated like this:
    scaledSize = ImageBuffer::compatibleBufferSize(size, context);

    which is almost equivalent to:
    scaledSize = size * context.scaleFactor();

But ImageBuffer::ImageBuffer(), m_size is calculated like this:
    m_size = IntSize(resolutionScale * size.width(), resolutionScale* size.width());

    which can be rewritten like this:
    m_size = IntSize(context.scaleFactor().width() * size.width(), context.scaleFactor().width() * size.width());

Then In this function when scaling the context we do this 
    buffer->context().scale(FloatSize(scaledSize.width() / size.width(), scaledSize.height() / size.height()));

    which can be rewritten like this:
    buffer->context().scale(context.scaleFactor());

I think we should we use "size * context.scaleFactor()" always instead of using "IntSize(context.scaleFactor().width() * size.width(), context.scaleFactor().width() * size.width())" sometimes.

[Dean Jackson]

(In reply to comment #84)
> Comment on attachment 283007 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283007&action=review
> 
> > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:88
> > +    std::unique_ptr<ImageBuffer> buffer(new ImageBuffer(scaledSize, resolutionScale, colorSpace.get(), renderingMode, success));
> 
> Why are sending scaledSize to the ImageBuffer constructor? In
> ImageBuffer::ImageBuffer(), it is going to be scaled it with resolutionScale:
> 
> float scaledWidth = ceilf(resolutionScale * size.width());
> float scaledHeight = ceilf(resolutionScale * size.height());

Yeah, this definitely seems wrong.


> > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:94
> > +    buffer->context().scale(FloatSize(scaledSize.width() / size.width(), scaledSize.height() / size.height()));
> 
> Why do we need to do this scaling? The ImageBuffer is created with the
> scaledSize to avoid lower resolution with zooming. Also there are
> inconsistencies here:
> 
> In this function, scaledSize is calculated like this:
>     scaledSize = ImageBuffer::compatibleBufferSize(size, context);
> 
>     which is almost equivalent to:
>     scaledSize = size * context.scaleFactor();
> 
> But ImageBuffer::ImageBuffer(), m_size is calculated like this:
>     m_size = IntSize(resolutionScale * size.width(), resolutionScale*
> size.width());
> 
>     which can be rewritten like this:
>     m_size = IntSize(context.scaleFactor().width() * size.width(),
> context.scaleFactor().width() * size.width());
> 
> Then In this function when scaling the context we do this 
>     buffer->context().scale(FloatSize(scaledSize.width() / size.width(),
> scaledSize.height() / size.height()));
> 
>     which can be rewritten like this:
>     buffer->context().scale(context.scaleFactor());
> 
> I think we should we use "size * context.scaleFactor()" always instead of
> using "IntSize(context.scaleFactor().width() * size.width(),
> context.scaleFactor().width() * size.width())" sometimes.

I agree.

[Dean Jackson]

(In reply to comment #85)
> (In reply to comment #84)
> > Comment on attachment 283007 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=283007&action=review
> > 
> > > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:88
> > > +    std::unique_ptr<ImageBuffer> buffer(new ImageBuffer(scaledSize, resolutionScale, colorSpace.get(), renderingMode, success));
> > 
> > Why are sending scaledSize to the ImageBuffer constructor? In
> > ImageBuffer::ImageBuffer(), it is going to be scaled it with resolutionScale:
> > 
> > float scaledWidth = ceilf(resolutionScale * size.width());
> > float scaledHeight = ceilf(resolutionScale * size.height());
> 
> Yeah, this definitely seems wrong.

Actually, it's not strictly wrong. The default ImageBuffer::createCompatibleBuffer does this as well.

The bug is that when you do this scaling, you should not then *also* pass in a resolution scale factor to the ImageBuffer constructor.

[Dean Jackson]

> > But ImageBuffer::ImageBuffer(), m_size is calculated like this:
> >     m_size = IntSize(resolutionScale * size.width(), resolutionScale*
> > size.width());
> > 
> >     which can be rewritten like this:
> >     m_size = IntSize(context.scaleFactor().width() * size.width(),
> > context.scaleFactor().width() * size.width());

Not really, since we don't have the context inside the constructor. We create another context later.

We also call applyDeviceScaleFactor(), but this is so that we can have another entry point that creates a highDPI buffer without going through createCompatibleBuffer.