Bug 237128

Summary: [macOS][REGRESSION] (r289518): Form controls are scaled twice on Retina display
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: Layout and RenderingAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, changseok, darin, dino, esprehn+autocc, ews-watchlist, fmalita, fred.wang, glenn, gyuyoung.kim, kondapallykalyan, pdr, schenney, sergio, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
test case
none
Patch
none
Patch
darin: review+, ews-feeder: commit-queue-
Patch none

Description Said Abou-Hallawa 2022-02-23 21:19:19 PST 
Created attachment 453070 [details]
test case

Open the attached test case on Retina display.

Result: The progress control is scaled twice.

When displaying a form control, we have to create a scratch ImageBuffer, then ask the AppKit to render the form control to the GraphicsContext of the ImageBuffer and finally draw the ImageBuffer to the destination GraphicsContext. On Retina display, we have to scale the scratch ImageBuffer by the device scaling factor. The bug happens when creating this scratch ImageBuffer

Before r289518, RenderThemeMac::paintProgressBar() had this call:

    auto imageBuffer = ImageBuffer::createCompatibleBuffer(inflatedRect.size(), deviceScaleFactor, DestinationColorSpace::SRGB(), paintInfo.context());

And this would be translated to this function:

RefPtr<ImageBuffer> ImageBuffer::createCompatibleBuffer(const FloatSize& size, float resolutionScale, const DestinationColorSpace& colorSpace, const GraphicsContext& context)
{
    return ImageBuffer::create(size, context.renderingMode(), resolutionScale, colorSpace, PixelFormat::BGRA8);
}

This means this version of createCompatibleBuffer() was just creating an ImageBuffer with context.renderingMode().

But after r289518, RenderThemeMac::paintProgressBar() the above call was changed to:

    auto imageBuffer = paintInfo.context().createImageBuffer(inflatedRect.size(), { deviceScaleFactor, deviceScaleFactor });

And this will be translated to these functions:

RefPtr<ImageBuffer> GraphicsContext::createImageBuffer(const FloatSize& size, const DestinationColorSpace& colorSpace, RenderingMode renderingMode, RenderingMethod renderingMethod) const
{
    if (renderingMethod == RenderingMethod::DisplayList)
        return ImageBuffer::create(size, renderingMode, ShouldUseDisplayList::Yes, RenderingPurpose::Unspecified, 1, colorSpace, PixelFormat::BGRA8);

     return ImageBuffer::create(size, renderingMode, 1, colorSpace, PixelFormat::BGRA8);
}

RefPtr<ImageBuffer> GraphicsContext::createImageBuffer(const FloatSize& size, const FloatSize& scale, const DestinationColorSpace& colorSpace, std::optional<RenderingMode> renderingMode, RenderingMethod renderingMethod) const
{
    auto expandedScaledSize = scaledImageBufferSize(size, scale);
    if (expandedScaledSize.isEmpty())
        return nullptr;

     auto clampingScale = clampingScaleForImageBufferSize(expandedScaledSize);

     auto imageBuffer = createImageBuffer(expandedScaledSize * clampingScale, colorSpace, renderingMode.value_or(this->renderingMode()), renderingMethod);
    if (!imageBuffer)
        return nullptr;

     imageBuffer->context().scale(clampingScale);

     // 'expandedScaledSize' is mapped to 'size'. So use 'expandedScaledSize / size'
    // not 'scale' because they are not necessarily equal.
    imageBuffer->context().scale(expandedScaledSize / size);
    return imageBuffer;
}

So rr289518 changed the behavior here to scale the size by the FloatSize 'scale' and always create the ImageBuffer with resolutionScale = 1. Moreover it handles the clamping and it scales the GraphicsContext of the ImageBuffer by 'scale'
Comment 1 Said Abou-Hallawa 2022-02-23 21:21:17 PST 
rdar://89064642
Comment 2 Said Abou-Hallawa 2022-02-23 22:03:22 PST 
Created attachment 453073 [details]
Patch
Comment 3 Simon Fraser (smfr) 2022-02-24 11:57:54 PST 
Comment on attachment 453073 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext.h:424
> +    WEBCORE_EXPORT RefPtr<ImageBuffer> createLogicalImageBuffer(const FloatSize&, const FloatSize& scale = { 1, 1 }, const DestinationColorSpace& = DestinationColorSpace::SRGB(), std::optional<RenderingMode> = std::nullopt, RenderingMethod = RenderingMethod::Default) const;
> +    WEBCORE_EXPORT RefPtr<ImageBuffer> createLogicalImageBuffer(const FloatRect&, const FloatSize& scale = { 1, 1 }, const DestinationColorSpace& = DestinationColorSpace::SRGB(), std::optional<RenderingMode> = std::nullopt, RenderingMethod = RenderingMethod::Default) const;

We need a comment here to help me understand what "logical" means in this context. Perhaps we need a better name than "logical".

Is the key points whether the scale passed in compounds with the scale of the content, or replaces it?
Comment 4 Said Abou-Hallawa 2022-02-24 15:50:16 PST 
Comment on attachment 453073 [details]
Patch

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

>> Source/WebCore/platform/graphics/GraphicsContext.h:424
>> +    WEBCORE_EXPORT RefPtr<ImageBuffer> createLogicalImageBuffer(const FloatRect&, const FloatSize& scale = { 1, 1 }, const DestinationColorSpace& = DestinationColorSpace::SRGB(), std::optional<RenderingMode> = std::nullopt, RenderingMethod = RenderingMethod::Default) const;
> 
> We need a comment here to help me understand what "logical" means in this context. Perhaps we need a better name than "logical".
> 
> Is the key points whether the scale passed in compounds with the scale of the content, or replaces it?

I think the trouble here is these three methods provide different levels of compatibility with the underling ImageBuffer.I think the difference between them is:

1. createImageBuffer(): It inherits RenderingMode and RenderingMethod from the underlying ImageBuffer.
2. createLogicalImageBuffer(): It inherits RenderingMode and RenderingMethod from the underlying ImageBuffer. It also transforms the context of the created ImageBuffer to provide logical coordinates to the caller. It scales the context by 'scale' and it translates it by 'rect.location()'. It handles the clamping also.
3. createCompatibleImageBuffer(): Similar to createLogicalImageBuffer() but it inherits the scale of the GraphicsContext instead of taking it as a parameter.

How about these names:
1. createTransformedImageBuffer() // This is a little bit misleading since the ImageBuffer is not transformed. What is transformed is the context. Besides we have to transform the coordinates anyway for CG to have the origin at the top-left corner.
2. createImageBufferInLogicalCoordinates() // Ditto
3. createImageBufferAndTransformContext() // Ditto
4. createImageBufferAndScaleContext() and createImageBufferAndScaleTranslateContext() // One for the FloatSize version and the other for the FloatRect version.
5. createDrawingImageBuffer() // Since we make it ready for drawing.
6. createImageBufferReadyForDrawing() // Since we make it ready for drawing.
7. createImageBufferAndPrepareForDrawing() // Since we make it ready for drawing.
Comment 5 Said Abou-Hallawa 2022-02-28 14:23:47 PST 
Created attachment 453427 [details]
Patch
Comment 6 Darin Adler 2022-02-28 14:43:26 PST 
Comment on attachment 453427 [details]
Patch

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

r=mews

> Source/WebCore/ChangeLog:9
> +        Using the name GraphicsContext::createImageBuffer() for different behaviors

I love the idea of using three different names for these three different functions. But I have no idea when each of these should be used when; the names do not yet make that clear to me. Some of these create bigger higher-resolution buffers. Are those called "scaled" buffers? What does "aligned" mean, scaled to match an underlying context’s resolution?

> Source/WebCore/ChangeLog:19
> +           It can be forced to create a none accelerated local ImageBuffer for example.

"none accelerated" -> "non-accelerated"

> Source/WebCore/ChangeLog:32
> +        To Fix this bug:

Nit: "Fix" -> "fix"

> Source/WebCore/platform/graphics/RenderingMode.h:39
> +enum class RenderingMethod : uint8_t { Local, DisplayList };

uint8_t -> bool
Comment 7 Said Abou-Hallawa 2022-03-01 12:30:31 PST 
Created attachment 453522 [details]
Patch
Comment 8 Said Abou-Hallawa 2022-03-01 12:37:15 PST 
Comment on attachment 453427 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        Using the name GraphicsContext::createImageBuffer() for different behaviors
> 
> I love the idea of using three different names for these three different functions. But I have no idea when each of these should be used when; the names do not yet make that clear to me. Some of these create bigger higher-resolution buffers. Are those called "scaled" buffers? What does "aligned" mean, scaled to match an underlying context’s resolution?

Yes, the "scaled" are used to create higher-resolution buffers. The "aligned" means the coordinates of the created ImageBuffer are aligned with the coordinates of the creator GraphicsContext. I added these statements in the Source/WebCore/ChangeLog:

        1) createImageBuffer(): ...
           
           The caller of this method usually uses a framework to draw some custom
           drawing and it just needs a scratch buffer to be drawn in the place of
           a render object. The caller does not require any transformation to be
           applied to the GraphicsContext of the ImageBuffer before starting its
           custom drawing. Drawing the form controls using AppKit is an example
           of such case.

        2) createScaledImageBuffer(): ...

           This method is suitable for cases when the overall scaleFatcor
           (device ScaleFactor + clamping ScaleFactor) has be known to the caller
           in advance. No clamping will be required in this case. SVG filter,
           masker, clipper and gradient are the callers to this function.

        3) createAlignedImageBuffer(): ...

           Usually the purpose of this method is to transfer the drawing from a
           layer to a scratch ImageBuffer temporarily then draw the scratch
           ImageBuffer in the place of the original drawing. Drawing a PDFDocument
           image, for example, requires using this method.

>> Source/WebCore/ChangeLog:19
>> +           It can be forced to create a none accelerated local ImageBuffer for example.
> 
> "none accelerated" -> "non-accelerated"

Fixed.

>> Source/WebCore/ChangeLog:32
>> +        To Fix this bug:
> 
> Nit: "Fix" -> "fix"

Fixed.

>> Source/WebCore/platform/graphics/RenderingMode.h:39
>> +enum class RenderingMethod : uint8_t { Local, DisplayList };
> 
> uint8_t -> bool

Fixed.
Comment 9 EWS 2022-03-01 15:18:03 PST 
Committed r290679 (247950@main): <https://commits.webkit.org/247950@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453522 [details].