Bug 235758

Created attachment 450210 [details] Patch

Created attachment 450261 [details] Patch

Comment on attachment 450210 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450210&action=review > Source/WebCore/platform/graphics/GraphicsContext.h:426 > + WEBCORE_EXPORT virtual RefPtr<ImageBuffer> createCompatibleImageBuffer(const FloatSize&, float resolutionScale, const DestinationColorSpace& = DestinationColorSpace::SRGB()) const; > + WEBCORE_EXPORT RefPtr<ImageBuffer> createCompatibleImageBuffer(const FloatSize&, const DestinationColorSpace& = DestinationColorSpace::SRGB()) const; > + RefPtr<ImageBuffer> createCompatibleImageBuffer(ImageBuffer& sourceImage, const FloatRect&, const DestinationColorSpace& = DestinationColorSpace::SRGB()) const; > + > + RefPtr<ImageBuffer> createCompatibleConcreteImageBuffer(const FloatRect&, const DestinationColorSpace& = DestinationColorSpace::SRGB()) const; Can we collapse these down? When writing code, I wouldn't know which one to use and how they differ.

Created attachment 450288 [details] Patch

The plan also is to move SVGRenderingContext::createImageBuffer() to GraphicsContext. So all the intermediate image buffers of the SVG resources can be the same type as the type of the underlying ImageBuffer of the GraphicsContext.

Comment on attachment 450288 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450288&action=review > Source/WebCore/platform/graphics/GraphicsContext.h:422 > + WEBCORE_EXPORT virtual RefPtr<ImageBuffer> createImageBuffer(const FloatSize&, float resolutionScale = 1, const DestinationColorSpace& = DestinationColorSpace::SRGB()) const; Let's put a blank line under this one > Source/WebCore/platform/graphics/GraphicsContext.h:425 > + RefPtr<ImageBuffer> copyRectToCompatibleImageBuffer(ImageBuffer& sourceImage, const FloatRect&, const DestinationColorSpace& = DestinationColorSpace::SRGB()) const; Do we need this one? The caller can do the same in a few lines of code, and it's confusing, because I don't know if the current context is, or has to be the context associated with sourceImage.

Created attachment 450853 [details] Patch

<rdar://problem/88478470>

Created attachment 450919 [details] Patch

Comment on attachment 450919 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450919&action=review > Source/WebCore/platform/graphics/GraphicsContext.h:419 > + enum class CreateMethod : bool { Default, Base }; These names don't really communicate what they do. I'm not sure from reading this which one I'd choose. Also, no-one outside of GraphicsContext uses this, so it would be nice to keep it internal. Maybe createImageBuffer(...CreateMethod) can be private, with a public createImageBuffer(...) that calls the internal function. > Source/WebCore/platform/graphics/GraphicsContext.h:421 > + FloatSize tightImageBufferSize(const FloatSize&, const FloatSize& scale) const; Not sure what "tight" means here. I think this is integral snapping the scaled size, then returning the size with scale accounted for? I think it should have the word "expanded" in the name, and name should try to clarify if the result is accounting for scale or not. It looks like this can be static since it doesn't consult GraphicsContext state. Maybe it can just be moved to the one call site? > Source/WebCore/platform/graphics/GraphicsContext.h:430 > + RefPtr<ImageBuffer> createImageBuffer(const FloatSize&, const FloatSize& scale, const DestinationColorSpace& = DestinationColorSpace::SRGB(), std::optional<RenderingMode> = std::nullopt, CreateMethod = CreateMethod::Default) const; > + RefPtr<ImageBuffer> createImageBuffer(const FloatRect&, const FloatSize& scale, const DestinationColorSpace& = DestinationColorSpace::SRGB(), std::optional<RenderingMode> = std::nullopt, CreateMethod = CreateMethod::Default) const; > + > + WEBCORE_EXPORT virtual RefPtr<ImageBuffer> createCompatibleImageBuffer(const FloatSize&, const DestinationColorSpace& = DestinationColorSpace::SRGB(), std::optional<RenderingMode> = std::nullopt) const; > + WEBCORE_EXPORT virtual RefPtr<ImageBuffer> createCompatibleImageBuffer(const FloatRect&, const DestinationColorSpace& = DestinationColorSpace::SRGB(), std::optional<RenderingMode> = std::nullopt) const; This is nice and clean but I wonder if we can simplify it further and just have FloatRect versions (FloatRect.location will be 0,0 most of the time).

Created attachment 450975 [details] Patch

Comment on attachment 450919 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450919&action=review >> Source/WebCore/platform/graphics/GraphicsContext.h:419 >> + enum class CreateMethod : bool { Default, Base }; > > These names don't really communicate what they do. I'm not sure from reading this which one I'd choose. Also, no-one outside of GraphicsContext uses this, so it would be nice to keep it internal. Maybe createImageBuffer(...CreateMethod) can be private, with a public createImageBuffer(...) that calls the internal function. Done. And I made the virtual function createImageBuffer as well to be protected. >> Source/WebCore/platform/graphics/GraphicsContext.h:421 >> + FloatSize tightImageBufferSize(const FloatSize&, const FloatSize& scale) const; > > Not sure what "tight" means here. I think this is integral snapping the scaled size, then returning the size with scale accounted for? I think it should have the word "expanded" in the name, and name should try to clarify if the result is accounting for scale or not. > > It looks like this can be static since it doesn't consult GraphicsContext state. Maybe it can just be moved to the one call site? I moved this function to RenderSVGResourcePattern::createTileImage(). RenderSVGResourcePattern::createTileImage() was calling a version of SVGRenderingContext::createImageBuffer() which was calling roundedIntSize() not expandedIntSize(). tightImageBufferSize() is supposed to return the outsets which would make expandedIntSize() differs from roundedIntSize(). The goal is to not add a new createImageBuffer() function or an enum to an existing one to preserve this behavior. >> Source/WebCore/platform/graphics/GraphicsContext.h:430 >> + WEBCORE_EXPORT virtual RefPtr<ImageBuffer> createCompatibleImageBuffer(const FloatRect&, const DestinationColorSpace& = DestinationColorSpace::SRGB(), std::optional<RenderingMode> = std::nullopt) const; > > This is nice and clean but I wonder if we can simplify it further and just have FloatRect versions (FloatRect.location will be 0,0 most of the time). I think we have to have both versions of the function. 1. The FloatRect version is used for drawing something we know its location and we want to temporarily draw to an ImageBuffer and then draw it to a destination context. But we have to consider for sub-pixels. So we should exclude the outsets of the expanded rectangle from the scaling and the translation. But we want to allow the drawing to leak to these outsets. 2. The FloatSize version is used for snapshotting for example. We need it to draw contents and then sink it to a NativeImage. So we ignore the sub-pixels at the borders because they are not noticeable in this case.

Comment on attachment 450975 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450975&action=review Starting reviewing this and then realized it was already landed maybe? > Source/WebCore/platform/graphics/GraphicsContext.cpp:554 > +static inline IntSize scaledImageBufferSize(const FloatSize& size, const FloatSize& scale) Not sure const& is a good optimization when the argument is a pair of floats. Also, unnecessary to write "inline"; compiler will likely inline this either way, and it’s not in a header where we need to use "inline" to deal with the one-definition rule. > Source/WebCore/platform/graphics/GraphicsContext.cpp:561 > +static inline IntRect scaledImageBufferRect(const FloatRect& rect, const FloatSize& scale) Similar thoughts here. > Source/WebCore/platform/graphics/GraphicsContext.cpp:571 > + ImageBuffer::sizeNeedsClamping(size, clampingScale); This seems like a peculiar function name. It sounds like it would return a boolean indicating whether a size needs clamping. > Source/WebCore/platform/graphics/GraphicsContext.cpp:610 > +RefPtr<ImageBuffer> GraphicsContext::createImageBuffer(const FloatRect& rect, const FloatSize& scale, const DestinationColorSpace& colorSpace, RenderingMode renderingMode, CreateMethod createMethod) const There is so much repetition between this and the function above. Would be great at some point figure out how to write them without so much copy and paste. Should be pretty easy with a template. > Source/WebCore/platform/graphics/ImageBuffer.h:53 > + RefPtr<ImageBuffer> clone(); Seems like this can be const. > Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:242 > + // Create in-process ImageBuffer since the deocoding of PDF images has to happen in WebProcess. Typo: "deocoding". > Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:243 > + m_cachedImageBuffer = context.GraphicsContext::createCompatibleImageBuffer(cachedImageSize); I think a separate named function would be better than calling through to GraphicsContext::createCompatibleImageBuffer. Can just have GraphicsContext::createCompatibleImageBuffer be an inline function that calls that named function. The name of that function would make it clearer what is going on here, with less burden on the comment. > Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:296 > + if (auto imageBuffer = context.GraphicsContext::createCompatibleImageBuffer(localDestinationRect.size())) { Ditto. > Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:382 > + // Create in-process ImageBuffer since the deocoding of OT-SVG fonts has to happen in WebProcess. > + if (auto imageBuffer = m_owner.GraphicsContext::createCompatibleImageBuffer(bounds)) { Name would help here too. Same spelling error here.

Created attachment 451066 [details] Patch

Comment on attachment 450975 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450975&action=review >> Source/WebCore/platform/graphics/GraphicsContext.cpp:554 >> +static inline IntSize scaledImageBufferSize(const FloatSize& size, const FloatSize& scale) > > Not sure const& is a good optimization when the argument is a pair of floats. Also, unnecessary to write "inline"; compiler will likely inline this either way, and it’s not in a header where we need to use "inline" to deal with the one-definition rule. 'inline' was removed. For passing the FloatSize as const& versus passing it by value, I searched GraphicsContext.h and I found all the FloatSize are passed by references. Here is the list: WEBCORE_EXPORT void setShadow(const FloatSize&, float blur, const Color&, ShadowRadiusMode = ShadowRadiusMode::Default); WEBCORE_EXPORT bool getShadow(FloatSize&, float&, Color&) const; virtual void drawNativeImage(NativeImage&, const FloatSize& selfSize, const FloatRect& destRect, const FloatRect& srcRect, const ImagePaintingOptions& = { }) = 0; WEBCORE_EXPORT virtual ImageDrawResult drawTiledImage(Image&, const FloatRect& destination, const FloatPoint& source, const FloatSize& tileSize, const FloatSize& spacing, const ImagePaintingOptions& = { }); WEBCORE_EXPORT virtual ImageDrawResult drawTiledImage(Image&, const FloatRect& destination, const FloatRect& source, const FloatSize& tileScaleFactor, Image::TileRule, Image::TileRule, const ImagePaintingOptions& = { }); virtual void drawPattern(NativeImage&, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& tileRect, const AffineTransform& patternTransform, const FloatPoint& phase, const FloatSize& spacing, const ImagePaintingOptions& = { }) = 0; virtual void scale(const FloatSize&) = 0; void translate(const FloatSize& size) { translate(size.width(), size.height()); } I reached the whole source code for "(FloatSize " and I found 39 hits. I searched the whole source code for "(const FloatSize&" and I found 249 hits. >> Source/WebCore/platform/graphics/GraphicsContext.cpp:571 >> + ImageBuffer::sizeNeedsClamping(size, clampingScale); > > This seems like a peculiar function name. It sounds like it would return a boolean indicating whether a size needs clamping. bool ImageBuffer::sizeNeedsClamping(const FloatSize& size, FloatSize& scale) It returns this Boolean but it also changes 'scale' such that "size * scale" will not need clamping. I can change the name of this function in a operate patch since this one is getting bigger and bigger. >> Source/WebCore/platform/graphics/ImageBuffer.h:53 >> + RefPtr<ImageBuffer> clone(); > > Seems like this can be const. Fixed. >> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:242 >> + // Create in-process ImageBuffer since the deocoding of PDF images has to happen in WebProcess. > > Typo: "deocoding". Fixed.

Comment on attachment 450975 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450975&action=review >>> Source/WebCore/platform/graphics/GraphicsContext.cpp:554 >>> +static inline IntSize scaledImageBufferSize(const FloatSize& size, const FloatSize& scale) >> >> Not sure const& is a good optimization when the argument is a pair of floats. Also, unnecessary to write "inline"; compiler will likely inline this either way, and it’s not in a header where we need to use "inline" to deal with the one-definition rule. > > 'inline' was removed. > > For passing the FloatSize as const& versus passing it by value, I searched GraphicsContext.h and I found all the FloatSize are passed by references. Here is the list: > > WEBCORE_EXPORT void setShadow(const FloatSize&, float blur, const Color&, ShadowRadiusMode = ShadowRadiusMode::Default); > WEBCORE_EXPORT bool getShadow(FloatSize&, float&, Color&) const; > virtual void drawNativeImage(NativeImage&, const FloatSize& selfSize, const FloatRect& destRect, const FloatRect& srcRect, const ImagePaintingOptions& = { }) = 0; > WEBCORE_EXPORT virtual ImageDrawResult drawTiledImage(Image&, const FloatRect& destination, const FloatPoint& source, const FloatSize& tileSize, const FloatSize& spacing, const ImagePaintingOptions& = { }); > WEBCORE_EXPORT virtual ImageDrawResult drawTiledImage(Image&, const FloatRect& destination, const FloatRect& source, const FloatSize& tileScaleFactor, Image::TileRule, Image::TileRule, const ImagePaintingOptions& = { }); > virtual void drawPattern(NativeImage&, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& tileRect, const AffineTransform& patternTransform, const FloatPoint& phase, const FloatSize& spacing, const ImagePaintingOptions& = { }) = 0; > virtual void scale(const FloatSize&) = 0; > void translate(const FloatSize& size) { translate(size.width(), size.height()); } > > I reached the whole source code for "(FloatSize " and I found 39 hits. I searched the whole source code for "(const FloatSize&" and I found 249 hits. Yes, I think const FloatSize& has been a long term bad habit. We should do some investigation. I think it’s hurting performance instead of helping it. Now that we are on 64-bit platforms, a FloatSize is the same size as a pointer to a FloatSize. But.I understand if you don}t want to start here. >>> Source/WebCore/platform/graphics/GraphicsContext.cpp:571 >>> + ImageBuffer::sizeNeedsClamping(size, clampingScale); >> >> This seems like a peculiar function name. It sounds like it would return a boolean indicating whether a size needs clamping. > > bool ImageBuffer::sizeNeedsClamping(const FloatSize& size, FloatSize& scale) > > It returns this Boolean but it also changes 'scale' such that "size * scale" will not need clamping. I can change the name of this function in a operate patch since this one is getting bigger and bigger. Sounds fine.

Created attachment 451154 [details] Patch

Comment on attachment 450975 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450975&action=review >> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:243 >> + m_cachedImageBuffer = context.GraphicsContext::createCompatibleImageBuffer(cachedImageSize); > > I think a separate named function would be better than calling through to GraphicsContext::createCompatibleImageBuffer. Can just have GraphicsContext::createCompatibleImageBuffer be an inline function that calls that named function. The name of that function would make it clearer what is going on here, with less burden on the comment. The function GraphicsContext::createInProcessCompatibleImageBuffer() is added for this purpose and it will be called from this function and will be used for similar cases.

Comment on attachment 451154 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451154&action=review > Source/WebCore/platform/cocoa/ThemeCocoa.mm:114 > + // Create in-process ImageBuffer since the decoding of PDF images has to happen in WebProcess. > + auto imageBuffer = context.createInProcessCompatibleImageBuffer(size); I don't know how, as a normal webkit developer, I'd know that I need to use an in-process buffer in some situations. Maybe instead we should have a "purpose" optionset, and if it contains "Purpose::PDF" then we do in-process? > Source/WebCore/platform/graphics/GraphicsContext.cpp:666 > +RefPtr<ImageBuffer> GraphicsContext::createInProcessCompatibleImageBuffer(const FloatSize& size, const DestinationColorSpace& colorSpace) const > +{ > + return createImageBuffer(size, scaleFactor(), colorSpace, RenderingMode::Unaccelerated, CreateMethod::Base); > +} > + > +RefPtr<ImageBuffer> GraphicsContext::createInProcessCompatibleImageBuffer(const FloatRect& rect, const DestinationColorSpace& colorSpace) const > +{ > + return createImageBuffer(rect, scaleFactor(), colorSpace, RenderingMode::Unaccelerated, CreateMethod::Base); > +} The original meaning of "compatible" was "match accelerated" but that's no longer true here. Also, this code leaks WebKit-level process stuff down here into WebCore. Can we instead use CreateMethod::Derived then have the derived class make the determination that it has to be an in-process buffer?

Created attachment 451341 [details] Patch

Comment on attachment 451154 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451154&action=review >> Source/WebCore/platform/cocoa/ThemeCocoa.mm:114 >> + auto imageBuffer = context.createInProcessCompatibleImageBuffer(size); > > I don't know how, as a normal webkit developer, I'd know that I need to use an in-process buffer in some situations. Maybe instead we should have a "purpose" optionset, and if it contains "Purpose::PDF" then we do in-process? I added the following enum: enum class RenderingMethod : uint8_t { Default, Local, DisplayList }; And I added it as an argument to createCompatibleImageBuffer(). For the PDF and SVG font cases, we should pass RenderingMethod::Local. This will indicate the compatible ImageBuffer is in-process or 'local' regardless of the type of the underlying ImageBuffer of the GraphicsContext. This call looks like this: // Create a local ImageBuffer because decoding the PDF images has to happen in WebProcess. auto imageBuffer = context.createCompatibleImageBuffer(size, DestinationColorSpace::SRGB(), RenderingMethod::Local); >> Source/WebCore/platform/graphics/GraphicsContext.cpp:666 >> +} > > The original meaning of "compatible" was "match accelerated" but that's no longer true here. Also, this code leaks WebKit-level process stuff down here into WebCore. > > Can we instead use CreateMethod::Derived then have the derived class make the determination that it has to be an in-process buffer? This function and the enum CreateMethod have been removed. Instead we can use createCompatibleImageBuffer with the new enum RenderingMethod. By default the RenderingMode of the compatible ImageBuffer will match the RenderingMode of the underlying ImageBuffer of the GraphicsContext. So this is the new implementation of this function: RefPtr<ImageBuffer> GraphicsContext::createCompatibleImageBuffer(const FloatRect& rect, const DestinationColorSpace& colorSpace, RenderingMethod renderingMethod) const { return createImageBuffer(rect, scaleFactor(), colorSpace, renderingMode(), renderingMethod); } So we will allow creating in-process ImageBuffers with IOSurface backend as long as the underlying ImageBuffer of the GraphicsContext itself is in-process. But we will override this behavior for RemoteImageBufferProxy. So the implementation of this function for RemoteDisplayListRecorderProxy will look like this: RefPtr<ImageBuffer> RemoteDisplayListRecorderProxy::createCompatibleImageBuffer(const FloatRect& rect, const DestinationColorSpace& colorSpace, RenderingMethod renderingMethod) const { auto renderingMode = renderingMethod == RenderingMethod::Default ? this->renderingMode() : RenderingMode::Unaccelerated; return GraphicsContext::createImageBuffer(rect, scaleFactor(), colorSpace, renderingMode, renderingMethod); } Since the underlying ImageBuffer of the GraphicsContext is remote and we are asked to create an in-porcess ImageBuffer, it does not make sense to create it with IOSurface backend.

Comment on attachment 451341 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451341&action=review > Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:70 > + // Ignore 2D rotation, as it doesn't affect the size of the mask. > + FloatSize scale(absoluteTransform.xScale(), absoluteTransform.yScale()); > + > + // Determine scale factor for the clipper. The size of intermediate ImageBuffers shouldn't be bigger than kMaxFilterSize. > + ImageBuffer::sizeNeedsClamping(repaintRect.size(), scale); > + > + auto maskImage = context->createImageBuffer(repaintRect, scale); There's a bit of repeated code here; I wonder if it should be moved into a helper method. > Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.cpp:482 > + auto renderingMode = renderingMethod == RenderingMethod::Default ? this->renderingMode() : RenderingMode::Unaccelerated; This implicit impact of renderingMethod on renderingMode is a bit subtle.

Comment on attachment 451341 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451341&action=review >> Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:70 >> + auto maskImage = context->createImageBuffer(repaintRect, scale); > > There's a bit of repeated code here; I wonder if it should be moved into a helper method. I agree. I will clean it in a separate patch. >> Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.cpp:482 >> + auto renderingMode = renderingMethod == RenderingMethod::Default ? this->renderingMode() : RenderingMode::Unaccelerated; > > This implicit impact of renderingMethod on renderingMode is a bit subtle. If renderingMethod != RenderingMethod::Default, this means the compatible ImageBuffer is in-process So it does not make sense to be drawing on RemoteImageBufferProxy (which was created many for security reasons) and then allow creating in-process ImageBuffer with IOSurface backend. I will add a comment in the clean-up patch.

Committed r289518 (247049@main): <https://commits.webkit.org/247049@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 451341 [details].

Reopening to attach new patch.

Created attachment 451500 [details] Patch

Oh, somehow webkit-patch upload got very confused.