1393077 - Handle rounding issue for layers-free mode

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P1)

Description

[Ethan Lin[:ethlin]]

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 1

[Ethan Lin[:ethlin]]

Attachment: Bug 1393077 - Part1. Add IntPointToPoint for Point.

Review commit: https://reviewboard.mozilla.org/r/174760/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174760/

Comment 2

[Ethan Lin[:ethlin]]

Attachment: Bug 1393077 - Part1. Round the transformed rectangles and transformed points in StackingContextHelper.

Review commit: https://reviewboard.mozilla.org/r/174878/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174878/

Comment 3

[Ethan Lin[:ethlin]]

Attachment: Bug 1393077 - Part2. Round the offset for the fallback.

Review commit: https://reviewboard.mozilla.org/r/174880/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174880/

Comment 4

[Ethan Lin[:ethlin]]

Comment on attachment 8902989 [details]
Bug 1393077 - Part1. Add IntPointToPoint for Point.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174760/diff/1-2/

Comment 5

[Ethan Lin[:ethlin]]

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 6

[Ethan Lin[:ethlin]]

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

[Kartikaya Gupta (email:[e-mail])]

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

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8903099 [details]
Bug 1393077 - Part1. Round the transformed rectangles and transformed points in StackingContextHelper.

https://reviewboard.mozilla.org/r/174878/#review179988

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8903100 [details]
Bug 1393077 - Part2. Round the offset for the fallback.

https://reviewboard.mozilla.org/r/174880/#review179990

Comment 10

[Ethan Lin[:ethlin]]

Comment on attachment 8903099 [details]
Bug 1393077 - Part1. Round the transformed rectangles and transformed points in StackingContextHelper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174878/diff/1-2/

Comment 11

[Ethan Lin[:ethlin]]

Comment on attachment 8903100 [details]
Bug 1393077 - Part2. Round the offset for the fallback.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174880/diff/1-2/

Comment 12

[Ethan Lin[:ethlin]]

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 13

[Ethan Lin[:ethlin]]

Attachment: Bug 1393077 - Part3. Modify annotations for affected testcases.

Review commit: https://reviewboard.mozilla.org/r/176744/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/176744/

Comment 14

[Ethan Lin[:ethlin]]

Comment on attachment 8903099 [details]
Bug 1393077 - Part1. Round the transformed rectangles and transformed points in StackingContextHelper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174878/diff/2-3/

Comment 15

[Ethan Lin[:ethlin]]

Comment on attachment 8903100 [details]
Bug 1393077 - Part2. Round the offset for the fallback.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174880/diff/2-3/

Comment 16

[Ethan Lin[:ethlin]]

Comment on attachment 8904935 [details]
Bug 1393077 - Part3. Modify annotations for affected testcases.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/176744/diff/1-2/

Comment 17

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 18

[Ethan Lin[:ethlin]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 19

[Ethan Lin[:ethlin]]

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 20

[Ethan Lin[:ethlin]]

Comment on attachment 8903099 [details]
Bug 1393077 - Part1. Round the transformed rectangles and transformed points in StackingContextHelper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174878/diff/3-4/

Comment 21

[Ethan Lin[:ethlin]]

Comment on attachment 8903100 [details]
Bug 1393077 - Part2. Round the offset for the fallback.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174880/diff/3-4/

Comment 22

[Ethan Lin[:ethlin]]

Comment on attachment 8904935 [details]
Bug 1393077 - Part3. Modify annotations for affected testcases.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/176744/diff/2-3/

Comment 23

[Ethan Lin[:ethlin]]

Comment on attachment 8904935 [details]
Bug 1393077 - Part3. Modify annotations for affected testcases.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/176744/diff/3-4/

Comment 24

[Kartikaya Gupta (email:[e-mail])]

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!

Comment 25

[Ethan Lin[:ethlin]]

The try result looks good. I'll push the patches.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/fa2ccc95cf0c
https://hg.mozilla.org/mozilla-central/rev/fc237de487fd
https://hg.mozilla.org/mozilla-central/rev/fa8562b32dae