Bug 114035 - REGRESSION (r147502): Accelerated reference filters are broken
Summary: REGRESSION (r147502): Accelerated reference filters are broken
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Max Vujovic
URL:
Keywords:
Depends on: 114051
Blocks: 68469
  Show dependency treegraph
 
Reported: 2013-04-05 07:52 PDT by Stephen White
Modified: 2014-03-02 09:07 PST (History)
5 users (show)

See Also:


Attachments
Test case (665 bytes, patch)
2013-04-05 07:52 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Better test case (reftest) (1.16 KB, patch)
2013-04-05 08:23 PDT, Stephen White
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
Patch (2.48 KB, patch)
2013-04-05 12:18 PDT, Max Vujovic
mvujovic: commit-queue-
Details | Formatted Diff | Diff
Patch (2.12 KB, patch)
2013-04-12 15:57 PDT, Max Vujovic
mvujovic: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen White 2013-04-05 07:52:10 PDT
Accelerated reference filters no longer work after r147502 when added dynamically to an element's style.
Comment 1 Stephen White 2013-04-05 07:52:42 PDT
Created attachment 196633 [details]
Test case
Comment 2 Stephen White 2013-04-05 08:23:40 PDT
Created attachment 196634 [details]
Better test case (reftest)
Comment 3 Max Vujovic 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.
Comment 4 Stephen White 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.
Comment 5 Max Vujovic 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!
Comment 6 Max Vujovic 2013-04-05 10:37:31 PDT
Note this doesn't repro in Safari. I don't think Safari accelerates the SVG reference filter.
Comment 7 Stephen White 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.
Comment 8 Max Vujovic 2013-04-05 11:33:23 PDT
Ok, I figured it out. Patch and explanation coming up!
Comment 9 Max Vujovic 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.
Comment 10 Stephen White 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.
Comment 11 Darin Adler 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.
Comment 12 Darin Adler 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.
Comment 13 Max Vujovic 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.
Comment 14 Max Vujovic 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.