Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Intermittent reftest failure in pixel_snapping_border_a.html #7730

Closed
jdm opened this issue Sep 24, 2015 · 13 comments
Closed

Intermittent reftest failure in pixel_snapping_border_a.html #7730

jdm opened this issue Sep 24, 2015 · 13 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Sep 24, 2015

---- pixel_snapping_border_a.html != pixel_snapping_border_ref.html stdout ----
    thread 'pixel_snapping_border_a.html != pixel_snapping_border_ref.html' panicked at 'assertion failed: `(left == right)` (left: `1600`, right: `1280`)', ../../tests/reftest.rs:323
@mbrubeck
Copy link
Contributor

@mbrubeck mbrubeck commented Sep 24, 2015

(left:1600, right:1280) means that one image was 1600 pixels wide, while the other was 1280 pixels wide. If both correctly used --device-pixel-ratio 2 as set in the manifest for this test, this means that one was rendered with a viewport of 800px and the other 640px.

Servo's default window size is 800px as set in util::opts::default_ops, but compositor::headless::NullCompositor::create resizes the window to 640px. Perhaps there's a race condition that causes the screenshot to sometimes be taken before this resize and sometimes after.

@mbrubeck mbrubeck self-assigned this Sep 30, 2015
mbrubeck added a commit to mbrubeck/servo that referenced this issue Sep 30, 2015
bors-servo pushed a commit that referenced this issue Sep 30, 2015
Use same default window size in headless and non-headless compositor

Attempts to fix #7730. r? @mrobinson

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7805)
<!-- Reviewable:end -->
mbrubeck added a commit to mbrubeck/servo that referenced this issue Sep 30, 2015
This may mitigate race conditions like servo#7730.
bors-servo pushed a commit that referenced this issue Oct 1, 2015
Start the constellation with the correct pixel ratio

This may mitigate race conditions like #7730. r? @mrobinson

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7808)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Oct 1, 2015
Start the constellation with the correct pixel ratio

This may mitigate race conditions like #7730. r? @mrobinson

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7808)
<!-- Reviewable:end -->
@jdm jdm reopened this Oct 4, 2015
@jdm
Copy link
Member Author

@jdm jdm commented Oct 4, 2015

Now we get:

---- pixel_snapping_border_a.html != pixel_snapping_border_ref.html stdout ----
    thread 'pixel_snapping_border_a.html != pixel_snapping_border_ref.html' panicked at 'assertion failed: `(left == right)` (left: `864`, right: `863`)', ../../tests/reftest.rs:344

So the heights differ by one pixel now??

@mbrubeck
Copy link
Contributor

@mbrubeck mbrubeck commented Oct 5, 2015

I have no idea where a height of 863 or 864 is coming from. The default window height is 600px and this test sets a 2× scale factor, which should result in a height of 1200 pixels.

Possibly relevant? 864 = 600 × 1.44 exactly.

@mbrubeck
Copy link
Contributor

@mbrubeck mbrubeck commented Oct 5, 2015

These failures are all in mac-dev-ref-unit, but they happen on both Mac test machines (servo-mac1 and servo-mac2).

@glennw
Copy link
Member

@glennw glennw commented Oct 7, 2015

Mac seems to use some kind of heuristic to determine where to place new windows. When running several tests in parallel, they can be opened in a way that they are partially off screen.

Previously this would result in a similar bug - the output image would be the clipped size. I thought this was fixed by using an FBO to render reftest output on mac. But perhaps this has changed, or perhaps it uses the window size to write the FBO image output, or something like that?

@mbrubeck
Copy link
Contributor

@mbrubeck mbrubeck commented Oct 7, 2015

Presumably this test triggers the problem just because its window is so big. Perhaps we can give it a custom window size that's smaller, as a workaround.

@glennw
Copy link
Member

@glennw glennw commented Oct 7, 2015

Yep - I suspect that's why this test is triggering the bug more often, and that would probably work around it. We should fix the underlying problem too though :)

@jdm
Copy link
Member Author

@jdm jdm commented Oct 7, 2015

initialize_png is given the window width and height and uses them for tex_image_2d.

@glennw
Copy link
Member

@glennw glennw commented Oct 7, 2015

I guess the question is what are the value of window width/height in these instances? I would have thought it would be the requested size rather than clipped to the screen.

@jdm
Copy link
Member Author

@jdm jdm commented Oct 7, 2015

From the builder in a failing instance:

w: 1280, h: 847
w: 1280, h: 850
@jdm
Copy link
Member Author

@jdm jdm commented Oct 7, 2015

In particular, we initialize our window_size member with window.framebuffer_size(), and our glutin port has this:

    fn framebuffer_size(&self) -> TypedSize2D<DevicePixel, u32> {
        let scale_factor = self.window.hidpi_factor() as u32;
        let (width, height) = self.window.get_inner_size().unwrap();
        Size2D::typed(width * scale_factor, height * scale_factor)
    }
@metajack
Copy link
Contributor

@metajack metajack commented Oct 7, 2015

What I can recall from setting up the mac builders is that if a window appears partially offscreen, the framebuffer created for the window is clipped to the actual visible area. To address this, we set the screen size large enough that ref tests wouldn't appear offscreen.

Lars has upgraded the mac builders a few times, so vncing into the mac builders and ensuring they have a large resolution is probably a worthwhile sanity check. I think we only do offscreen rendering on linux.

glennw pushed a commit to glennw/servo that referenced this issue Oct 7, 2015
glennw pushed a commit to glennw/servo that referenced this issue Oct 8, 2015
bors-servo pushed a commit that referenced this issue Oct 8, 2015
bors-servo
Don't check for equal image size and all white pixels for flaky tests.

Workaround for #7730.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7916)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Oct 8, 2015
Workaround for #7730.



<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7920)
<!-- Reviewable:end -->
jimberlage pushed a commit to jimberlage/servo that referenced this issue Oct 8, 2015
Fixes servo#7907: rendering for unescaped string "<iframe>" in doc

webgl: Implement WebGLContextEvent and use it on context creation error

spec: https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.15

Move code to exit servo after writing a screenshot out of composite_specific_target.

The structure of this function was confusing, so move some parts out into the
caller where they seem like a more natural fit and add documentation of the
functions

Remove constellation round trip for subpage mapping in compositor.

This makes use of the new functionality that allows iframes to generate their own pipeline IDs in order to remove any knowledge of subpage ids from the compositor.

(This is the first of several commits removing subpage from parts of servo).

Workaround for servo#7730.

Simplify and unify compositor shutdown code paths

Unify all compositor shutdown code paths into two methods, one which
starts the shutdown and the other that finishes it. This simplifies the
way the compositor shuts down and prevents "leaking" pixmaps when
exiting in uncommon ways.
@jdm
Copy link
Member Author

@jdm jdm commented Apr 22, 2016

Hasn't been seen in months, not disabled => close.

@jdm jdm closed this Apr 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.