732535 - layout/reftests/border-radius has 4 tests that fail on android when we turn browser.tabs.remote=false

Status: RESOLVED WONTFIX

(Core :: Graphics: Layers, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

On android xul, we run with e10s and our reftest harness will setup the xul window to have the remote=true attribute.  For native fennec we have the preference to turn on e10s set, but we don't read it.  This means that reftest is still thinking we are remote, but in reality we are not.  

In trying to turn this off, I found that we have 1 new test which fails and 3 tests which unexpected pass because we previously had fails-if(android) in the manifest.  

unexpected-pass:
clipping-4-image.html
clipping-5-refi.html
intersecting-clipping-1-refi.html

failed:
curved-border-background-nogap.html


The log file showing this is here:
http://people.mozilla.org/~jmaher/android_reftests/border-radius.log

Comment 1

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

The failure is a single pixel that's rgb(1, 1, 1) when it should be rgb(0, 0, 0).

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Could this be related to viewport settings or maybe some preference?  I really don't know anything about this, but I would like to see us start fixing some of these problems.

Comment 3

[Geoff Brown [:gbrown]]

I have confirmed jmaher's results and particularly that curved-border-background-nogap.html succeeds if remote=true and fails if remote=false:

    gBrowser = gContainingWindow.document.createElementNS(XUL_NS, "xul:browser");
      ...
  - gBrowser.setAttribute("remote", gBrowserIsRemote ? "true" : "false");
  + gBrowser.setAttribute("remote", "false");


Searching mxr for getAttribute("remote" has not turned up any plausible explanation for the failure. (Thoughts, suggestions welcome!)

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Jet, this looks like something we'll need your team to look at.

Comment 5

[Geoff Brown [:gbrown]]

These tests are passing today (with remote=false)! Does anyone know why?

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

we have 4 tests that fail on a recent build:
clipping-4-canvas.html
clipping-5-refc.html
intersecting-clipping-1-refc.html
scroll-1.html

Comment 7

[Karl Tomlinson (:karlt)]

What these tests have in common is that a border-radius clip on an active layer, which uses a mask, is compared with a clip on an inactive layer.

Comment 9

[Nick Cameron [:nrc]]

Using a mask layer (as clipping to border-radius does) uses a very different method of clipping than our usual path and so we usually end up with ever so slightly different results. We had to fuzz a fair few tests to get them passing at the time. I imagine that if something has changed about the way we use graphics memory etc. Then we could get a different number of pixels different, in which case things just need fuzzing. If it is a lot of pixels, then maybe mask layers got broken. I'll look into it.

Comment 10

[Nick Cameron [:nrc]]

Oh, if anyone has a link to a failing Try run, that would be helpful, thanks!

Comment 11

[Karl Tomlinson (:karlt)]

I was looking at the same failures with GL/X11 layers.

There the difference was due to image surfaces used (for the mask) but XLib surfaces when applying clips to other drawing.  Image surfaces use a Tor scan convertor, while xlib surfaces use Bentley Ottman tesselation to generate trapezoids for X.  Differences are up to 29/255, but image surfaces are the right thing to use for GL masks, so I think we'll just mark the fuzz.

Using an X11 surface for the mask reduced differences to 0 or 1/255.  The remainder may be due to rounding in GL transforms or implementation</speculation>.

Android uses image surfaces even for drawing, so I would have expected the same algorithms to be used, and so this doesn't the explain failures there.
I don't have an Android log, sorry.

Comment 12

[Nick Cameron [:nrc]]

Oh, so on closer inspection I am not sure what needs doing here, the tests are already fuzzed to pass and I think they should stay like that. So I don't think there is anything to do here and we should just close the bug.

I expect that masking using a shader could give different results than clipping on the CPU due to some optimisations on the graphics hardware. This might particularly be the case if we are using 565 for some layer (which I think we do for reftests somewhere). Or possibly they are using different algorithms for anti-aliasing on gpu vs cpu. All guesses.

Comment 13

[Karl Tomlinson (:karlt)]

If the same drawing backend is used, which I expect to be the case for Android, the algorithm should be very similar when applying the clip on the mask surface as when applying the clip when drawing the background.

These tests are marked "fails-if(Android&&!browserIsRemote)".

Comment 14

[Karl Tomlinson (:karlt)]

(In reply to Nick Cameron [:nrc] from comment #12)
> I expect that masking using a shader could give different results than
> clipping on the CPU due to some optimisations on the graphics hardware. This
> might particularly be the case if we are using 565 for some layer (which I
> think we do for reftests somewhere).

Sorry, didn't read this carefully enough.

Yes, RGB565 for drawing vs an A8 mask, or perhaps less precision in the GPU might explain this.

Comment 15

[Nick Cameron [:nrc]]

Try push with new fuzz and no fails-if: https://tbpl.mozilla.org/?tree=Try&rev=473ff19cf8d2

I'm not sure when exactly we get remote and non-remote though, so does this cover our bases (Android, Android ARMv6, Android NoIon, B2G)?

Comment 16

[Nick Cameron [:nrc]]

Attachment: fuzz tests rather than failing

Comment 17

[Karl Tomlinson (:karlt)]

Comment on attachment 692784 [details] [diff] [review]
fuzz tests rather than failing

I would have guessed a difference in the order of 2^3 from 5-bit truncation errors, but yes, better to mark this as fuzzy than fail, and probably not worth more investigation.

Even better if you want to remove the reference to bug 602892, which is no longer relevant since http://hg.mozilla.org/mozilla-central/rev/b6286cc2817a

Comment 18

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8ebb9359cff

Comment 19

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/mozilla-central/rev/d8ebb9359cff

Comment 20

[Nick Cameron [:nrc]]

Looks like the amount of fuzz required is a bit random and sometimes more than I put in the patch. I have no idea why that might be. (See TBPL bot posts in bug 780868).

Comment 21

[Nick Cameron [:nrc]]

Attachment: Moar fuzz

More fuzz, because the other patch didn't have enough, sometimes.

Comment 22

[Karl Tomlinson (:karlt)]

Comment on attachment 695799 [details] [diff] [review]
Moar fuzz

I assume this won't help because bug 780868 comment 21, for example, has a "max difference: 166".

166 is too large to explain through rounding errors, and the edges actually look chunky.  See scroll-1 in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=18a3309af87e for example.

Comment 23

[Nick Cameron [:nrc]]

(In reply to Karl Tomlinson (:karlt) from comment #22)
> Comment on attachment 695799 [details] [diff] [review]
> Moar fuzz
> 
> I assume this won't help because bug 780868 comment 21, for example, has a
> "max difference: 166".
> 
> 166 is too large to explain through rounding errors, and the edges actually
> look chunky.  See scroll-1 in
> https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=18a3309af87e for example.

Oh yeah, I read "max difference" as the allowed amount of fuzz.

I think the scroll test is a special case - the chunkiness looks to me like artifacts from the scrolling, but I have no idea why they appear here. The other three tests are just a bunch of pixels out in the AA zone. I can only imagine that the mask and the regular clip are formed using different tessellation algorithms for some reason.

I would like to fuzz those three tests and fail the scrolling one, perhaps we should look into the scrolling one in more depth. I don't have any ideas off the top of my head for it though.

Comment 24

[Nick Cameron [:nrc]]

Attachment: even more fuzz

Comment 25

[Nick Cameron [:nrc]]

Attachment: remote/non-remote fuzz

Comment 26

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/044831770290

Comment 27

[Nick Cameron [:nrc]]

leave-open, because we should investigate why we need the large amount of fuzz.

Comment 28

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/044831770290

Comment 29

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/message/reply/question/etc.)]

In bug #1072417 we removed the browser.tabs.remote,
so can this bug be closed or we should leave it open as it will be more investigation done here?

Comment 30

[Nick Cameron [:nrc]]

Close it. No plans here. (Closing as invalid since |browser.tabs.remote| no longer exists)

Comment 31

[Joel Maher ( :jmaher ) (UTC -8)]

there are 4 instances in here:
http://dxr.mozilla.org/mozilla-central/source/layout/reftests/border-radius/reftest.list#46

please remove these above items from the manifest before closing this bug.

Comment 32

[Geoff Brown [:gbrown]]

Test manifests updated in bug 1257948.