Bug 225081 - Use ImageBuffer scaling in RemoteLayerBackingStore, rather than handling scale in the class
Summary: Use ImageBuffer scaling in RemoteLayerBackingStore, rather than handling scal...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-26 16:28 PDT by Sam Weinig
Modified: 2022-05-04 22:38 PDT (History)
5 users (show)

See Also:


Attachments
Patch (7.62 KB, patch)
2021-04-28 12:43 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (9.11 KB, patch)
2021-04-28 19:23 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (10.08 KB, patch)
2021-05-03 16:59 PDT, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2021-04-26 16:28:35 PDT
Use ImageBuffer scaling in RemoteLayerBackingStore, rather than handling scale in the class.
Comment 1 Tim Horton 2021-04-28 12:43:13 PDT
Created attachment 427288 [details]
Patch
Comment 2 Tim Horton 2021-04-28 16:03:08 PDT
Interesting! The bot breakage I can reproduce, but only with accelerated drawing off (which I of course did not test locally)... I will investigate!
Comment 3 Tim Horton 2021-04-28 17:54:07 PDT
Mystifying. We make a software buffer with resolutionScale=2 but when it comes back it's got a CTM with scale 1. But only on iOS, and only software buffers, IOSurface works fine.
Comment 4 Tim Horton 2021-04-28 18:05:46 PDT
OK, here's why:

ImageBufferShareableBitmapBackend::create adopts a GraphicsContext created by ShareableBitmap::createGraphicsContext.

ShareableBitmap doesn't know about scale; it's just a bag of bytes.

So, if we use ImageBuffer's setupContext to apply the root transform (the flip and device scale), we get the scale as we'd like, but then we've double flipped (because ShareableBitmap::createGraphicsContext DOES flip).

There are many ways to fix this, I'm not sure which is best.
Comment 5 Said Abou-Hallawa 2021-04-28 18:30:25 PDT
A possible solution:

1. Add a new parameter ShareableBitmap::createGraphicsContext(float scaleFactor = 1)
2. Add the following call at the end of ShareableBitmap::createGraphicsContext()
   CGContextSetBaseCTM(bitmapContext.get(), CGAffineTransformScale(CGContextGetBaseCTM(bitmapContext.get()), scaleFactor, scaleFactor));
3. Change ImageBufferShareableBitmapBackend::create() to call bitmap->createGraphicsContext(parameters.resolutionScale)
Comment 6 Tim Horton 2021-04-28 18:36:48 PDT
Yeah, "teach ShareableBitmap about scale" is definitely one of the options.

The other easy one is "make ImageBufferShareableBitmapBackend apply its scale when it adopts the context".

*Ideally* setupContext would just like... do the right thing, and we'd have enough bits on each of the backends to know what we need. But that's a bigger refactor.
Comment 7 Tim Horton 2021-04-28 19:23:40 PDT
Created attachment 427324 [details]
Patch
Comment 8 Said Abou-Hallawa 2021-04-28 19:31:12 PDT
Comment on attachment 427324 [details]
Patch

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

> Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.cpp:131
> +    m_context->applyDeviceScaleFactor(resolutionScale());

I think this is fine. Splitting the context setup between here and ShareableBitmap::createGraphicsContext() seems inconsistent with IOSurface and CGBitamp backends. So maybe in the future we need to call setupContext() for all CG contexts instead.
Comment 9 Tim Horton 2021-04-28 19:31:44 PDT
(In reply to Said Abou-Hallawa from comment #8)
> Comment on attachment 427324 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427324&action=review
> 
> > Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.cpp:131
> > +    m_context->applyDeviceScaleFactor(resolutionScale());
> 
> I think this is fine. Splitting the context setup between here and
> ShareableBitmap::createGraphicsContext() seems inconsistent with IOSurface
> and CGBitamp backends. So maybe in the future we need to call setupContext()
> for all CG contexts instead.

Agreed, there is definitely some cleanup that can be done, with more typing :)
Comment 10 Tim Horton 2021-04-29 02:27:55 PDT
Seems like there's oooonneeee broken test left.
Comment 11 Sam Weinig 2021-05-02 19:16:39 PDT
Just to ask the really dumb question here, but what about going the other way, and removing ImageBuffer scaling entirely?
Comment 12 Radar WebKit Bug Importer 2021-05-03 16:29:14 PDT
<rdar://problem/77477683>
Comment 13 Tim Horton 2021-05-03 16:59:13 PDT
Created attachment 427621 [details]
Patch
Comment 14 Tim Horton 2021-05-03 17:00:03 PDT
(In reply to Sam Weinig from comment #11)
> Just to ask the really dumb question here, but what about going the other
> way, and removing ImageBuffer scaling entirely?

That is a much scarier prospect. Not crazy, but would require more typing and care for sure.
Comment 15 Sam Weinig 2021-05-03 19:06:23 PDT
(In reply to Tim Horton from comment #14)
> (In reply to Sam Weinig from comment #11)
> > Just to ask the really dumb question here, but what about going the other
> > way, and removing ImageBuffer scaling entirely?
> 
> That is a much scarier prospect. Not crazy, but would require more typing
> and care for sure.

Definitely. I just don't recall the reason we made scale a property of ImageBuffer.
Comment 16 Said Abou-Hallawa 2021-05-03 19:16:38 PDT
(In reply to Sam Weinig from comment #15)
> (In reply to Tim Horton from comment #14)
> > (In reply to Sam Weinig from comment #11)
> > > Just to ask the really dumb question here, but what about going the other
> > > way, and removing ImageBuffer scaling entirely?
> > 
> > That is a much scarier prospect. Not crazy, but would require more typing
> > and care for sure.
> 
> Definitely. I just don't recall the reason we made scale a property of
> ImageBuffer.

I think the scaling is used when creating ImageBuffers for Retina display. We want to have the right pixel resolution for the backend. But at the same time we draw to the context in logical coordinates. See ImageBufferCGBackend::setupContext().
Comment 17 Tim Horton 2021-05-03 19:18:29 PDT
(In reply to Said Abou-Hallawa from comment #16)
> (In reply to Sam Weinig from comment #15)
> > (In reply to Tim Horton from comment #14)
> > > (In reply to Sam Weinig from comment #11)
> > > > Just to ask the really dumb question here, but what about going the other
> > > > way, and removing ImageBuffer scaling entirely?
> > > 
> > > That is a much scarier prospect. Not crazy, but would require more typing
> > > and care for sure.
> > 
> > Definitely. I just don't recall the reason we made scale a property of
> > ImageBuffer.
> 
> I think the scaling is used when creating ImageBuffers for Retina display.
> We want to have the right pixel resolution for the backend. But at the same
> time we draw to the context in logical coordinates. See
> ImageBufferCGBackend::setupContext().

Sam's point is that this could be something that *clients* of ImageBuffer worry about, instead of ImageBuffer itself (just like e.g. IOSurface).
Comment 18 Said Abou-Hallawa 2021-05-03 19:25:05 PDT
This will make things a little bit more difficult. For example putImageData() and getImageData() have deal with logical coordinates.
Comment 19 EWS 2021-05-03 21:08:31 PDT
Committed r276945 (237278@main): <https://commits.webkit.org/237278@main>

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