Closed Bug 672704 Opened 10 years ago Closed 9 years ago
Can't ignore resolution when running reftests with hardware-accelerated layers
In bug 648104, Brad added the ability to ignore the window resolution when running reftests so we pass DRAWWINDOW_USE_WIDGET_LAYERS unconditionally. Unfortunately, when we move to hardware-accelerated GLES layers, we absolutely can't ignore the window resolution. When we use hardware-accelerated layers, the reftest framework actually results in us reading the contents of the video card back into main memory, then comparing that to our reference. Thus, if we try to use a smaller window size, our reftests actually won't work, because the full content of the test won't be on the video card. We have to find some way around this. We might need to lower our reftest requirements by changing our reftests. Alternately, we could use different hardware with a larger physical or virtual screen, so we can fit our window's full 1000 vertical pixels. (This might actually already be possible; Clint tells me that the Tegra 250 machines we're currently using are at a resolution something like 1300x1050.) The Motorola Xoom, or something like it, might be another option, as long as it's used in the vertical orientation. Benoit/Ali, you're going to need to take a look into this before too long, I think.
You're talking about reftests with cross-process layers on tinderbox, right? I think the motivation for this switch may be slightly unclear: we would *never ever ever* use it on tinderbox, exactly for the reason you describe (we wouldn't be comparing all the pixels we're supposed to, leading to false negatives). This option exists solely so that devs, locally, can run the majority of the reftests on screens with insufficient resolution and be reasonably confident about patches before pushing to tryserver. Unless there's something I'm missing here, this looks to be a WORKSFORME. Lowering the resolution requirement for reftests is an unknown/scary problem but I guess worth filing if you want.
Food for though: What if we modified the reftest to only compare the region that is shown on screen for mobile? This would lead to false positive in some case where the only differences end up off the screen but how frequent would that be? Perhaps we wouldn't miss out on significant test coverage with this approach and could refactor any test case that do require this. Is it possible to run reftest while setting a 'zoom out' option? If that's possible this solution would be more desirable because we can run reftest on all hardwares instead of moving to a world where were only testing on a specific subset of devices like Xoom's. The downside is where not comparing pixel 1-to-1 so the down scaling could give us false positive in rare instances. Similarly, I'm not sure if this is possible either but, for large test we could run it 4 times with offsets (0, 0), (0, w), (h, 0), (h, w). This would increase the time taken for these test but again remove screen resolution constraints.
(In reply to comment #1) > You're talking about reftests with cross-process layers on tinderbox, right? > > I think the motivation for this switch may be slightly unclear: we would > *never ever ever* use it on tinderbox, exactly for the reason you describe > (we wouldn't be comparing all the pixels we're supposed to, leading to false > negatives). > Yeah, this isn't something we'd turn on by default on tinderbox. Sorry if I gave that impression. The changes Blassey made are for the devs so they can run _something_ on their devices. However, changing the resolution on tegras requires a reboot, so this is an expensive operation to do on the tegras (a reboot takes about 2 minutes). If we can get away from this resolution requirement, that would make us all quite happy, but we're not planning to run crippled reftests on the tinderbox in the meantime. We're looking into tablets for our next set of android automation because of this issue and also because the next version of the tegra dev board is very pricey (buying consumer tablets would actually be cheaper).
For developers I think it's totally OK to run reftests just testing whatever fits on the screen. For tryserver/tinderbox we need to test 800x1000, but it sounds like we can keep doing that, so it's all good. Zoom out is not an option since one of the things we test with reftests is zooming :-).
I saw this on the list at https://wiki.mozilla.org/Platform/GFX/GLLayersOnMobile, but it's not clear to me what needs to be done here. The reftests run on tinderbox don't ignore resolution. Devs, locally, can force resolution to be ignored if they want. What do we still need?
We discussed this at today's graphics meeting. We need to at least have a list of reftests known to require "large"-sized windows, so we know what tests _should_ fail (or otherwise need special handling) when overriding resolution requirements.
I don't think any tests are going to fail if you run them at too low resolution (although I could be wrong). There is only a risk of having false negatives --- tests that start failing but the harness doesn't notice because the failure is offscreen.
There's a suggestion if we see failure caused by mobile constraints (screensize, RAM, VRAM): https://bugzilla.mozilla.org/show_bug.cgi?id=668594#c12
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7) > I don't think any tests are going to fail if you run them at too low > resolution (although I could be wrong). There is only a risk of having false > negatives --- tests that start failing but the harness doesn't notice > because the failure is offscreen. It turns out that low resolution can indeed cause failures: for tests where the expected result is inequality, the test will fail if the all the differences are offscreen.
!= tests are quite rare, so you could just add an option somewhere to let all != tests be treated as random.
We're running tests at above the required resolution 800x1000. Is this bug still relevant?
I don't think so.
Re-open and give a short description of what still needs to be done if you disable with me closing this.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.