ImageBuffer with floating point logicalSize() paints into a slightly truncated destination rect
Created attachment 427720 [details] Patch
Of course, my find-and-replace missed some things that aren't in the Xcode project :|
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.
(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 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.
(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 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.
<rdar://problem/77874483>
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
*** Bug 232470 has been marked as a duplicate of this bug. ***
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.
I'm going to treat this bug as the second half of https://bugs.webkit.org/show_bug.cgi?id=232470
Created attachment 448468 [details] Patch
Committed r287774 (245834@trunk): <https://commits.webkit.org/245834@trunk>