Bug 235305 - [GPU Process] ImageBuffer::convertToLuminanceMask() and transformToColorSpace() should not access the backend in WebProcess
Summary: [GPU Process] ImageBuffer::convertToLuminanceMask() and transformToColorSpace...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks: 236508
  Show dependency treegraph
 
Reported: 2022-01-17 16:41 PST by Said Abou-Hallawa
Modified: 2022-02-11 13:26 PST (History)
5 users (show)

See Also:


Attachments
Patch (10.59 KB, patch)
2022-01-17 16:44 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (10.60 KB, patch)
2022-01-17 17:15 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2022-01-17 16:41:12 PST
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.
Comment 1 Said Abou-Hallawa 2022-01-17 16:41:38 PST
rdar://83437815
Comment 2 Said Abou-Hallawa 2022-01-17 16:44:59 PST
Created attachment 449354 [details]
Patch
Comment 3 Sam Weinig 2022-01-17 16:57:16 PST
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?
Comment 4 Said Abou-Hallawa 2022-01-17 17:15:06 PST
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.
Comment 5 Said Abou-Hallawa 2022-01-17 17:15:22 PST
Created attachment 449357 [details]
Patch
Comment 6 Sam Weinig 2022-01-18 11:30:05 PST
(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.
Comment 7 Sam Weinig 2022-01-18 11:35:22 PST
(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.)
Comment 8 EWS 2022-01-19 13:39:35 PST
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].