Bug 225081 – Use ImageBuffer scaling in RemoteLayerBackingStore, rather than handling scale in the class

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Sam Weinig]

Use ImageBuffer scaling in RemoteLayerBackingStore, rather than handling scale in the class.

[Tim Horton]

Created attachment 427288 [details]
Patch

[Tim Horton]

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

[Tim Horton]

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.

[Tim Horton]

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.

[Said Abou-Hallawa]

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)

[Tim Horton]

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.

[Tim Horton]

Created attachment 427324 [details]
Patch

[Said Abou-Hallawa]

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.

[Tim Horton]

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

[Tim Horton]

Seems like there's oooonneeee broken test left.

[Sam Weinig]

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

[Radar WebKit Bug Importer]

<rdar://problem/77477683>

[Tim Horton]

Created attachment 427621 [details]
Patch

[Tim Horton]

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

[Sam Weinig]

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

[Said Abou-Hallawa]

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

[Tim Horton]

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

[Said Abou-Hallawa]

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

[EWS]

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