Bug 158983 - <img> with a wide gamut PDF does not display using a wide gamut color space
Summary: <img> with a wide gamut PDF does not display using a wide gamut color space
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: PDF (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
: 159486 (view as bug list)
Depends on: 159491
Blocks:
  Show dependency treegraph
 
Reported: 2016-06-21 04:32 PDT by Antoine Quint
Modified: 2016-07-19 18:25 PDT (History)
9 users (show)

See Also:


Attachments
Patch (11.31 KB, patch)
2016-06-21 05:03 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (11.19 KB, patch)
2016-06-21 05:44 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (11.18 KB, patch)
2016-06-21 06:08 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-yosemite (1.22 MB, application/zip)
2016-06-21 07:04 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (967.62 KB, application/zip)
2016-06-21 07:09 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-yosemite (1.71 MB, application/zip)
2016-06-21 07:11 PDT, Build Bot
no flags Details
Patch (11.46 KB, patch)
2016-06-22 02:40 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (11.42 KB, patch)
2016-06-22 02:52 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (11.42 KB, patch)
2016-06-22 02:56 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (11.52 KB, patch)
2016-06-22 03:39 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (11.52 KB, patch)
2016-06-22 03:43 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (35.27 KB, patch)
2016-06-23 07:10 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (35.29 KB, patch)
2016-06-23 09:35 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (21.41 KB, patch)
2016-07-01 14:13 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (21.76 KB, patch)
2016-07-01 14:30 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (910.07 KB, application/zip)
2016-07-01 15:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (931.36 KB, application/zip)
2016-07-01 15:23 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (1.63 MB, application/zip)
2016-07-01 15:29 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.65 MB, application/zip)
2016-07-01 15:36 PDT, Build Bot
no flags Details
Patch (30.95 KB, patch)
2016-07-05 05:22 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (974.93 KB, application/zip)
2016-07-05 06:12 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (782.31 KB, application/zip)
2016-07-05 06:17 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.83 MB, application/zip)
2016-07-05 06:25 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-yosemite (1.44 MB, application/zip)
2016-07-05 06:25 PDT, Build Bot
no flags Details
Patch (30.91 KB, patch)
2016-07-06 03:11 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (814.39 KB, application/zip)
2016-07-06 04:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (827.16 KB, application/zip)
2016-07-06 04:05 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-yosemite (1.45 MB, application/zip)
2016-07-06 04:13 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.60 MB, application/zip)
2016-07-06 04:15 PDT, Build Bot
no flags Details
Patch (31.91 KB, patch)
2016-07-06 07:00 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (32.85 KB, patch)
2016-07-06 08:11 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (33.64 KB, patch)
2016-07-07 05:26 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2016-06-21 04:32:33 PDT
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.
Comment 1 Antoine Quint 2016-06-21 04:32:51 PDT
<rdar://problem/25720247>
Comment 2 Antoine Quint 2016-06-21 05:03:50 PDT
Created attachment 281731 [details]
Patch
Comment 3 WebKit Commit Bot 2016-06-21 05:04:40 PDT
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.
Comment 4 Antoine Quint 2016-06-21 05:44:15 PDT
Created attachment 281734 [details]
Patch
Comment 5 WebKit Commit Bot 2016-06-21 05:46:57 PDT
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.
Comment 6 Antoine Quint 2016-06-21 06:08:12 PDT
Created attachment 281735 [details]
Patch
Comment 7 Build Bot 2016-06-21 07:04:38 PDT
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
Comment 8 Build Bot 2016-06-21 07:04:41 PDT
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
Comment 9 Build Bot 2016-06-21 07:09:18 PDT
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
Comment 10 Build Bot 2016-06-21 07:09:21 PDT
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
Comment 11 Build Bot 2016-06-21 07:11:04 PDT
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
Comment 12 Build Bot 2016-06-21 07:11:07 PDT
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
Comment 13 Antoine Quint 2016-06-21 07:21:26 PDT
I don't think these test failures have anything to do with my patch.
Comment 14 Simon Fraser (smfr) 2016-06-21 08:45:19 PDT
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.
Comment 15 Antoine Quint 2016-06-21 08:54:38 PDT
(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.
Comment 16 Dean Jackson 2016-06-21 14:30:11 PDT
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
Comment 17 Alexey Proskuryakov 2016-06-21 20:28:14 PDT
> 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
Comment 18 Antoine Quint 2016-06-22 02:40:45 PDT
Created attachment 281829 [details]
Patch
Comment 19 Antoine Quint 2016-06-22 02:52:43 PDT
Created attachment 281831 [details]
Patch
Comment 20 Antoine Quint 2016-06-22 02:56:05 PDT
Created attachment 281832 [details]
Patch
Comment 21 Antoine Quint 2016-06-22 03:00:49 PDT
(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.
Comment 22 Antoine Quint 2016-06-22 03:01:05 PDT
(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.
Comment 23 Antoine Quint 2016-06-22 03:39:29 PDT
Created attachment 281833 [details]
Patch
Comment 24 Antoine Quint 2016-06-22 03:43:11 PDT
Created attachment 281834 [details]
Patch
Comment 25 Antoine Quint 2016-06-22 03:45:24 PDT
(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.
Comment 26 Simon Fraser (smfr) 2016-06-22 10:17:46 PDT
Comment on attachment 281834 [details]
Patch

I still think this is the wrong approach. You need to get the destination context's colorSpace.
Comment 27 Dean Jackson 2016-06-22 14:19:08 PDT
(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.
Comment 28 Simon Fraser (smfr) 2016-06-22 14:34:24 PDT
(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.
Comment 29 Dean Jackson 2016-06-22 15:43:58 PDT
(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.
Comment 30 Dean Jackson 2016-06-22 15:50:34 PDT
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.
Comment 31 Dean Jackson 2016-06-22 16:17:55 PDT
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.
Comment 32 Antoine Quint 2016-06-23 00:58:37 PDT
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.
Comment 33 Antoine Quint 2016-06-23 07:10:04 PDT
Created attachment 281907 [details]
Patch
Comment 34 Antoine Quint 2016-06-23 09:35:56 PDT
Created attachment 281912 [details]
Patch
Comment 35 Dean Jackson 2016-06-23 11:23:52 PDT
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?
Comment 36 Antoine Quint 2016-07-01 14:13:27 PDT
Created attachment 282577 [details]
Patch
Comment 37 Antoine Quint 2016-07-01 14:30:44 PDT
Created attachment 282582 [details]
Patch
Comment 38 Build Bot 2016-07-01 15:18:00 PDT
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
Comment 39 Build Bot 2016-07-01 15:18:04 PDT
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
Comment 40 Build Bot 2016-07-01 15:23:23 PDT
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
Comment 41 Build Bot 2016-07-01 15:23:27 PDT
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
Comment 42 Build Bot 2016-07-01 15:29:51 PDT
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
Comment 43 Build Bot 2016-07-01 15:29:55 PDT
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
Comment 44 Build Bot 2016-07-01 15:36:31 PDT
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
Comment 45 Build Bot 2016-07-01 15:36:35 PDT
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
Comment 46 Tim Horton 2016-07-02 11:05:57 PDT
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??
Comment 47 Simon Fraser (smfr) 2016-07-02 17:16:40 PDT
Comment on attachment 282582 [details]
Patch

r- based on tim's comments.
Comment 48 Antoine Quint 2016-07-03 03:25:57 PDT
(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.
Comment 49 Tim Horton 2016-07-04 23:19:49 PDT
(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.
Comment 50 Antoine Quint 2016-07-05 05:22:23 PDT
Created attachment 282772 [details]
Patch
Comment 51 Build Bot 2016-07-05 06:12:33 PDT
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
Comment 52 Build Bot 2016-07-05 06:12:38 PDT
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
Comment 53 Build Bot 2016-07-05 06:17:04 PDT
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
Comment 54 Build Bot 2016-07-05 06:17:08 PDT
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
Comment 55 Build Bot 2016-07-05 06:25:39 PDT
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
Comment 56 Build Bot 2016-07-05 06:25:43 PDT
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
Comment 57 Build Bot 2016-07-05 06:25:53 PDT
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
Comment 58 Build Bot 2016-07-05 06:25:57 PDT
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
Comment 59 Tim Horton 2016-07-05 15:49:04 PDT
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.
Comment 60 Brent Fulgham 2016-07-05 21:19:32 PDT
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.
Comment 61 Antoine Quint 2016-07-06 03:11:44 PDT
Created attachment 282871 [details]
Patch
Comment 62 Build Bot 2016-07-06 04:02:49 PDT
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
Comment 63 Build Bot 2016-07-06 04:02:54 PDT
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
Comment 64 Build Bot 2016-07-06 04:05:26 PDT
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
Comment 65 Build Bot 2016-07-06 04:05:32 PDT
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
Comment 66 Build Bot 2016-07-06 04:13:05 PDT
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
Comment 67 Build Bot 2016-07-06 04:13:10 PDT
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
Comment 68 Build Bot 2016-07-06 04:15:22 PDT
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
Comment 69 Build Bot 2016-07-06 04:15:27 PDT
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
Comment 70 Antoine Quint 2016-07-06 07:00:44 PDT
Created attachment 282890 [details]
Patch
Comment 71 Antoine Quint 2016-07-06 08:11:41 PDT
Created attachment 282895 [details]
Patch
Comment 72 WebKit Commit Bot 2016-07-06 11:47:27 PDT
Comment on attachment 282895 [details]
Patch

Clearing flags on attachment: 282895

Committed r202867: <http://trac.webkit.org/changeset/202867>
Comment 73 WebKit Commit Bot 2016-07-06 11:47:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 74 WebKit Commit Bot 2016-07-06 14:08:36 PDT
Re-opened since this is blocked by bug 159491
Comment 75 Ryan Haddad 2016-07-06 14:11:58 PDT
*** Bug 159486 has been marked as a duplicate of this bug. ***
Comment 76 Ryan Haddad 2016-07-06 14:17:58 PDT
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.
Comment 77 Alexey Proskuryakov 2016-07-06 19:03:39 PDT
Two threads in vImage, perhaps manipulating the same data?
Comment 78 Tim Horton 2016-07-06 22:32:50 PDT
(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).
Comment 79 Antoine Quint 2016-07-07 04:35:22 PDT
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.
Comment 80 Antoine Quint 2016-07-07 04:56:23 PDT
In WKT, we hit this assertion:

ASSERT(endx.unsafeGet() <= size.width())
Comment 81 Antoine Quint 2016-07-07 05:26:00 PDT
Created attachment 283007 [details]
Patch
Comment 82 WebKit Commit Bot 2016-07-07 12:33:11 PDT
Comment on attachment 283007 [details]
Patch

Clearing flags on attachment: 283007

Committed r202927: <http://trac.webkit.org/changeset/202927>
Comment 83 WebKit Commit Bot 2016-07-07 12:33:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 84 Said Abou-Hallawa 2016-07-19 13:16:23 PDT
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.
Comment 85 Dean Jackson 2016-07-19 17:58:28 PDT
(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.
Comment 86 Dean Jackson 2016-07-19 18:21:43 PDT
(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.
Comment 87 Dean Jackson 2016-07-19 18:25:18 PDT
> > 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.