Closed Bug 1897783 Opened 1 year ago Closed 1 year ago

When using certain SVG filters, screenshot tool and color picker show a different color to the page rendered normally

Categories

(Core :: Graphics, defect)

Firefox 125
defect

Tracking

()

RESOLVED FIXED
135 Branch
Tracking Status
firefox135 --- fixed

People

(Reporter: tcathcartburn, Assigned: tcathcartburn, NeedInfo)

References

Details

Attachments

(5 files, 1 obsolete file)

Attached file negativeFilter.html

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

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.

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.

Component: Untriaged → Screenshots

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?

Flags: needinfo?(tcathcartburn)

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

Flags: needinfo?(tcathcartburn)

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.

Component: Screenshots → Graphics
Product: Firefox → Core

Were you intending to supply a patch? If so, do you need any help with that?

Flags: needinfo?(tcathcartburn)

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.

Flags: needinfo?(tcathcartburn)

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.

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)

Assignee: nobody → tcathcartburn
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

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

Just the waiting now, unless you want to move onto something else.

The severity field is not set for this bug.
:bhood, could you have a look please?

For more information, please visit BugBot documentation.

Flags: needinfo?(bhood)
Pushed by longsonr@gmail.com: https://hg.mozilla.org/integration/autoland/rev/084a189b3e01 Fix incorrect rounding in software rendering of feColorMatrix. r=longsonr
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/46842 for changes under testing/web-platform/tests

Backed out for causing reftest svg/css filters related failures

Backout link

Push with failures

Failure log // Failure log 2

Flags: needinfo?(tcathcartburn)
Upstream PR was closed without merging

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?

Flags: needinfo?(tcathcartburn)

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.

Pushed by longsonr@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c8f96b912b44 Fix incorrect rounding in software rendering of feColorMatrix. r=longsonr

Backed out for causing reftest failures

Backout link

Push with failures

Failure log

Flags: needinfo?(tcathcartburn)
Upstream PR was closed without merging

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.

Flags: needinfo?(tcathcartburn)
Attachment #9409349 - Attachment is obsolete: true

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

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.

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

resubmit to phabricator after merging with mercurial

Pushed by longsonr@gmail.com: https://hg.mozilla.org/integration/autoland/rev/bb4c90542990 Fix incorrect rounding in software rendering of feColorMatrix. r=longsonr

Backed out for causing wr failures @svg-multiple-filter-functions.html.

Flags: needinfo?(tcathcartburn)
Regressions: 1936695
Upstream PR was closed without merging
Pushed by longsonr@gmail.com: https://hg.mozilla.org/integration/autoland/rev/89be5f493531 Fix incorrect rounding in software rendering of feColorMatrix. r=longsonr
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 135 Branch
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: