(In reply to Said Abou-Hallawa from bug #225907 comment #11) > Comment on attachment 429266 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=429266&action=review > > > Source/WebCore/platform/graphics/cairo/ImageBufferCairoBackend.cpp:103 > > + m_parameters.colorSpace = destColorSpace; > > + > > This change suggests that transformColorSpace() should take only > destColorSpace. I checked the callers of this function and I found they all > pass the same colorSpace they used to create the ImageBuffer with. So I > would suggest: > 1. Change the name of this function to transformToColorSpace > 2. Make it take only a newColorSpace.
Created attachment 429477 [details] Patch
Created attachment 429511 [details] Patch
Comment on attachment 429511 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429511&action=review > Source/WebCore/ChangeLog:7 > + Can you say something about this change? For example, "The 'srcColorSpace' argument of ImageBuffer::transformColorSpace() should be removed because it is redundant and it is the same as ImageBuffer::colorSpace()." > Source/WebCore/platform/graphics/cairo/ImageBufferCairoBackend.cpp:115 > + } else if (newColorSpace == DestinationColorSpace::SRGB()) { I think there is no need to check if (newColorSpace == DestinationColorSpace::SRGB()) } else { .... } Since we checked above newColorSpace is either DestinationColorSpace::LinearSRGB() or destColorSpace == DestinationColorSpace::SRGB(). > Source/WebCore/rendering/svg/RenderSVGResourceMasker.cpp:-125 > -#if !USE(CG) > - maskerData->maskImage->transformColorSpace(DestinationColorSpace::SRGB(), colorSpace); > -#else > - UNUSED_PARAM(colorSpace); > -#endif Why this block was removed? Should not the function just be renamed and the argument 'colorSpace' be removed?
Comment on attachment 429511 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429511&action=review >> Source/WebCore/rendering/svg/RenderSVGResourceMasker.cpp:-125 >> -#endif > > Why this block was removed? Should not the function just be renamed and the argument 'colorSpace' be removed? Agreed, seems weird to remove this. Though I am not sure this ever really did what was intended. What is happening here is that if svgStyle.colorInterpolation() == ColorInterpolation::LinearRGB, an ImageBuffer is created with a LinearSRGB colorSpace. This works for the CG ports, with drawing happening in LinearSRGB space. For the cairo ports, the drawing happens in normal SRGB space, and then I guess the idea was to transform the whole image buffer to LinearSRGB here. It's not clear to me that those two operations are equivalent.
Comment on attachment 429511 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429511&action=review >> Source/WebCore/ChangeLog:7 >> + > > Can you say something about this change? For example, "The 'srcColorSpace' argument of ImageBuffer::transformColorSpace() should be removed because it is redundant and it is the same as ImageBuffer::colorSpace()." Agreed. >> Source/WebCore/platform/graphics/cairo/ImageBufferCairoBackend.cpp:115 >> + } else if (newColorSpace == DestinationColorSpace::SRGB()) { > > I think there is no need to check if (newColorSpace == DestinationColorSpace::SRGB()) > > } else { > .... > } > > Since we checked above newColorSpace is either DestinationColorSpace::LinearSRGB() or destColorSpace == DestinationColorSpace::SRGB(). I think so. Will fix. >>> Source/WebCore/rendering/svg/RenderSVGResourceMasker.cpp:-125 >>> -#endif >> >> Why this block was removed? Should not the function just be renamed and the argument 'colorSpace' be removed? > > Agreed, seems weird to remove this. Though I am not sure this ever really did what was intended. What is happening here is that if svgStyle.colorInterpolation() == ColorInterpolation::LinearRGB, an ImageBuffer is created with a LinearSRGB colorSpace. This works for the CG ports, with drawing happening in LinearSRGB space. > > For the cairo ports, the drawing happens in normal SRGB space, and then I guess the idea was to transform the whole image buffer to LinearSRGB here. > > It's not clear to me that those two operations are equivalent. Good point. I think I should do the following for cairo case 1. SVGRenderingContext::createImageBuffer with SRGB 2. Call SVGRenderingContext::renderSubtreeToContext (this always renders in SRGB) 3. Do maskerData->maskImage->transformToColorSpace(colorSpace) I will study more and revise the patch. Thank you very much for pointing out.
Created attachment 429624 [details] test case
Created attachment 429625 [details] Patch
svg/custom/grayscale-gradient-mask-2.svg is another test case.
Comment on attachment 429625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429625&action=review > Source/WebCore/rendering/svg/RenderSVGResourceMasker.cpp:82 > +#if USE(CG) > + auto renderingColorSpace = colorSpace; > +#else > + auto renderingColorSpace = DestinationColorSpace::SRGB(); > +#endif I think combining the calculation of colorSpace and renderingColorSpace might be more clear. I would suggest something like this: auto maskColorSpace = DestinationColorSpace::SRGB(); auto drawColorSpace = DestinationColorSpace::SRGB(); #if ENABLE(DESTINATION_COLOR_SPACE_LINEAR_SRGB) if (svgStyle.colorInterpolation() == ColorInterpolation::LinearRGB) { #if USE(CG) maskColorSpace = DestinationColorSpace::LinearSRGB(); #endif drawColorSpace = DestinationColorSpace::LinearSRGB(); } #endif So this code is saying the default colorSpace for the mask and the drawing is SRGB. The exception happens if svgStyle.colorInterpolation is LinearRGB. In this case, the drawing colorSpace will be LinearSRGB. And the CG ports will have an exception also. The mask image will be created with LinearSRGB colorSpace.
Created attachment 429729 [details] Patch for landing
Created attachment 429731 [details] Patch for landing
Comment on attachment 429731 [details] Patch for landing Clearing flags on attachment: 429731 Committed r278078 (238157@main): <https://commits.webkit.org/238157@main>
All reviewed patches have been landed. Closing bug.
<rdar://problem/78496702>