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 ]
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.
Created attachment 428914 [details] invalidate-sourceAlpha-crash-log.txt (WinCairo WK1 Debug)
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
Created attachment 428983 [details] WIP patch
(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.
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
Created attachment 429132 [details] WIP patch
Created attachment 429137 [details] WIP patch
Bug 226061 – [WinCairo] Disable DESTINATION_COLOR_SPACE_LINEAR_SRGB temporarily until Bug 225907 is fixed
Created attachment 429266 [details] Patch
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.
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?
Comment on attachment 429266 [details] Patch Clearing flags on attachment: 429266 Committed r277878 (238016@main): <https://commits.webkit.org/238016@main>
All reviewed patches have been landed. Closing bug.
<rdar://problem/78324805>
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