Bug 235305

Created attachment 449354 [details] Patch

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

Created attachment 449357 [details] Patch

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

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

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