Bug 225907

Summary: ASSERTION FAILED: m_imageBufferResult->colorSpace() == m_resultColorSpace in FilterEffect::copyPremultipliedResult
Product: WebKit Reporter: Fujii Hironori <fujii.hironori>
Component: PlatformAssignee: Fujii Hironori <fujii.hironori>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, dpino, ews-watchlist, kondapallykalyan, sabouhallawa, sam, webkit-bug-importer, zimmermann
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 225841    
Bug Blocks:    
Attachments:
Description Flags
invalidate-sourceAlpha-crash-log.txt (WinCairo WK1 Debug)
none
WIP patch
none
color-interpolation-filters-crash-log.txt (WinCairo WK1 Debug with attachment#428983)
none
WIP patch
none
WIP patch
none
Patch none

Fujii Hironori
Reported 2021-05-17 22:52:09 PDT
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 ]
Attachments
invalidate-sourceAlpha-crash-log.txt (WinCairo WK1 Debug) (71.55 KB, text/plain)
2021-05-17 23:05 PDT, Fujii Hironori 		no flags
Details
WIP patch (1.32 KB, patch)
2021-05-18 15:03 PDT, Fujii Hironori 		no flags
DetailsFormatted DiffDiff
color-interpolation-filters-crash-log.txt (WinCairo WK1 Debug with attachment#428983) (352.94 KB, text/plain)
2021-05-19 13:52 PDT, Fujii Hironori 		no flags
Details
WIP patch (2.20 KB, patch)
2021-05-19 21:52 PDT, Fujii Hironori 		no flags
DetailsFormatted DiffDiff
WIP patch (3.66 KB, patch)
2021-05-19 22:32 PDT, Fujii Hironori 		no flags
DetailsFormatted DiffDiff
Patch (6.13 KB, patch)
2021-05-20 22:59 PDT, Fujii Hironori 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Fujii Hironori
Comment 2 2021-05-17 23:05:42 PDT
Created attachment 428914 [details] invalidate-sourceAlpha-crash-log.txt (WinCairo WK1 Debug)
Fujii Hironori
Comment 3 2021-05-18 15:02:12 PDT
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
Comment 4 2021-05-18 15:03:49 PDT
Created attachment 428983 [details] WIP patch
Sam Weinig
Comment 5 2021-05-18 16:10:34 PDT
(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
Comment 6 2021-05-19 13:52:11 PDT
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
Comment 7 2021-05-19 21:52:03 PDT
Created attachment 429132 [details] WIP patch
Fujii Hironori
Comment 8 2021-05-19 22:32:53 PDT
Created attachment 429137 [details] WIP patch
Fujii Hironori
Comment 9 2021-05-20 18:18:43 PDT
Bug 226061 – [WinCairo] Disable DESTINATION_COLOR_SPACE_LINEAR_SRGB temporarily until Bug 225907 is fixed
Fujii Hironori
Comment 10 2021-05-20 22:59:48 PDT
Created attachment 429266 [details] Patch
Said Abou-Hallawa
Comment 11 2021-05-20 23:12:34 PDT
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 12 2021-05-20 23:21:38 PDT
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 13 2021-05-21 13:17:39 PDT
Comment on attachment 429266 [details] Patch Clearing flags on attachment: 429266 Committed r277878 (238016@main): <https://commits.webkit.org/238016@main>
Fujii Hironori
Comment 14 2021-05-21 13:17:43 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 15 2021-05-21 13:18:22 PDT
<rdar://problem/78324805>
Fujii Hironori
Comment 16 2021-05-21 13:35:26 PDT
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
Note You need to log in before you can comment on or make changes to this bug.