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.
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 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
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
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 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
(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.
(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.
(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.
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 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
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
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
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??
(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 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
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
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
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.
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 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
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
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
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
(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.
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.
2016-06-21 05:03 PDT, Antoine Quint
2016-06-21 05:44 PDT, Antoine Quint
2016-06-21 06:08 PDT, Antoine Quint
2016-06-21 07:04 PDT, Build Bot
2016-06-21 07:09 PDT, Build Bot
2016-06-21 07:11 PDT, Build Bot
2016-06-22 02:40 PDT, Antoine Quint
2016-06-22 02:52 PDT, Antoine Quint
2016-06-22 02:56 PDT, Antoine Quint
2016-06-22 03:39 PDT, Antoine Quint
2016-06-22 03:43 PDT, Antoine Quint
2016-06-23 07:10 PDT, Antoine Quint
2016-06-23 09:35 PDT, Antoine Quint
2016-07-01 14:13 PDT, Antoine Quint
2016-07-01 14:30 PDT, Antoine Quint
2016-07-01 15:18 PDT, Build Bot
2016-07-01 15:23 PDT, Build Bot
2016-07-01 15:29 PDT, Build Bot
2016-07-01 15:36 PDT, Build Bot
2016-07-05 05:22 PDT, Antoine Quint
2016-07-05 06:12 PDT, Build Bot
2016-07-05 06:17 PDT, Build Bot
2016-07-05 06:25 PDT, Build Bot
2016-07-05 06:25 PDT, Build Bot
2016-07-06 03:11 PDT, Antoine Quint
2016-07-06 04:02 PDT, Build Bot
2016-07-06 04:05 PDT, Build Bot
2016-07-06 04:13 PDT, Build Bot
2016-07-06 04:15 PDT, Build Bot
2016-07-06 07:00 PDT, Antoine Quint
2016-07-06 08:11 PDT, Antoine Quint
2016-07-07 05:26 PDT, Antoine Quint