Rename ImageBuffer::transformColorSpace to transformToColorSpace, and it should take a single argument

• Component: Platform
• Status: RESOLVED FIXED

[Fujii Hironori]

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

[Fujii Hironori]

Created attachment 429477 [details]
Patch

[Fujii Hironori]

Created attachment 429511 [details]
Patch

[Said Abou-Hallawa]

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?

[Sam Weinig]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 429624 [details]
test case

[Fujii Hironori]

Created attachment 429625 [details]
Patch

[Fujii Hironori]

svg/custom/grayscale-gradient-mask-2.svg is another test case.

[Said Abou-Hallawa]

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.

[Fujii Hironori]

Created attachment 429729 [details]
Patch for landing

[Fujii Hironori]

Created attachment 429731 [details]
Patch for landing

[Fujii Hironori]

Comment on attachment 429731 [details]
Patch for landing

Clearing flags on attachment: 429731

Committed r278078 (238157@main): <https://commits.webkit.org/238157@main>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/78496702>