Bug 232459

Summary: Yahoo.com has fragments of wrongly-scaled content throughout when using CGDisplayListImageBufferBackend
Product: WebKit Reporter: Tim Horton <thorton>
Component: New BugsAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, dino, sabouhallawa, simon.fraser, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Tim Horton 2021-10-28 15:31:44 PDT
Yahoo.com has fragments of wrongly-scaled content throughout when using CGDisplayListImageBufferBackend
Comment 1 Tim Horton 2021-10-28 15:33:05 PDT
Created attachment 442753 [details]
Patch
Comment 2 Tim Horton 2021-10-28 15:35:16 PDT
Created attachment 442754 [details]
Patch
Comment 3 Darin Adler 2021-10-28 15:44:32 PDT
Comment on attachment 442754 [details]
Patch

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

> Source/WebKit/Shared/RemoteLayerTree/CGDisplayListImageBufferBackend.cpp:40
> +    GraphicsContextCGDisplayList(CGContextRef cgContext, float immutableBaseScaleFactor)

Since AffineTransform is based on double, maybe we should store this as a double? Generally speaking not sure how performance critical any of this is, but written like this we have a float to double conversion every time setCTM and getCTM are called. But we also have floating point division in setCTM, and making the same AffineTransform each time the function is called, so I suspect it’s likely not all that performance critical.
Comment 4 Tim Horton 2021-10-28 15:57:14 PDT
(In reply to Darin Adler from comment #3)
> Comment on attachment 442754 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442754&action=review
> 
> > Source/WebKit/Shared/RemoteLayerTree/CGDisplayListImageBufferBackend.cpp:40
> > +    GraphicsContextCGDisplayList(CGContextRef cgContext, float immutableBaseScaleFactor)
> 
> Since AffineTransform is based on double, maybe we should store this as a
> double?

True! I was following all of our deviceScaleFactors, which are float-y, since that's where it *comes* from.

> Generally speaking not sure how performance critical any of this is,
> but written like this we have a float to double conversion every time setCTM
> and getCTM are called. But we also have floating point division in setCTM,
> and making the same AffineTransform each time the function is called, so I
> suspect it’s likely not all that performance critical.

Neither are /very/ hot in my testing, no, but these are both good points. I will adjust slightly to address both :) (in fact, because CGDisplayListImageBufferBackend's context is only ever used as the secondary context in a BifurcatedGraphicsContext, its getCTM is actually never called, I only included it here for completeness's sake and in case we ever want to use it directly).
Comment 5 Tim Horton 2021-10-28 20:07:51 PDT
Created attachment 442774 [details]
Patch
Comment 6 EWS 2021-10-28 20:49:32 PDT
Committed r285017 (243661@main): <https://commits.webkit.org/243661@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442774 [details].
Comment 7 Radar WebKit Bug Importer 2021-10-28 20:50:18 PDT
<rdar://problem/84792074>