Bug 225907 – ASSERTION FAILED: m_imageBufferResult->colorSpace() == m_resultColorSpace in FilterEffect::copyPremultipliedResult

• Component: Platform
• Status: RESOLVED FIXED

[Fujii Hironori]

ASSERTION FAILED: m_imageBufferResult->colorSpace() == m_resultColorSpace in FilterEffect::copyPremultipliedResult

WinCairo WK1/WK2 Debug

ASSERTION FAILED: m_imageBufferResult->colorSpace() == m_resultColorSpace
C:\home\webkit\gb\Source\WebCore\platform/graphics/filters/FilterEffect.cpp(547) : WebCore::FilterEffect::copyPremultipliedResult

  css3/filters/color-interpolation-filters.html [ Crash ]
  css3/filters/effect-reference.html [ Crash ]
  css3/filters/invalidate-sourceAlpha.html [ Crash ]
  css3/filters/reference-filter-set-filter-regions.html [ Crash ]

[Fujii Hironori]

237800@main (r277572): first bad
https://build.webkit.org/#/builders/32/builds/414

237795@main (r277567): last good 
https://build.webkit.org/#/builders/32/builds/413

r277569 (Bug 225841) seems the culprit.

[Fujii Hironori]

Created attachment 428914 [details]
invalidate-sourceAlpha-crash-log.txt (WinCairo WK1 Debug)

[Fujii Hironori]

FilterEffect::transformResultColorSpace does nothing if USE(CG). It calls ImageBuffer::transformColorSpace otherwise.
https://github.com/WebKit/WebKit/blob/3de6f842827f19bb0b6de1068d88026e8868cf26/Source/WebCore/platform/graphics/filters/FilterEffect.cpp#L641

There are three problems.

1. ImageBuffer::colorSpace() returns the old color space even after ImageBuffer::transformColorSpace.
2. ImageBufferBackend::colorSpace() returns the old color space even after ImageBuffer::transformColorSpace.
3. convertImagePixels doesn't support color space conversion yet for USE(ACCELERATE) && USE(CG).
   https://github.com/WebKit/WebKit/blob/3de6f842827f19bb0b6de1068d88026e8868cf26/Source/WebCore/platform/graphics/PixelBufferConversion.cpp#L254

[Fujii Hironori]

Created attachment 428983 [details]
WIP patch

[Sam Weinig]

(In reply to Fujii Hironori from comment #3)
> 3. convertImagePixels doesn't support color space conversion yet for
> USE(ACCELERATE) && USE(CG).
>   
> https://github.com/WebKit/WebKit/blob/
> 3de6f842827f19bb0b6de1068d88026e8868cf26/Source/WebCore/platform/graphics/
> PixelBufferConversion.cpp#L254

Is convertImagePixels() actually being used for any color space conversions on non-CoreGraphics ports? My intent was that it wouldn't be, but I could have easily looked over something?

I think it would be great if we could converge on a single color space transformation tactic for both CoreGraphics and non-CoreGraphics platforms. I happen to like the CoreGraphics approach which is that ImageBuffers have a color space that is immutable, and if you want to change the color space, you draw into another ImageBuffer which has the right color space and the conversion is done during the drawing.

[Fujii Hironori]

Created attachment 429094 [details]
color-interpolation-filters-crash-log.txt (WinCairo WK1 Debug with attachment#428983 [details])

(In reply to Sam Weinig from comment #5)
> Is convertImagePixels() actually being used for any color space conversions
> on non-CoreGraphics ports? My intent was that it wouldn't be, but I could
> have easily looked over something?

Even after comment#4 patch, the assertion in convertImagePixels fails by running css3/filters tests.

FEColorMatrix::platformApplySoftware is explicitly specifying DestinationColorSpace::SRGB.
https://github.com/WebKit/WebKit/blob/3de6f842827f19bb0b6de1068d88026e8868cf26/Source/WebCore/platform/graphics/filters/FEColorMatrix.cpp#L288

[Fujii Hironori]

Created attachment 429132 [details]
WIP patch

[Fujii Hironori]

Created attachment 429137 [details]
WIP patch

[Fujii Hironori]

Bug 226061 – [WinCairo] Disable DESTINATION_COLOR_SPACE_LINEAR_SRGB temporarily until Bug 225907 is fixed

[Fujii Hironori]

Created attachment 429266 [details]
Patch

[Said Abou-Hallawa]

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]

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

Sounds a good idea. But, I want to fix this crashing problem ASAP. Can I do that it as a separate patch?

[Fujii Hironori]

Comment on attachment 429266 [details]
Patch

Clearing flags on attachment: 429266

Committed r277878 (238016@main): <https://commits.webkit.org/238016@main>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/78324805>

[Fujii Hironori]

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
>>> +
>> 
>> 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.
> 
> Sounds a good idea. But, I want to fix this crashing problem ASAP. Can I do that it as a separate patch?

Filed: Bug 226108 – Rename ImageBuffer::transformColorSpace to transformToColorSpace, and it should take a single argument