Closed Bug 732535 Opened 9 years ago Closed 5 years ago

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

Categories

(Core :: Graphics: Layers, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED WONTFIX
mozilla20

People

(Reporter: jmaher, Assigned: gbrown)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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
Blocks: 732541
The failure is a single pixel that's rgb(1, 1, 1) when it should be rgb(0, 0, 0).
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.
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!)
Assignee: nobody → gbrown
Jet, this looks like something we'll need your team to look at.
Assignee: gbrown → jet
These tests are passing today (with remote=false)! Does anyone know why?
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
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
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Blocks: 760270
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.
Component: Layout → Graphics: Layers
Duplicate of this bug: 760270
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.
Assignee: bugs → ncameron
Oh, if anyone has a link to a failing Try run, that would be helpful, thanks!
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.
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.
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)".
(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.
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 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
Attachment #692784 - Flags: review?(karlt) → review+
https://hg.mozilla.org/mozilla-central/rev/d8ebb9359cff
Status: REOPENED → RESOLVED
Closed: 9 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 824745
Attached patch Moar fuzz (obsolete) — Splinter Review
More fuzz, because the other patch didn't have enough, sometimes.
Attachment #695799 - Flags: review?(karlt)
No longer blocks: 760270
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.
Attachment #695799 - Flags: review?(karlt) → review-
(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.
Attached patch even more fuzz (obsolete) — Splinter Review
Attachment #695799 - Attachment is obsolete: true
Attachment #695909 - Flags: review?(karlt)
Attachment #695909 - Attachment is obsolete: true
Attachment #695909 - Flags: review?(karlt)
Attachment #696183 - Flags: review?(karlt)
Attachment #696183 - Flags: review?(karlt) → review+
leave-open, because we should investigate why we need the large amount of fuzz.
Depends on: 891255
Assignee: ncameron → nobody
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?
Flags: needinfo?(ncameron)
Close it. No plans here. (Closing as invalid since |browser.tabs.remote| no longer exists)
Status: REOPENED → RESOLVED
Closed: 8 years ago6 years ago
Flags: needinfo?(ncameron)
Resolution: --- → INVALID
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.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Test manifests updated in bug 1257948.
Assignee: nobody → gbrown
Status: REOPENED → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → WONTFIX
Whiteboard: [leave-open]
You need to log in before you can comment on or make changes to this bug.