Bug 114035

Created attachment 196633 [details] Test case

Created attachment 196634 [details] Better test case (reftest)

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.

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.

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

Note this doesn't repro in Safari. I don't think Safari accelerates the SVG reference filter.

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

Ok, I figured it out. Patch and explanation coming up!

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.

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.

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.

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.

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

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.

This code has been significantly refactored since this patch was proposed. There doesn't seem to be any action we can take here.