Closed Bug 1016222 Opened 10 years ago Closed 10 years ago

B2G ICS Emulator Opt reftest failures when enabling low-res and progressive painting with OOP reftests

Categories

(Core :: Panning and Zooming, defect)

Product:
Core
Component:
Panning and Zooming
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla32

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(2 files, 1 obsolete file)

Part 1 - Fuzz some tests
(deleted), patch
jwatt
: review+
Details | Diff | Splinter Review
Part 2 - Disable low-res painting on a specific test
(deleted), patch
cwiiis
: review+
Details | Diff | Splinter Review 
I had turned on low-res and progressive painting and landed it on inbound. Simultaneously :ahal made the emulator reftests run in OOP mode on b2g-inbound. When these patches combined it resulted in a slew of reftest failures. So low-res and progressive painting was disabled again. (See bug 1016081 for details).

This bug tracks fixing those reftest failures so that we can re-enable low-res and progressive painting.

Relevant patches:
	f05fbe6261ac	Vincent Chen — Bug 981477 - Disable B2G emulator reftests that fail oop, r=ahal
	6ae6efd7f5e6	Andrew Halberstadt — Bug 922680 - Enable out of process b2g emulator reftests, r=jgriffin
Per https://tbpl.mozilla.org/?rev=545c35907eff&jobname=b2g_emulator the specific reftests that are failing are:

R1: layout/reftests/reftest-sanity/async-scroll-1b.html
R4: layout/reftests/bugs/404553-1.html
R6: layout/reftests/css-gradients/radial-shape-closest-corner-1b.html
R10: layout/reftests/marquee/336736-1a.html
R10: layout/reftests/marquee/336736-1b.html
R13: layout/reftests/svg/svg-integration/mask-html-zoomed-01.xhtml
Had a quick look at these, and to me these all look like legitimate failures that fall into two categories;

1- Accuracy issues caused by progressive rendering (as highlighted by real-world content in bug 1014333)
2- Low precision buffer drawing when it shouldn't/high precision buffer not being updated when it should

The first is something I've been looking at, but I'm not convinced there's an easy fix - it requires some work in TiledContentClient.cpp, perhaps there's a way of not losing accuracy when drawing away from the dirty region origin.

The second is likely indicating something wrong in the progressive update region calculation (ComputeProgressiveUpdateRegion and the code in BeginPaint in ClientTiledThebesLayer).

This is all off the top of my head, so take it with a pinch of salt.
tn, do you know why async-scroll-1b.html is expected to != async-scroll-1-ref.html? It looks to me like they should both render the same thing. It was added in bug 975931, part 9. In fact async-scroll-1b.html is identical to async-scroll-1a.html except for the height of the displayport, which AFAICT shouldn't make a difference to the rendering in this case. 1a is marked as == in the reftest.list file but 1b is marked as != and I don't understand why.
Flags: needinfo?(tnikkel)
I started looking into the failure on layout/reftests/bugs/404553-1.html (see link comment 1 to get the reftest analyzer view). From the analyzer it looks like the rightmost part of the div is getting rendered in low-res, or something along those lines. I can reproduce this locally using an emulator build, and added logging.

In the relevant timeframe, I see 4 calls to FrameLayerBuilder::DrawThebesLayer, with these draw regions:
1. < (x=0, y=0, w=800, h=1000) >
2. < (x=783, y=0, w=17, h=19) >
3. < (x=800, y=0, w=34, h=19) >
4. < (x=0, y=0, w=800, h=1000) >

The middle two draw regions look suspicious - for one thing the page should be contained entirely within 800 horizontal pixels and draw region 3 extends beyond that.

I got the layer dump to see what the layer tree looks like. Here it is simplified:

ClientLayerManager
  ClientContainerLayer visible=< (x=0, y=0, w=800, h=1000) > displayport=(0, 0, 800, 1000)
    ClientTiledThebesLayer visible=< (x=0, y=0, w=800, h=1000) >
    ClientTiledThebesLayer visible=< (x=783, y=0, w=51, h=19) > transform=[ 1 0; 0 1; -42 8 ]

So what's happening is the tiling code is taking that second tiled layer (ignoring the transform on it) and drawing part of it high-res and part of it low-res. If it took into account the transform it would realize that the entire thing is inside the critical displayport and draw it all in high-res.
R6: layout/reftests/css-gradients/radial-shape-closest-corner-1b.html
R13: layout/reftests/svg/svg-integration/mask-html-zoomed-01.xhtml
- Visually these appear similar, maybe they just need to be fuzzed?

R10: layout/reftests/marquee/336736-1a.html
R10: layout/reftests/marquee/336736-1b.html
- Visually these appear to be the same problem as comment 4
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> tn, do you know why async-scroll-1b.html is expected to !=
> async-scroll-1-ref.html? It looks to me like they should both render the
> same thing. It was added in bug 975931, part 9. In fact async-scroll-1b.html
> is identical to async-scroll-1a.html except for the height of the
> displayport, which AFAICT shouldn't make a difference to the rendering in
> this case. 1a is marked as == in the reftest.list file but 1b is marked as
> != and I don't understand why.

Hmm, I remember it making sense when I reviewed it, but I don't know what I was thinking. I don't see any problem with your analysis.

The test is currently working on linux Ripc though, how does it pass there?
Flags: needinfo?(tnikkel)
(In reply to Timothy Nikkel (:tn) from comment #7)
> The test is currently working on linux Ripc though, how does it pass there?

Good question. It fails when I run it locally, so I have to debug on try. I pushed a patch that should dump the images for passing != comparisons: https://tbpl.mozilla.org/?tree=Try&rev=0121b01b8333
Split the mystery of async-scroll-1b into bug 1017065.
Latest try push with various fixes and fuzzes: https://tbpl.mozilla.org/?tree=Try&rev=83aaf51ba5bd
Oops, that was a waste. Had an extra space in the reftest.list file :( Here we go again: https://tbpl.mozilla.org/?tree=Try&rev=9e6a88ad5a77
^ that one had a compile error but I caught it early. Here's the real deal: https://tbpl.mozilla.org/?tree=Try&rev=d046a9fca42f, all green.
Attached patch Part 1 - Fuzz some tests (deleted) — — Splinter Review 
If it makes you feel better, the native-widget one was also recently fuzzed on other platforms (see rebased-to-latest-mc patch at https://hg.mozilla.org/try/rev/c3b20da940a3)
Attachment #8430785 - Flags: review?(chrislord.net)
This test explicitly relies on setting a displayport and having only that drawn. When low-res is turned on, the displayport becomes the critical displayport and we draw a larger low-res displayport which makes the test fail, so I think it makes sense to disable low-res just for this test.

Also note that the patch I'm uploading here assumes the patch on bug 1017065 is applied, but there's no actual dependency there. The try push I just did has this patch rebased to not assume the other patch, see https://hg.mozilla.org/try/rev/eef96ea170c4 for what it looks like.
Attachment #8430788 - Flags: review?(chrislord.net)
No longer depends on: 1017065
Blocks: 897996
No longer blocks: 993473
Sorry, apparently I need to make sure the pref exists everywhere before it can be disabled in a reftest.
Attachment #8430788 - Attachment is obsolete: true
Attachment #8430788 - Flags: review?(chrislord.net)
Attachment #8430813 - Flags: review?(chrislord.net)
Comment on attachment 8430785 [details] [diff] [review]
Part 1 - Fuzz some tests

Review of attachment 8430785 [details] [diff] [review]:
-----------------------------------------------------------------

I'm taking myself off this review, I think it would be better to get someone from layout to look at these. Ftr, I think the very last test fuzzing might be indicative of an actual bug (possibly bug 1014333) and I'd block on that before fuzzing by such a large amount.
Attachment #8430785 - Flags: review?(chrislord.net)
Comment on attachment 8430813 [details] [diff] [review]
Part 2 - Disable low-res painting on a specific test

Review of attachment 8430813 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, with your explanation.
Attachment #8430813 - Flags: review?(chrislord.net) → review+
Attachment #8430785 - Flags: review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: