Handle rounding issue for layers-free mode

RESOLVED FIXED in mozilla57

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ethlin, Assigned: ethlin)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 unaffected, firefox57 unaffected)

Details

(Whiteboard: [wr-mvp])

Attachments

(3 attachments, 1 obsolete attachment)

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
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.
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 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 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+
Attachment #8902989 - Attachment is obsolete: true
According to the try result, I should add fuzzy-if or change the fuzz number.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c3e2aba97bd1ae9950ab461fbb3e9fbeb40f8f6
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [wr-mvp]
Target Milestone: --- → mozilla57
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-
(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.
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.
Attachment #8904935 - Flags: review?(bugmail)
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+
The try result looks good. I'll push the patches.
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
Duplicate of this bug: 1366692
You need to log in before you can comment on or make changes to this bug.