Bug 225081

Created attachment 427288 [details] Patch

Interesting! The bot breakage I can reproduce, but only with accelerated drawing off (which I of course did not test locally)... I will investigate!

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.

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.

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)

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.

Created attachment 427324 [details] Patch

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.

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

Seems like there's oooonneeee broken test left.

Just to ask the really dumb question here, but what about going the other way, and removing ImageBuffer scaling entirely?

<rdar://problem/77477683>

Created attachment 427621 [details] Patch

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

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

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

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

This will make things a little bit more difficult. For example putImageData() and getImageData() have deal with logical coordinates.

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].