Bug 225377 - ImageBuffer with floating point logicalSize() paints into a slightly truncated destination rect
Summary: ImageBuffer with floating point logicalSize() paints into a slightly truncate...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on: 232470 232515 232520 232528
Blocks:
  Show dependency treegraph
 
Reported: 2021-05-04 18:39 PDT by Tim Horton
Modified: 2022-01-07 11:49 PST (History)
20 users (show)

See Also:


Attachments
Patch (28.94 KB, patch)
2021-05-04 18:40 PDT, Tim Horton
sabouhallawa: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (3.36 KB, patch)
2022-01-05 23:21 PST, Myles C. Maxfield
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2021-05-04 18:39:39 PDT
ImageBuffer with floating point logicalSize() paints into a slightly truncated destination rect
Comment 1 Tim Horton 2021-05-04 18:40:25 PDT
Created attachment 427720 [details]
Patch
Comment 2 Tim Horton 2021-05-04 19:00:18 PDT
Of course, my find-and-replace missed some things that aren't in the Xcode project :|
Comment 3 Said Abou-Hallawa 2021-05-04 19:14:47 PDT
Comment on attachment 427720 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext.cpp:830
> +    auto imageLogicalSize = image->logicalSize();

I think we can inline this local variable in the call below.

> Source/WebCore/platform/graphics/GraphicsContext.cpp:831
>      drawConsumingImageBuffer(WTFMove(image), destination, FloatRect(FloatPoint(), FloatSize(imageLogicalSize)), imagePaintingOptions);

FloatSize(imageLogicalSize) -> imageLogicalSize

> Source/WebCore/platform/graphics/ImageBuffer.h:87
> +    virtual IntSize truncatedLogicalSize() const = 0;

Can't we use a different name instead of prefixing an existing one? I have this suggestion:

FloatSize logicalSize(); // The size in the client coordinates
IntSize imageSize();     // The size in logical pixels
IntSize backendSize();   // The size in physical (or device) pixels.
Comment 4 Tim Horton 2021-05-04 19:21:42 PDT
(In reply to Said Abou-Hallawa from comment #3)
> Comment on attachment 427720 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427720&action=review
> 
> > Source/WebCore/platform/graphics/GraphicsContext.cpp:830
> > +    auto imageLogicalSize = image->logicalSize();
> 
> I think we can inline this local variable in the call below.
> 
> > Source/WebCore/platform/graphics/GraphicsContext.cpp:831
> >      drawConsumingImageBuffer(WTFMove(image), destination, FloatRect(FloatPoint(), FloatSize(imageLogicalSize)), imagePaintingOptions);
> 
> FloatSize(imageLogicalSize) -> imageLogicalSize

All true.

> > Source/WebCore/platform/graphics/ImageBuffer.h:87
> > +    virtual IntSize truncatedLogicalSize() const = 0;
> 
> Can't we use a different name instead of prefixing an existing one? I have
> this suggestion:
> 
> FloatSize logicalSize(); // The size in the client coordinates
> IntSize imageSize();     // The size in logical pixels
> IntSize backendSize();   // The size in physical (or device) pixels.

I don't think so. The "truncated-" was there sort of in the spirit of "deprecated-": as a message that you're probably doing something wrong. Your name doesn't give me the uneasy feeling that I was going for :)
Comment 5 Said Abou-Hallawa 2021-05-04 19:30:49 PDT
Comment on attachment 427720 [details]
Patch

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

>>> Source/WebCore/platform/graphics/ImageBuffer.h:87
>>> +    virtual IntSize truncatedLogicalSize() const = 0;
>> 
>> Can't we use a different name instead of prefixing an existing one? I have this suggestion:
>> 
>> FloatSize logicalSize(); // The size in the client coordinates
>> IntSize imageSize();     // The size in logical pixels
>> IntSize backendSize();   // The size in physical (or device) pixels.
> 
> I don't think so. The "truncated-" was there sort of in the spirit of "deprecated-": as a message that you're probably doing something wrong. Your name doesn't give me the uneasy feeling that I was going for :)

"deprecated-"? I think we need both. When you deal with the ImageBuffer internally, e.g. getImageData() and putImageData(), you need the IntSize. And when you draw the ImageBuffer itself in the client context you need the FloatSize. So I do not think we can get it rid of the IntSize.
Comment 6 Tim Horton 2021-05-04 19:36:58 PDT
(In reply to Said Abou-Hallawa from comment #5)
> Comment on attachment 427720 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427720&action=review
> 
> >>> Source/WebCore/platform/graphics/ImageBuffer.h:87
> >>> +    virtual IntSize truncatedLogicalSize() const = 0;
> >> 
> >> Can't we use a different name instead of prefixing an existing one? I have this suggestion:
> >> 
> >> FloatSize logicalSize(); // The size in the client coordinates
> >> IntSize imageSize();     // The size in logical pixels
> >> IntSize backendSize();   // The size in physical (or device) pixels.
> > 
> > I don't think so. The "truncated-" was there sort of in the spirit of "deprecated-": as a message that you're probably doing something wrong. Your name doesn't give me the uneasy feeling that I was going for :)
> 
> "deprecated-"? I think we need both. When you deal with the ImageBuffer
> internally, e.g. getImageData() and putImageData(), you need the IntSize.
> And when you draw the ImageBuffer itself in the client context you need the
> FloatSize. So I do not think we can get it rid of the IntSize.

Maybe the few callers who rightfully need truncation do it themselves? I'm not sure :)

It's definitely wrong *most* of the time, so having something attractively named "imageSize" that does the *mostly wrong* thing is bad, right?
Comment 7 Said Abou-Hallawa 2021-05-06 14:17:45 PDT
Comment on attachment 427720 [details]
Patch

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

>>>>> Source/WebCore/platform/graphics/ImageBuffer.h:87
>>>>> +    virtual IntSize truncatedLogicalSize() const = 0;
>>>> 
>>>> Can't we use a different name instead of prefixing an existing one? I have this suggestion:
>>>> 
>>>> FloatSize logicalSize(); // The size in the client coordinates
>>>> IntSize imageSize();     // The size in logical pixels
>>>> IntSize backendSize();   // The size in physical (or device) pixels.
>>> 
>>> I don't think so. The "truncated-" was there sort of in the spirit of "deprecated-": as a message that you're probably doing something wrong. Your name doesn't give me the uneasy feeling that I was going for :)
>> 
>> "deprecated-"? I think we need both. When you deal with the ImageBuffer internally, e.g. getImageData() and putImageData(), you need the IntSize. And when you draw the ImageBuffer itself in the client context you need the FloatSize. So I do not think we can get it rid of the IntSize.
> 
> Maybe the few callers who rightfully need truncation do it themselves? I'm not sure :)
> 
> It's definitely wrong *most* of the time, so having something attractively named "imageSize" that does the *mostly wrong* thing is bad, right?

Okay I think this is fine. Maybe later we can remove truncatedLogicalSize() and let the callers does the conversion from FloatSize to IntSize.
Comment 8 Radar WebKit Bug Importer 2021-05-11 18:40:17 PDT
<rdar://problem/77874483>
Comment 9 Myles C. Maxfield 2021-10-29 13:50:23 PDT
This patch now has lots of conflicts and can no longer be landed. I'm going to take this over as a part of https://bugs.webkit.org/show_bug.cgi?id=232470
Comment 10 Myles C. Maxfield 2021-10-29 13:50:39 PDT
*** Bug 232470 has been marked as a duplicate of this bug. ***
Comment 11 Myles C. Maxfield 2021-10-29 14:05:28 PDT
Comment on attachment 427720 [details]
Patch

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

>>>>>> Source/WebCore/platform/graphics/ImageBuffer.h:87
>>>>>> +    virtual IntSize truncatedLogicalSize() const = 0;
>>>>> 
>>>>> Can't we use a different name instead of prefixing an existing one? I have this suggestion:
>>>>> 
>>>>> FloatSize logicalSize(); // The size in the client coordinates
>>>>> IntSize imageSize();     // The size in logical pixels
>>>>> IntSize backendSize();   // The size in physical (or device) pixels.
>>>> 
>>>> I don't think so. The "truncated-" was there sort of in the spirit of "deprecated-": as a message that you're probably doing something wrong. Your name doesn't give me the uneasy feeling that I was going for :)
>>> 
>>> "deprecated-"? I think we need both. When you deal with the ImageBuffer internally, e.g. getImageData() and putImageData(), you need the IntSize. And when you draw the ImageBuffer itself in the client context you need the FloatSize. So I do not think we can get it rid of the IntSize.
>> 
>> Maybe the few callers who rightfully need truncation do it themselves? I'm not sure :)
>> 
>> It's definitely wrong *most* of the time, so having something attractively named "imageSize" that does the *mostly wrong* thing is bad, right?
> 
> Okay I think this is fine. Maybe later we can remove truncatedLogicalSize() and let the callers does the conversion from FloatSize to IntSize.

I know I'm 6 months too late here, but there is no situation where truncating the logical size is meaningful. There should be a helper function `physicalSize()` which multiplies the logical size by the resolution scale, and rounds up. That's what getImageData() and putImageData() should be using.
Comment 12 Myles C. Maxfield 2022-01-05 19:26:33 PST
I'm going to treat this bug as the second half of https://bugs.webkit.org/show_bug.cgi?id=232470
Comment 13 Myles C. Maxfield 2022-01-05 23:21:06 PST
Created attachment 448468 [details]
Patch
Comment 14 Myles C. Maxfield 2022-01-07 11:49:11 PST
Committed r287774 (245834@trunk): <https://commits.webkit.org/245834@trunk>