REGRESSION (r147502): Accelerated reference filters are broken

• Component: CSS
• Status: RESOLVED CONFIGURATION CHANGED

[Stephen White]

Accelerated reference filters no longer work after r147502 when added dynamically to an element's style.

[Stephen White]

Created attachment 196633 [details]
Test case

[Stephen White]

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

[Max Vujovic]

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]

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]

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

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

[Stephen White]

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

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

[Max Vujovic]

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]

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 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]

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]

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

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]

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