RESOLVED CONFIGURATION CHANGED 114035
REGRESSION (r147502): Accelerated reference filters are broken
https://bugs.webkit.org/show_bug.cgi?id=114035
Summary REGRESSION (r147502): Accelerated reference filters are broken
Stephen White
Reported 2013-04-05 07:52:10 PDT
Accelerated reference filters no longer work after r147502 when added dynamically to an element's style.
Attachments
Test case (665 bytes, patch)
2013-04-05 07:52 PDT, Stephen White
no flags
Better test case (reftest) (1.16 KB, patch)
2013-04-05 08:23 PDT, Stephen White
darin: review-
darin: commit-queue-
Patch (2.48 KB, patch)
2013-04-05 12:18 PDT, Max Vujovic
mvujovic: commit-queue-
Patch (2.12 KB, patch)
2013-04-12 15:57 PDT, Max Vujovic
mvujovic: commit-queue-
Stephen White
Comment 1 2013-04-05 07:52:42 PDT
Created attachment 196633 [details] Test case
Stephen White
Comment 2 2013-04-05 08:23:40 PDT
Created attachment 196634 [details] Better test case (reftest)
Max Vujovic
Comment 3 2013-04-05 08:58:20 PDT
Thanks for the test cases! First idea that comes to mind is that we might need to put a RenderLayerBacking::updateFilters call back in updateGraphicsLayerGeometry. I'll investigate.
Stephen White
Comment 4 2013-04-05 09:53:17 PDT
Just to let you know, I'm probably going to revert r147502 in blink for now. Once you get a fix, let me know and we can reland it.
Max Vujovic
Comment 5 2013-04-05 09:58:06 PDT
(In reply to comment #4) > Just to let you know, I'm probably going to revert r147502 in blink for now. > > Once you get a fix, let me know and we can reland it. If this isn't blocking your work for today, I'm working on a fix. If it's blocking, go ahead and revert. Sorry!
Max Vujovic
Comment 6 2013-04-05 10:37:31 PDT
Note this doesn't repro in Safari. I don't think Safari accelerates the SVG reference filter.
Stephen White
Comment 7 2013-04-05 10:41:28 PDT
(In reply to comment #6) > Note this doesn't repro in Safari. I don't think Safari accelerates the SVG reference filter. No, only Chrome does. Safari falls back to the non-composited path for reference filters, AFAIK.
Max Vujovic
Comment 8 2013-04-05 11:33:23 PDT
Ok, I figured it out. Patch and explanation coming up!
Max Vujovic
Comment 9 2013-04-05 12:18:03 PDT
Created attachment 196664 [details] Patch The problem is that we rely on FilterEffectRenderer to build the filter graph, and we call backing()->updateFilters() before we have a FilterEffectRenderer. Here's what's happening now in RenderLayer::updateFilters: 1) Call backing()->updateFilters() to see if we can paint the filters in the compositor. It returns true, but the filter graph is not built yet, so the compositor doesn't get it. 2) Call updateOrRemoveFilterEffectRenderer(), which creates a FilterEffectRenderer if we're painting filters in software (we're not) or if we have an SVG reference filter (we do). The simplest fix right now is to call backing()->updateFilters() again as step #3, after we have a FilterEffectRenderer. Before, when we called updateFilters() in RenderLayerBacking::updateGraphicsLayerGeometry, the call happened late enough to have a FilterEffectRenderer for SVG reference filters, but too late to fall back to software filter painting. The long-term fix is to extract the SVG graph building functionality out of FilterEffectRenderer, and make it happen in RenderLayer::computeFilterOperations. We can take our time to fix this correctly in WebKit, since no port is accelerating SVG reference filters right now. I've filed a bug for that: https://bugs.webkit.org/show_bug.cgi?id=114051 For Blink, this patch should fix it right now until Blink can get the long-term fix. There are additional comments in the patch.
Stephen White
Comment 10 2013-04-12 09:43:24 PDT
Although the bug only affects blink currently (and has been fixed there), it might be a good idea to land the test here in WebKit, just in case someone runs into the same problem here. I leave it up to WebKit reviewers to decide.
Darin Adler
Comment 11 2013-04-12 11:50:51 PDT
Comment on attachment 196634 [details] Better test case (reftest) Test looks OK to me, but even a test needs a change log for review and commit.
Darin Adler
Comment 12 2013-04-12 11:51:33 PDT
It would really be good to get this reftest landed. Someone could do the work to add a change log entry and get it in.
Max Vujovic
Comment 13 2013-04-12 12:44:41 PDT
(In reply to comment #12) > It would really be good to get this reftest landed. Someone could do the work to add a change log entry and get it in. I'd be happy to write the ChangeLog entry and reupload.
Max Vujovic
Comment 14 2013-04-12 15:57:42 PDT
Created attachment 197900 [details] Patch Added ChangeLog to senorblanco's test. However, the test is failing locally on Safari due, seemingly due to color space and/or precision issues. In the PNGs generated by DRT: The expected PNG has a color of RGB(0, 249, 0) in Photoshop. The actual png has a color of RGB(2, 249, 0) in Photoshop. When I load the files locally: The expected file has a native color of RGB(0, 255, 0). The test file has a native color of RGB(145, 249, 45) on my monitor, and depends on color space. This needs some more investigation before landing, or the quick "fix" is to make it a pixel test instead of a reftest.
Brent Fulgham
Comment 15 2022-07-13 15:30:13 PDT
This code has been significantly refactored since this patch was proposed. There doesn't seem to be any action we can take here.
Note You need to log in before you can comment on or make changes to this bug.