Closed
Bug 1393077
Opened 7 years ago
Closed 7 years ago
Handle rounding issue for layers-free mode
Categories
(Core :: Graphics: WebRender, enhancement, P1)
Core
Graphics: WebRender
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox56 | --- | unaffected |
firefox57 | --- | unaffected |
People
(Reporter: ethlin, Assigned: ethlin)
References
Details
(Whiteboard: [wr-mvp])
Attachments
(3 files, 1 obsolete file)
According to the try result[1], there are many failures are about rounding issue. I think we should round the bounds before sending WR commands. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=1df2ac1e4b07e0e1b2607f881e3b3aa4eded801e&selectedJob=125124356
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
I tested "layout/reftests/pixel-rounding/reftest.list". The result became "Successful: 149, Unexpected: 1" with the patches. Almost all are fixed. Basically my idea is just rounding all final rectangles.
Assignee | ||
Comment 6•7 years ago
|
||
The original try result is [1]. There are many pixel-rounding failures. The new try result is [2]. There is only 1 exception of pixel-rounding. Actually it's unexpected-pass, so I think all rounding tests are passed. [1] https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/UuWU3_ahT9WV6yei64LFpg/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1 [2] https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/fMH5pM2USjiwd12CHNECOQ/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8902989 [details] Bug 1393077 - Part1. Add IntPointToPoint for Point. https://reviewboard.mozilla.org/r/174760/#review179986 I don't think you need this, there is already a Point constructor that takes an IntPoint so that should be sufficient: http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/gfx/2d/Point.h#132
Attachment #8902989 -
Flags: review?(bugmail)
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8903099 [details] Bug 1393077 - Part1. Round the transformed rectangles and transformed points in StackingContextHelper. https://reviewboard.mozilla.org/r/174878/#review179988
Attachment #8903099 -
Flags: review?(bugmail) → review+
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8903100 [details] Bug 1393077 - Part2. Round the offset for the fallback. https://reviewboard.mozilla.org/r/174880/#review179990
Attachment #8903100 -
Flags: review?(bugmail) → review+
Updated•7 years ago
|
Blocks: stage-wr-nightly
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8902989 -
Attachment is obsolete: true
Assignee | ||
Comment 12•7 years ago
|
||
According to the try result, I should add fuzzy-if or change the fuzz number. https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c3e2aba97bd1ae9950ab461fbb3e9fbeb40f8f6
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Priority: -- → P3
Updated•7 years ago
|
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [wr-mvp]
Target Milestone: --- → mozilla57
Updated•7 years ago
|
status-firefox56:
--- → unaffected
status-firefox57:
--- → unaffected
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8904935 [details] Bug 1393077 - Part3. Modify annotations for affected testcases. https://reviewboard.mozilla.org/r/176744/#review183266 Given the number of failures this is causing we might want to reconsider landing these patches, maybe do the rounding somewhere else in the code. See comments below. ::: layout/reftests/image/reftest.list:3 (Diff revision 2) > -== image-seam-1a.html image-seam-1-ref.html > -== image-seam-1b.html image-seam-1-ref.html > -fuzzy-if(Android,255,154) fuzzy-if(webrender,0-1,0-400) == image-seam-2.html image-seam-2-ref.html # Bug 1128229 > +fuzzy-if(webrender,255-255,77-77) == image-seam-1a.html image-seam-1-ref.html > +fuzzy-if(webrender,255-255,77-77) == image-seam-1b.html image-seam-1-ref.html > +fuzzy-if(Android,255,154) fuzzy-if(webrender,255-255,97-97) == image-seam-2.html image-seam-2-ref.html # Bug 1128229 It looks like these tests are actually testing for precise positioning of object elements, and the reftest analyzer shows that the positions are off between the test and reference images. I don't think this can be fuzzed away, we should mark these fails-if ::: layout/reftests/w3c-css/submitted/images3/reftest.list:174 (Diff revision 2) > -fails-if(!styloVsGecko&&!webrender) == object-position-png-001c.html object-position-png-001-ref.html # bug 1105150 > -== object-position-png-001e.html object-position-png-001-ref.html > -== object-position-png-001i.html object-position-png-001-ref.html > -== object-position-png-001o.html object-position-png-001-ref.html > -== object-position-png-001p.html object-position-png-001-ref.html > -fails-if(!styloVsGecko&&!webrender) == object-position-png-002c.html object-position-png-002-ref.html # bug 1105150 > -== object-position-png-002e.html object-position-png-002-ref.html > -== object-position-png-002i.html object-position-png-002-ref.html > -== object-position-png-002o.html object-position-png-002-ref.html > -== object-position-png-002p.html object-position-png-002-ref.html > +fuzzy-if(webrender,211-211,100-100) fails-if(!styloVsGecko) == object-position-png-001c.html object-position-png-001-ref.html # bug 1105150 > +fuzzy-if(webrender,211-211,100-100) == object-position-png-001e.html object-position-png-001-ref.html > +fuzzy-if(webrender,211-211,100-100) == object-position-png-001i.html object-position-png-001-ref.html > +fuzzy-if(webrender,211-211,100-100) == object-position-png-001o.html object-position-png-001-ref.html > +fuzzy-if(webrender,211-211,100-100) == object-position-png-001p.html object-position-png-001-ref.html > +fuzzy-if(webrender,211-211,100-100) fails-if(!styloVsGecko) == object-position-png-002c.html object-position-png-002-ref.html # bug 1105150 > +fuzzy-if(webrender,211-211,100-100) == object-position-png-002e.html object-position-png-002-ref.html > +fuzzy-if(webrender,211-211,100-100) == object-position-png-002i.html object-position-png-002-ref.html > +fuzzy-if(webrender,211-211,100-100) == object-position-png-002o.html object-position-png-002-ref.html > +fuzzy-if(webrender,211-211,100-100) == object-position-png-002p.html object-position-png-002-ref.html I think these should also be marked fails-if ::: layout/reftests/xul/reftest.list:62 (Diff revision 2) > -fuzzy-if(webrender,16,20) == object-position-png-001.xul object-position-png-001-ref.html > -== object-position-png-002.xul object-position-png-002-ref.html > +fuzzy-if(webrender,211-211,100-100) == object-position-png-001.xul object-position-png-001-ref.html > +fuzzy-if(webrender,211-211,100-100) == object-position-png-002.xul object-position-png-002-ref.html These as well ::: layout/xul/reftest/reftest.list:4 (Diff revision 2) > fails-if(Android) == textbox-multiline-noresize.xul textbox-multiline-ref.xul # reference is blank on Android (due to no native theme support?) > != textbox-multiline-resize.xul textbox-multiline-ref.xul > == popup-explicit-size.xul popup-explicit-size-ref.xul > -random-if(Android) fuzzy-if(webrender,21-21,10746-10746) == image-size.xul image-size-ref.xul > +random-if(Android) fuzzy-if(webrender,255-255,10850-10850) == image-size.xul image-size-ref.xul And this one. The existing fuzz on this was due to slight color differences, but new fuzz is positioning which I think is more important.
Attachment #8904935 -
Flags: review?(bugmail) → review-
Assignee | ||
Comment 18•7 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17) > Comment on attachment 8904935 [details] > Bug 1393077 - Part3. Modify annotations for affected testcases. > > https://reviewboard.mozilla.org/r/176744/#review183266 > > Given the number of failures this is causing we might want to reconsider > landing these patches, maybe do the rounding somewhere else in the code. See > comments below. > I see. I will investigate more before landing patches. I may set fails-if for some tests that are passed in layers-free mode.
Assignee | ||
Comment 19•7 years ago
|
||
After turning on layers-free, all position problems disappear. So I set fails-if for those tests and add corresponding tests with layers-free on.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8904935 -
Flags: review?(bugmail)
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8904935 [details] Bug 1393077 - Part3. Modify annotations for affected testcases. https://reviewboard.mozilla.org/r/176744/#review183748 This seems reasonable if it's passing try. Thanks!
Attachment #8904935 -
Flags: review+
Assignee | ||
Comment 25•7 years ago
|
||
The try result looks good. I'll push the patches.
Comment 26•7 years ago
|
||
Pushed by ethlin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fa2ccc95cf0c Part1. Round the transformed rectangles and transformed points in StackingContextHelper. r=kats https://hg.mozilla.org/integration/autoland/rev/fc237de487fd Part2. Round the offset for the fallback. r=kats https://hg.mozilla.org/integration/autoland/rev/fa8562b32dae Part3. Modify annotations for affected testcases. r=kats
Comment 27•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fa2ccc95cf0c https://hg.mozilla.org/mozilla-central/rev/fc237de487fd https://hg.mozilla.org/mozilla-central/rev/fa8562b32dae
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•