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.
<rdar://problem/25720247>
Created attachment 281731 [details] Patch
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.
Created attachment 281734 [details] Patch
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.
Created attachment 281735 [details] Patch
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
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 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
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 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
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
I don't think these test failures have anything to do with my patch.
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.
(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 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
> 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
Created attachment 281829 [details] Patch
Created attachment 281831 [details] Patch
Created attachment 281832 [details] Patch
(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.
(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.
Created attachment 281833 [details] Patch
Created attachment 281834 [details] Patch
(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 on attachment 281834 [details] Patch I still think this is the wrong approach. You need to get the destination context's colorSpace.
(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.
(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.
(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.
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.
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.
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.
Created attachment 281907 [details] Patch
Created attachment 281912 [details] Patch
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?
Created attachment 282577 [details] Patch
Created attachment 282582 [details] Patch
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
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 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
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 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
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 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
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 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 on attachment 282582 [details] Patch r- based on tim's comments.
(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.
(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.
Created attachment 282772 [details] Patch
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
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 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
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 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
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 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
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 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 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.
Created attachment 282871 [details] Patch
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
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 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
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 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
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 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
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
Created attachment 282890 [details] Patch
Created attachment 282895 [details] Patch
Comment on attachment 282895 [details] Patch Clearing flags on attachment: 282895 Committed r202867: <http://trac.webkit.org/changeset/202867>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 159491
*** Bug 159486 has been marked as a duplicate of this bug. ***
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.
Two threads in vImage, perhaps manipulating the same data?
(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).
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.
In WKT, we hit this assertion: ASSERT(endx.unsafeGet() <= size.width())
Created attachment 283007 [details] Patch
Comment on attachment 283007 [details] Patch Clearing flags on attachment: 283007 Committed r202927: <http://trac.webkit.org/changeset/202927>
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.
(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.
(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.
> > 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.