Bug 235305 – [GPU Process] ImageBuffer::convertToLuminanceMask() and transformToColorSpace() should not access the backend in WebProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Said Abou-Hallawa]

When DOM rendering is handled in GPU Process, no backend access will be allowed. So all the operations that require access to the backend should be handled in GPU Process. The WebProcess will stream messages for these operations to GPUProcess.

[Said Abou-Hallawa]

rdar://83437815

[Said Abou-Hallawa]

Created attachment 449354 [details]
Patch

[Sam Weinig]

Comment on attachment 449354 [details]
Patch

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

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:64
> +    virtual void transformToColorSpace(const DestinationColorSpace&) = 0;

Is transformToColorSpace to ever used? It doesn't make a lot (any?) sense in the way ImageBuffer works since it ImageBuffers have color spaces already.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:265
> +        m_remoteDisplayList.convertToLuminanceMask();

Why does this call convertToLuminanceMask() as well?

[Said Abou-Hallawa]

Comment on attachment 449354 [details]
Patch

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

>> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:64
>> +    virtual void transformToColorSpace(const DestinationColorSpace&) = 0;
> 
> Is transformToColorSpace to ever used? It doesn't make a lot (any?) sense in the way ImageBuffer works since it ImageBuffers have color spaces already.

transformToColorSpace() is implemented for the Cairo port in ImageBufferCairoBackend::transformToColorSpace(). The call to this function in WebCore is not guarded by #if USE(CAIRO). For other ports, we fall back to the default implementation in ImageBufferBackend which is just an empty function.

>> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:265
>> +        m_remoteDisplayList.convertToLuminanceMask();
> 
> Why does this call convertToLuminanceMask() as well?

Copy/Paste mistake. Fixed.

[Said Abou-Hallawa]

Created attachment 449357 [details]
Patch

[Sam Weinig]

(In reply to Said Abou-Hallawa from comment #4)
> Comment on attachment 449354 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449354&action=review
> 
> >> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:64
> >> +    virtual void transformToColorSpace(const DestinationColorSpace&) = 0;
> > 
> > Is transformToColorSpace to ever used? It doesn't make a lot (any?) sense in the way ImageBuffer works since it ImageBuffers have color spaces already.
> 
> transformToColorSpace() is implemented for the Cairo port in
> ImageBufferCairoBackend::transformToColorSpace(). The call to this function
> in WebCore is not guarded by #if USE(CAIRO). For other ports, we fall back
> to the default implementation in ImageBufferBackend which is just an empty
> function.

Ah. Seems like we should figure out what they are trying to accomplish with it and remove it since it makes no sense. Probably makes more sense to create a new ImageBuffer from an existing one with some per-pixel transform or just implement it for them in terms of getPixelData/setPixelData.

> 
> >> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:265
> >> +        m_remoteDisplayList.convertToLuminanceMask();
> > 
> > Why does this call convertToLuminanceMask() as well?
> 
> Copy/Paste mistake. Fixed.

[Sam Weinig]

(In reply to Sam Weinig from comment #6)
> (In reply to Said Abou-Hallawa from comment #4)
> > Comment on attachment 449354 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=449354&action=review
> > 
> > >> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:64
> > >> +    virtual void transformToColorSpace(const DestinationColorSpace&) = 0;
> > > 
> > > Is transformToColorSpace to ever used? It doesn't make a lot (any?) sense in the way ImageBuffer works since it ImageBuffers have color spaces already.
> > 
> > transformToColorSpace() is implemented for the Cairo port in
> > ImageBufferCairoBackend::transformToColorSpace(). The call to this function
> > in WebCore is not guarded by #if USE(CAIRO). For other ports, we fall back
> > to the default implementation in ImageBufferBackend which is just an empty
> > function.
> 
> Ah. Seems like we should figure out what they are trying to accomplish with
> it and remove it since it makes no sense. Probably makes more sense to
> create a new ImageBuffer from an existing one with some per-pixel transform
> or just implement it for them in terms of getPixelData/setPixelData.

And by makes no sense I mean, doesn't match the semantics of ImageBuffer (though if they are depending on drawing into an sRGB image buffer and then converting all the pixels to linear-sRGB being the same as drawing into a linear-sRGB buffer I don't think they will be as the blending will be different.)

[EWS]

Committed r288240 (246194@main): <https://commits.webkit.org/246194@main>

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