Bug 225377 – ImageBuffer with floating point logicalSize() paints into a slightly truncated destination rect

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

ImageBuffer with floating point logicalSize() paints into a slightly truncated destination rect

[Tim Horton]

Created attachment 427720 [details]
Patch

[Tim Horton]

Of course, my find-and-replace missed some things that aren't in the Xcode project :|

[Said Abou-Hallawa]

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.

[Tim Horton]

(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 :)

[Said Abou-Hallawa]

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.

[Tim Horton]

(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?

[Said Abou-Hallawa]

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.

[Radar WebKit Bug Importer]

<rdar://problem/77874483>

[Myles C. Maxfield]

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

[Myles C. Maxfield]

*** Bug 232470 has been marked as a duplicate of this bug. ***

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

I'm going to treat this bug as the second half of https://bugs.webkit.org/show_bug.cgi?id=232470

[Myles C. Maxfield]

Created attachment 448468 [details]
Patch

[Myles C. Maxfield]

Committed r287774 (245834@trunk): <https://commits.webkit.org/245834@trunk>