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

Delay reftest screenshot while WR frame is rendering #25822

Merged
merged 1 commit into from Feb 21, 2020

Conversation

@jdm
Copy link
Member

jdm commented Feb 21, 2020

This PR addresses the theory that #24726 occurs when WR is performing an async frame render and the reftest screenshot decides it's time to synchronously read the framebuffer. If there have not been any completed frames rendered yet, that would yield the page background colour.

The changes in this PR introduce an additional layer of synchronization - the compositor stores an AtomicBool value that indicates whether we know that a WR frame has started rendering, which is set to true when an IPC request from layout that submits a new display list is received. This bool is set to false when WR notifies us that a frame has been rendered. The screenshot code refuses to take a screenshot if the bool is true, causing us to delay taking a screenshot until there is no frame pending.

@highfive
Copy link

highfive commented Feb 21, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/constellation.rs
  • @cbrewster: components/constellation/constellation.rs
  • @KiChjang: components/script_traits/script_msg.rs
@jdm
Copy link
Member Author

jdm commented Feb 21, 2020

@bors-servo try=wpt

bors-servo added a commit that referenced this pull request Feb 21, 2020
Delay reftest screenshot while WR frame is rendering
@bors-servo
Copy link
Contributor

bors-servo commented Feb 21, 2020

Trying commit 15fe0fe with merge def4a51...

@jdm
Copy link
Member Author

jdm commented Feb 21, 2020

I'm going to retry a bunch of WPT tests and see if this affects #24726 at all.

@bors-servo
Copy link
Contributor

bors-servo commented Feb 21, 2020

💔 Test failed - status-taskcluster

@jdm

This comment was marked as outdated.

@jdm jdm force-pushed the jdm:delay-reftest-async-render branch from 15fe0fe to 35ba7c9 Feb 21, 2020
@jdm
Copy link
Member Author

jdm commented Feb 21, 2020

@bors-servo try=wpt

bors-servo added a commit that referenced this pull request Feb 21, 2020
Delay reftest screenshot while WR frame is rendering
@bors-servo
Copy link
Contributor

bors-servo commented Feb 21, 2020

Trying commit 35ba7c9 with merge 814e6f6...

@bors-servo
Copy link
Contributor

bors-servo commented Feb 21, 2020

💔 Test failed - status-taskcluster

@jdm jdm force-pushed the jdm:delay-reftest-async-render branch from 35ba7c9 to 5869abd Feb 21, 2020
@jdm
Copy link
Member Author

jdm commented Feb 21, 2020

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Feb 21, 2020

Trying commit 5869abd with merge 5084d07...

bors-servo added a commit that referenced this pull request Feb 21, 2020
Delay reftest screenshot while WR frame is rendering
@jdm jdm marked this pull request as ready for review Feb 21, 2020
@bors-servo
Copy link
Contributor

bors-servo commented Feb 21, 2020

💔 Test failed - status-taskcluster

@jdm
Copy link
Member Author

jdm commented Feb 21, 2020

https://community-tc.services.mozilla.com/tasks/groups/RbZ--54CQYGFmjcK--pc6w is a try run that runs the linux WPT tasks 30 times each and did not report a single instance of #24627.

@jdm jdm force-pushed the jdm:delay-reftest-async-render branch from 5869abd to c215746 Feb 21, 2020
@jdm
Copy link
Member Author

jdm commented Feb 21, 2020

@bors-servo r=emilio on Matrix

@bors-servo
Copy link
Contributor

bors-servo commented Feb 21, 2020

📌 Commit c215746 has been approved by emilio

@highfive highfive assigned emilio and unassigned nox Feb 21, 2020
@jdm
Copy link
Member Author

jdm commented Feb 21, 2020

@bors-servo
Copy link
Contributor

bors-servo commented Feb 21, 2020

Testing commit c215746 with merge be4ecb9...

@bors-servo
Copy link
Contributor

bors-servo commented Feb 21, 2020

☀️ Test successful - status-taskcluster
Approved by: emilio
Pushing be4ecb9 to master...

@bors-servo bors-servo merged commit be4ecb9 into servo:master Feb 21, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Feb 21, 2020
3 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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