When using certain SVG filters, screenshot tool and color picker show a different color to the page rendered normally
Categories
(Core :: Graphics, defect)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox135 | --- | fixed |
People
(Reporter: tcathcartburn, Assigned: tcathcartburn, NeedInfo)
References
Details
Attachments
(5 files, 1 obsolete file)
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:125.0) Gecko/20100101 Firefox/125.0
Steps to reproduce:
Use the color grabber (from the Inspector tab of developer tools) while viewing a webpage including feColorMatrix SVG filters with negative values (such as the one attached) and compare the color of pixels affected by the filter to the color reported by an external color grabber tool.
Alternatively compare the colors in a screenshot using Firefox's internal screenshot tool to the colors in a screenshot created by and external screenshot tool.
Actual results:
The colors differ. In the attached example webpage, I observe the colors #0156ff using Firefox's color grabber, #0055ff it appears to be according to other tools
Expected results:
The color grabber shows the same color as is seen on the page rendered normally (in this case #0055ff).
| Assignee | ||
Comment 1•1 year ago
|
||
I believe this is caused by differences between the software rendering path and the webRender path for SVG filters. In this case, the problem appears to be due to incorrect rounding in gfx/2d/FilterProcessingSIMD-inl.h . I have a fix for this, although there are still cases where the results of the rendering paths differ because webRender appears to use substantially more precision.
Comment 2•1 year ago
|
||
The Bugbug bot thinks this bug should belong to the 'Firefox::Screenshots' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.
Comment 3•1 year ago
|
||
Thanks for filing this bug.
I wasn't able to reproduce this with the page provided on Windows 11. I got #0156ff with the Firefox color picker and in paint.
Are you able to reproduce this issue with screenshots.browser.component.enabled set to true in about:config?
| Assignee | ||
Comment 4•1 year ago
|
||
| Assignee | ||
Comment 5•1 year ago
|
||
| Assignee | ||
Comment 6•1 year ago
|
||
(In reply to Niklas Baumgardner [:niklas] from comment #3)
Thanks for filing this bug.
I wasn't able to reproduce this with the page provided on Windows 11. I got #0156ff with the Firefox color picker and in paint.
Are you able to reproduce this issue with
screenshots.browser.component.enabledset to true in about:config?
Yes. That setting doesn't change anything for me.
Do you observe obvious problems with the diagonal of the middle image of https://penteract.github.io/filterBug/bug.html under the Firefox color picker? I added files showing the difference between what I see when the page is rendered normally (as shown by gnome-screenshot) and what I see using Firefox's screenshot tool (which are the same colors shown under Firefox's color picker).
I didn't use that example in the original bug report despite being more obvious, because it could be argued that the filter there is not one that needs to be supported due to large matrix entries.
Comment 7•1 year ago
|
||
Thanks for sharing that page! I'm able to reproduce the issue now.
I think this is a graphics issue so I'm moving the bug there so they can triage.
Comment 8•1 year ago
|
||
Were you intending to supply a patch? If so, do you need any help with that?
| Assignee | ||
Comment 9•1 year ago
|
||
This is my proposed patch. Should I submit it to phabricator as described in https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html? Who should be a reviewer? mstange appears to be the last person to meaningfully edit the code.
It doesn't fix a similar problem in ArithmeticCombineTwoPixels (corresponding to feComposite with operator='arithmetic') because the simplest fixes there have performance implications, appear wrong, or are wrong for common inputs.
Comment 10•1 year ago
|
||
We do need a phabricator patch. Reviewers are listed here: https://firefox-source-docs.mozilla.org/mots/index.html mstange is a core graphics peer so seems a fine choice.
Can you create a reftest too as part of the patch? Something that passes with your fix and fails without it. You can put the reftest in testing/web-platform/tests/css/filter-effects or layout/reftests/svg/filters The former are cross browser tests and the latter are firefox only tests. cross-browser tests are better. See https://firefox-source-docs.mozilla.org/web-platform/index.html for details of how to create a web platform reftest.
| Assignee | ||
Comment 11•1 year ago
|
||
This doesn't fix a similar problem in ArithmeticCombineTwoPixels because the fixes there have performance implications, appear wrong, or are wrong for common inputs.
Since hue-rotate is defined in terms of a color matrix, some web platform tests involving hue-rotate unexpectedly pass or fail:
Unexpected Results
/css/filter-effects/effect-reference-on-span.html
FAIL
Found 1890 pixels different, maximum difference per channel 95
Also fails in the parent commit
/css/filter-effects/svg-image-root-filter.html
FAIL
Found 400 pixels different, maximum difference per channel 1
Change in rounding behaviour means 92 (0x5c) is reached instead of 91 (0x5b). 91 is more accurate (90.678 or 90.78 is the exact value), but there are only 7 bits of precision in matrix entries between 0 and 1, so errors are inevitable here without substantial changes to the code.
/css/filter-effects/svg-multiple-filter-functions.html
UNEXPECTED-PASS
/css/filter-effects/svg-mutation-single-to-multiple-002.html
UNEXPECTED-PASS
(the updated code now produces 0x80 rather than 0x7f)
Updated•1 year ago
|
Comment 12•1 year ago
|
||
The tests fail because the minimum fuzziness is now 1 pixel and android is not fuzzy at all.
Comment 13•1 year ago
|
||
https://treeherder.mozilla.org/jobs?repo=try&revision=481330e8e36c199b902a16004cb9efcffc6d2cfc
Doesn't seem like any failures are as a result of this patch.
| Assignee | ||
Comment 14•1 year ago
|
||
(In reply to Robert Longson [:longsonr] from comment #13)
https://treeherder.mozilla.org/jobs?repo=try&revision=481330e8e36c199b902a16004cb9efcffc6d2cfc
Doesn't seem like any failures are as a result of this patch.
I would agree. Is there anything I should be doing about this patch, or should I just wait for a review?
Comment 15•1 year ago
|
||
Just the waiting now, unless you want to move onto something else.
Comment 16•1 year ago
|
||
The severity field is not set for this bug.
:bhood, could you have a look please?
For more information, please visit BugBot documentation.
Comment 17•1 year ago
|
||
Comment 19•1 year ago
|
||
Backed out for causing reftest svg/css filters related failures
| Assignee | ||
Comment 21•1 year ago
|
||
I've submitted an updated revision with those tests marked as appropriately fuzzy (https://phabricator.services.mozilla.com/D211113). I've made them unconditionally fuzzy since I don't know what situations cause a particular webRender code path to be chosen nor how to describe that as reftest conditions.
I also changed the tests so that the reference colors match the spec https://www.w3.org/TR/filter-effects/#ShorthandEquivalents to high precision - this is the code I wrote to calculate what they should be: https://gist.github.com/penteract/109f39adf003b2f2c64ad66f575c06f1.
I assume the old values were obtained by using the behavior of Firefox. There may other test cases that have been made that way and should be changed.
This change might cause some configurations to be outside the given interval on those test cases. It might also be possible to shrink the fuzziness intervals, since my fix to the rounding behavior reduces the maximum possible error.
Can I ask how to locally build and run the test configuration "Linux 18.04 x64 WebRender debug" that triggered this?
Comment 22•1 year ago
•
|
||
To run as debug set your mozconfig with something like
ac_add_options --enable-debug
ac_add_options --disable-optimize
./mach reftest layout/reftests/svg/filters/css-filter-chains/long-chain.html would run that reftest for instance, or you can specify a directory to run all the reftests in it.
Or you could apply for try server access. https://www.mozilla.org/en-US/about/governance/policies/commit/ I can vouch for you for that if you wish.
Comment 23•1 year ago
|
||
Comment 24•1 year ago
|
||
Comment 25•1 year ago
|
||
| Assignee | ||
Comment 27•1 year ago
|
||
I've updated the again, increasing the fuzziness for what is hopefully the last failing test.
I wasn't able to observe the test failure myself by putting the suggested options into my mozconfig and rebuilding.
Comment 28•1 year ago
|
||
Comment 29•1 year ago
|
||
Updated•1 year ago
|
Comment 30•1 year ago
|
||
| Assignee | ||
Comment 31•1 year ago
|
||
(In reply to Robert Longson [:longsonr] from comment #30)
https://treeherder.mozilla.org/jobs?repo=try&revision=47aaeca5823df85464e7a45cf217d5c1fcaa9193
Is that treeherder job testing my patch? I only see the patch with the description No try selector specified, use "Add New Jobs" to select tasks. being applied there and the test failures aren't like previous ones.
Comment 32•1 year ago
|
||
If you click on that and select parent you'll get to https://hg.mozilla.org/try/rev/4e11194543c781a0a11bee3d5eddb4aca3f0c529
Note that other filter changes landed - that's why your change didn't apply, I merged the changes and that's what I came up with. It has a WPT failure.
| Assignee | ||
Comment 33•1 year ago
|
||
(In reply to Robert Longson [:longsonr] from comment #32)
If you click on that and select parent you'll get to https://hg.mozilla.org/try/rev/4e11194543c781a0a11bee3d5eddb4aca3f0c529
Note that other filter changes landed - that's why your change didn't apply, I merged the changes and that's what I came up with. It has a WPT failure.
Thanks for explaining that. I assume the only test failure you'd attribute to this patch is /css/filter-effects/svg-mutation-single-to-multiple-002.html.
I ran the test locally and it looks like the conflicting changeset (https://searchfox.org/mozilla-central/commit/cac93eda001fe052c9f7f4f6166d534f9755a58f) is the reason why this test is now failing. Immediately before that revision I seeFound 5000 pixels different, maximum difference per channel 1; after it is applied, I see Found 4500 pixels different, maximum difference per channel 255 like in the treeherder link you posted. Since the test was just marked as expected: FAIL, they wouldn't have been alerted about it.
To fix the test failure for this patch, all that needs to happen is un-removing the file testing/web-platform/meta/css/filter-effects/svg-mutation-single-to-multiple-002.html.ini, but that's only appropriate to do as part of a merge or rebase. How should I resubmit to phabricator after merging using mercurial?
The conflicting change makes some reftests a bit less fuzzy (particularly in layout/reftests/svg/filters/svg-filter-chains/) and it would be a shame to lose that reduction in fuzziness. I've applied for try server access that I could use to get those right.
Comment 34•1 year ago
|
||
resubmit to phabricator after merging with mercurial
Comment 35•1 year ago
|
||
Comment 36•1 year ago
|
||
Comment 37•1 year ago
|
||
Backed out for causing wr failures @svg-multiple-filter-functions.html.
Comment 39•1 year ago
|
||
Comment 40•1 year ago
|
||
| bugherder | ||
Description
•