Closed Bug 593275 Opened 9 years ago Closed 9 years ago

Reftests aren't testing accelerated layer managers properly since bug 130078

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: roc, Assigned: roc)

References

Details

Attachments

(3 files, 2 obsolete files)

Reftests are still calling drawWindow on a child window. Post bug-130078, that window has no widget, so this code quietly turns off USE_WIDGET_LAYERS:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#5284

We need to fix this. Now, before we turn on any accelerated layer managers by default.
blocking2.0: --- → beta6+
Attached patch WIP (obsolete) — Splinter Review
This kinda works but mysteriously, reftests hang when we load a XUL document.
That's expected behaviour because we don't load remote xul anymore. Run make reftest and the content pref in question will be set for you.
Ah, thanks for the tip!
Attached patch fix (obsolete) — Splinter Review
This actually seems to work! I'm sending it to the tryserver to see if it shows any new test failures.
Assignee: nobody → roc
Attachment #471764 - Attachment is obsolete: true
Attachment #471829 - Flags: review?(dbaron)
This patch makes us always take snapshots of the toplevel window ('window' for reftest.js). That way, we get to use the retained layers.

Because the toplevel window is not affected by a zoomed subdocument, this actually simplifies the code a lot. No matter what the zoom factor of the subdocument, the area we need to paint is exactly the bounds of the destination canvas. For incremental updates, we do need to map the coordinates of the MozAfterPaint event up to coordinates in the toplevel window. We could set the MozAfterPaint listener on the root window to get around that, I suppose.
(In reply to comment #5)
> Because the toplevel window is not affected by a zoomed subdocument, this
> actually simplifies the code a lot. No matter what the zoom factor of the
> subdocument, the area we need to paint is exactly the bounds of the destination
> canvas. For incremental updates, we do need to map the coordinates of the
> MozAfterPaint event up to coordinates in the toplevel window. We could set the
> MozAfterPaint listener on the root window to get around that, I suppose.

I'm puzzled why UpdateCurrentCanvasForEvent doesn't need to adjust the coordinates by the scrollX/scrollY as well.  (Or maybe you didn't hit any scrolled tests yet.)

That said, I wouldn't be puzzled if you had the MozAfterPaint listener on the toplevel window, which does seem like it might simplify things even more.


Also, could you move that rgb(255,255,255) comment to where we currently pass rgb(255,255,255) instead of removing it?  Or is it no longer correct?
Or are the MozAfterPaint events in the scrolled window already relative to its scrolled position?
(In reply to comment #6)
> I'm puzzled why UpdateCurrentCanvasForEvent doesn't need to adjust the
> coordinates by the scrollX/scrollY as well.  (Or maybe you didn't hit any
> scrolled tests yet.)

It probably does, but it didn't before.

> That said, I wouldn't be puzzled if you had the MozAfterPaint listener on the
> toplevel window, which does seem like it might simplify things even more.

I'll try that.

> Also, could you move that rgb(255,255,255) comment to where we currently pass
> rgb(255,255,255) instead of removing it?  Or is it no longer correct?

It might be correct, but it probably isn't and it doesn't seem worth testing.
Attached patch v2Splinter Review
Does MozAfterPaint listening on the toplevel. Still seems to work.
Attachment #471829 - Attachment is obsolete: true
Attachment #471843 - Flags: review?(dbaron)
Attachment #471829 - Flags: review?(dbaron)
This does indeed cause us to use the hardware-accelerated layer manager, at least on Mac.
Attachment #471843 - Flags: review?(dbaron) → review+
Keywords: checkin-needed
Whiteboard: [needs landing]
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Backed out for bustage on Linux, at least.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I managed to reproduce the problem on my machine. (At least I think it's the same problem.) When I run reftests on a small-size screen --- with compiz disabled --- the tests start running at the desired window size (800x1000), BUT when we get to test-async.xul the window gets resized from outside to fit on screen! For the first few tests the window hasn't actually appeared so I think we're able to get going before the window manager is showing the window, and I guess the window manager shrinks our window to fit on screen before allowing it to display. Anyway, with the current code we don't notice that the window size has changed so we proceed with USE_WIDGET_LAYERS even though that won't work anymore.

This patch applies on top of the previous patch and simply recomputes the flags on every single drawWindow. It fixes the bug for me; we dynamically switch off USE_WIDGET_LAYERS at the right time.

This a bit scary, though, because there's no guarantee that the window size will remain unchanged between drawing a test and drawing its reference. If the window size does change between test and reference, we might render the test and reference with different settings for USE_WIDGET_LAYERS, so the test might fail spuriously. We could perhaps fix that by making the current test expected-random whenever we detect a flags change. However I haven't seen this happen in practice so I have no way to test that approach.

Interesting note: with compiz enabled, the window manager makes no attempt to make my windows fit on screen. Maybe we should enable compiz on all our test boxes, at least the GL ones!
Attachment #472138 - Flags: review?(dbaron)
Attached patch Part 2 v2Splinter Review
Marks the test expected-random if drawWindow flags change during the test.
Attachment #472138 - Attachment is obsolete: true
Attachment #472140 - Flags: review?(dbaron)
Attachment #472138 - Flags: review?(dbaron)
Linux reftests pass on try with this patch.
However, all Linux tryserver machines aren't quite able to get the reftest window big enough. They all reduce the window size to height 974. Do we have a bug on file about that?
Comment on attachment 472140 [details] [diff] [review]
Part 2 v2

Why does test_async.xul cause the window to be resized?  Can we fix that?

And shouldn't we make the test fail if it's doing something that causes it never to be tested, rather than just hiding the failure by marking it expected-random?
(In reply to comment #17)
> Why does test_async.xul cause the window to be resized?  Can we fix that?

I don't think test-async.xul is specifically causing it. That test doesn't really do anything other than test reftest-wait. It's probably just the first reftest-wait test that triggers it, or maybe the first XUL test, or maybe even just the test that runs a certain amount of time into the run.

> And shouldn't we make the test fail if it's doing something that causes it
> never to be tested, rather than just hiding the failure by marking it
> expected-random?

Not if it's not the test's fault, I think.
BTW, my tryserver push had these parts 1 and 2 on top of the patch to enable D3D layers, and there are three test failures that look real:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1283611007.1283612477.19724.gz#err0

I *presume* that with D3D layers disabled on trunk, we should be still OK to land parts 1 and 2.
(In reply to comment #19)
> BTW, my tryserver push had these parts 1 and 2 on top of the patch to enable
> D3D layers, and there are three test failures that look real:
> 
> http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1283611007.1283612477.19724.gz#err0

Filed bug 593618 on the first two (I'm optimistically assuming same root problem); third failure is visually identical (one tone delta), added it to the list in 591155 for now.
Comment on attachment 472140 [details] [diff] [review]
Part 2 v2

I don't think it's acceptable for tests to be randomly disabled.  I think if we run a set of reftests they should all get run, and any failure not marked as known in the manifest should be reported.
Attachment #472140 - Flags: review?(dbaron) → review-
An alternative approach that would cause that would be that if you're unable to run both test and reference with DRAW_USE_WIDGET_LAYERS, and you have a mismatched pair, you rerun without the flag whichever was previously run with it.

That said, it would be good to get an approach for testing this that doesn't depend on window sizes.  Can we force rendering to a layer that's larger than the widget?
Maybe. We could probably even use cjones' SetDisplayPort API to do it on the layout side. We would probably also have to do something on the layer manager side (in particular for GL/D3D) to ensure that the backbuffer is the right size. I'm not sure how hard that would be.

(In reply to comment #22)
> An alternative approach that would cause that would be that if you're unable to
> run both test and reference with DRAW_USE_WIDGET_LAYERS, and you have a
> mismatched pair, you rerun without the flag whichever was previously run with
> it.

We could do that. I'm not sure how much to care about this since we can (and should) also change the test machine configurations to be have a slightly taller screen size. So in the end the only usage of either of these two approaches is for developers with unrealistically small screens, and running reftests accelerated on mobile devices.

Another immediate option is to just take the first version of the patch, in comment #13. In my testing, and on try, tests pass with it. If something changes and the size change happens at a bad time, that will show up as a test failure. Is that OK with you?
(In reply to comment #23)
> Another immediate option is to just take the first version of the patch, in
> comment #13. In my testing, and on try, tests pass with it. If something
> changes and the size change happens at a bad time, that will show up as a test
> failure. Is that OK with you?

That's ok with me, although I think we should try to find a solution that doesn't depend on screen size.
Comment on attachment 472138 [details] [diff] [review]
Part 2: update drawWindow flags dynamically

Given Rob and David's latest comments on this bug, let's see what David has to say about this patch.
Attachment #472138 - Attachment is obsolete: false
Attachment #472138 - Flags: review?(dbaron)
Comment on attachment 472138 [details] [diff] [review]
Part 2: update drawWindow flags dynamically

I'm fine with the reftest.js part, but don't go regenerating yacc-generated parsers as part of the same patch.
Attachment #472138 - Flags: review?(dbaron) → review+
Re-landed along with part 2 v1:
http://hg.mozilla.org/mozilla-central/rev/f64155b51b37
http://hg.mozilla.org/mozilla-central/rev/c81b6eb6605a
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
(In reply to comment #16)
> However, all Linux tryserver machines aren't quite able to get the reftest
> window big enough. They all reduce the window size to height 974. Do we have a
> bug on file about that?

No, I don't think so. I don't even think we manage the screen resolution with puppet on the Fedora slaves. Please file a bug when you know that what we need is a bigger screen size or to enable compiz enabled.
You need to log in before you can comment on or make changes to this bug.