1366977 - stylo: image has vertical strips of color difference

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Shing Lyu [:shinglyu]]

Attachment: screenshot

Many Alexa top 200 reftest shows this kind of color difference pattern (see the attachment)

On images, there are vertical strips of pixels (dotted) which has very small color differences, e.g. rgb(255, 255,255) => rgb(254, 254, 254)

The pattern is not fixed, it appears in different location on different platform (try server linux 64 vs local machine linux). But they are all vertical strips.

Comment 1

[Bobby Holley (:bholley)]

Manish, any idea what's up with this?

P1 since this likely impacts our Alexa numbers.

Comment 2

[Shing Lyu [:shinglyu]]

Attachment: gamespot.com-home.html.zip (test case)

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

I'm wondering whether there could be some bad interaction between the styling threads and the image decoder thread (IIRC image decoding is also off-main-thread? or maybe still on main thread but just async?)

Another hypothesis is that, perhaps the style system produces a slightly different width which causes different pixel combining from image decoder?

Or maybe the decoder is path-dependent, so if the size ever changes during the reflow, it could produce different final result even if the final size is identical?

Comment 4

[Bobby Holley (:bholley)]

Per this morning's call, Manish is investigating this.

Comment 5

[Manish Goregaokar [:manishearth]]

Same result in STYLO_THREADS=1 mode.

I suspect we're doing some rounding wrong. Would love to be able to diff the computed style tree.

Comment 6

[Manish Goregaokar [:manishearth]]

I diffed the computed style tree; and there's no difference there. getComputedStyle returns the exact same thing for each element in the document.

Though IIRC it rounds things, which might be part of the issue here? Probably need to hack it to not round coords and colors.

Comment 7

[Manish Goregaokar [:manishearth]]

Hmm, so the first div.videoCube.videoCube_3_child element has a small difference on it. Stylo has a width of 47.994999%, whereas Gecko has 47.995003%.

Adding a div.videoCube {display:none} gets rid of the vertical lines, but there are still single-pixel differences elsewhere. However, I have not yet inspected the entire computed style tree (it takes too long to generate), so there may be other discrepancies causing this.

Here's a minimal example of the rounding error:

<div id=foo style="min-width: 47.995%;"></div>
<script type="text/javascript">console.log(getComputedStyle(window.foo).minWidth)</script>

It won't actually show the rounding error until you turn truncation off for getComputedStyle (see nsTSubstring_CharT::AppendFloat).

I suspect we convert percent to app units differently.



-----


Something else is *really* weird here. `getComputedStyle(document.querySelectorAll('div.videoCube.videoCube_3_child')[0]).getPropertyValue('width')` on the gamespot page in dev edition Gecko returns a pixel value. In my nightly build it returns a percentage value. getComputedStyle for CSS 2.0 properties returns the used values, not the computed values. Stylo shouldn't actually be touching getComputedStyle, so I'm not sure where this difference comes from. Furthermore, I can't reproduce this difference for a smaller document. Hm. Anyway, since regular gecko displays this behavior too I'm not too concerned about it wrt this bug. But we should look into this at some point.

Comment 8

[Manish Goregaokar [:manishearth]]

Er, we parse percent wrong here.

Comment 9

[Manish Goregaokar [:manishearth]]

It's another rounding issue. Awesome.

It boils down to the difference between dividing a double by 100 and casting to float or casting to float and then dividing by 100.

println!("{:.10} {:.10}", 47.995f64 as f32 / 100., (47.995f64 / 100.) as f32); return values on either side of 47.995: 0.4799499810 and 0.4799500108.

A hopefully exact copy of the servo and gecko parsing algorithms are at https://gist.github.com/Manishearth/550c4f46c527596c5fb79a5adf49ac4b . They are different as to how they operate. For the integral part, Servo accumulates it into a float (f64) to handle overflows, but Gecko parses it into an int. I presume this won't change the result. When parsing the fractional part, Servo stores a "factor" of (0.1)^n, which it multiplies by. Gecko stores a "divisor" of (10)^n. This will lead to small differences, though not in the case of the given input.

Finally, neither store as f64, they store it as f32. In the case of a percentage, both divide by 100 before storing. Stylo casts first, Gecko divides first.

Note that I'm not sure yet if this is the *only* issue here; I have to try out a fix to be sure.

Comment 10

[Manish Goregaokar [:manishearth]]

https://github.com/servo/rust-cssparser/pull/152 should fix it. Works locally, though the pixel above the [Replay] is still broken for some unknown reason. No longer a vertical stripe issue though. May still look into that.

Comment 11

[Manish Goregaokar [:manishearth]]

Okay, the remaining differences all turn up in stylo vs stylo mode. It's ~25 pixels, so we should just add a fuzziness.

(The test still won't pass because the search/hamburger elements use `<use xlink:href=..>` which I don't think we handle yet)

Comment 12

[Manish Goregaokar [:manishearth]]

Different but similar differences turn up in gecko vs gecko mode. I think the round things are just naturally fuzzy, as is the area around the text.

Comment 13

[Manish Goregaokar [:manishearth]]

This should be fixed on autoland. Shing, has this reduced failures?

Comment 14

[Hiroyuki Ikezoe (:hiro)]

FWIW, there is another rounding issue we've found when computing computed value from specified value.
See bug 1361663 comment 1. And I am confident bug 1367327 was caused by the rounding issue.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

If this hasn't reduced failures, here's something that could explain it. There are very similar differences when the Blink team tried to migrate windows builds to clang, see[1]. Assuming the normal Gecko build wasn't with LLVM/clang++, there could be similar rounding issues.

[1]: https://crbug.com/727463

Comment 16

[Manish Goregaokar [:manishearth]]

Yeah. Ultimately we still use different algorithms to compute it; I only fixed the part that could have the most impact. I don't feel that we should strive to make them exactly the same everywhere.

The gamespot test will still fail because of the <use> tag.

Comment 17

[Shing Lyu [:shinglyu]]

https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/GWTYiiktQ3-8XiZkN93fvQ/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

^ The problem seems to persist in the latest try push. with a slightly different pattern. I believe your patch was released in cssparser 0.13.7, and the version I pushed already have 0.13.7 in its Cargo.lock. 

(In case you want to link back to the try result: 
https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&selectedJob=104448696 )

Also, do you think the color difference around the circular logos has the same root cause as the vertical strip?

Comment 18

[Manish Goregaokar [:manishearth]]

Huh. Yeah, I was getting some weirdness where it disappeared locally but only on *some* try pushes, but I thought that went away. Maybe not.

We do have different rounding rules ultimately, but like I said I don't think we should strive to make them exactly the same.

Regarding the circular play button, IIRC gecko-vs-gecko itself has trouble there. It's possible that gecko-vs-gecko has the vertical stripes too.

Might be worth twiddling with the viewport size till the vertical stripes disappear. I don't enjoy the idea of making these compute to the exact same float value.