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

Fix flickering on Android #21842

Merged
merged 1 commit into from Oct 3, 2018
Merged

Fix flickering on Android #21842

merged 1 commit into from Oct 3, 2018

Conversation

@jdm
Copy link
Member

jdm commented Sep 29, 2018

This is particularly noticeable in debug builds on WebGL pages, but also appears during startup when loading normal pages. requestRender() causes Android to swap buffers under the assumption that the buffer contains a fully rendered frame, but when calling it from doFrame there are no guarantees that Servo has finished compositing yet. This causes stale buffers to be composited instead, leading to flickering at startup when there is no content, or general jerkiness on pages using animation callbacks as previous frames replace current frames.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #21796
  • These changes do not require tests because no android integration tests.

This change is Reviewable

@jdm
Copy link
Member Author

jdm commented Sep 29, 2018

@highfive highfive assigned paulrouget and unassigned emilio Sep 29, 2018
@paulrouget
Copy link
Contributor

paulrouget commented Oct 1, 2018

I'm surprised that works.

I agree that the requestRender should not be here and only be called from the compositor.

But, if servo is in "animating" mode, and your PR works, that means someone is calling performUpdate when it should not.

When Servo is animating, performUpdate should be called at 60 FPS by the embedder, which will eventually call requestRender if there is anything to draw.

So, I think the proper fix is to replace requestRender() with a mServo.performUpdate() call in doFrame (you'll need to add a performUpdate function to Servo.java).

But I'd like to know why this is working without performUpdate. Maybe my understanding of what should happen on animation is wrong.

@jdm
Copy link
Member Author

jdm commented Oct 1, 2018

I should probably verify that animating pages that don't involve webgl still work.

@paulrouget
Copy link
Contributor

paulrouget commented Oct 1, 2018

@jdm I will r+ this if you replace requestRender in doFrame with mServo.performUpdate().

I will then start investigating why set_animation_state doesn't do what I think it's supposed to do. Looks like an issue on Desktop as well.

@paulrouget
Copy link
Contributor

paulrouget commented Oct 2, 2018

You also need to add performUpdates to Servo.java.

@bors-servo
Copy link
Contributor

bors-servo commented Oct 2, 2018

The latest upstream changes (presumably #21830) made this pull request unmergeable. Please resolve the merge conflicts.

@paulrouget paulrouget force-pushed the jdm:android-extra-frame branch from c0ce120 to 7c9476b Oct 3, 2018
@paulrouget paulrouget force-pushed the jdm:android-extra-frame branch from 7c9476b to de8fae1 Oct 3, 2018
@paulrouget
Copy link
Contributor

paulrouget commented Oct 3, 2018

@bors-servo
Copy link
Contributor

bors-servo commented Oct 3, 2018

📌 Commit de8fae1 has been approved by paulrouget

@bors-servo
Copy link
Contributor

bors-servo commented Oct 3, 2018

Testing commit de8fae1 with merge e780552...

bors-servo added a commit that referenced this pull request Oct 3, 2018
Fix flickering on Android

This is particularly noticeable in debug builds on WebGL pages, but also appears during startup when loading normal pages. requestRender() causes Android to swap buffers under the assumption that the buffer contains a fully rendered frame, but when calling it from `doFrame` there are no guarantees that Servo has finished compositing yet. This causes stale buffers to be composited instead, leading to flickering at startup when there is no content, or general jerkiness on pages using animation callbacks as previous frames replace current frames.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21796
- [x] These changes do not require tests because no android integration tests.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21842)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 3, 2018

💔 Test failed - linux-rel-wpt

@CYBAI
Copy link
Collaborator

CYBAI commented Oct 3, 2018

@bors-servo retry

{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/css/css-transforms/matrix/svg-matrix-043.html", "line": 30217, "action": "test_result", "expected": "PASS"}
@bors-servo
Copy link
Contributor

bors-servo commented Oct 3, 2018

@bors-servo
Copy link
Contributor

bors-servo commented Oct 3, 2018

💔 Test failed - linux-rel-wpt

@CYBAI
Copy link
Collaborator

CYBAI commented Oct 3, 2018

@bors-servo retry

  • oom
@bors-servo
Copy link
Contributor

bors-servo commented Oct 3, 2018

@bors-servo
Copy link
Contributor

bors-servo commented Oct 3, 2018

💔 Test failed - linux-rel-wpt

@CYBAI
Copy link
Collaborator

CYBAI commented Oct 3, 2018

@bors-servo retry

  • oom
@bors-servo
Copy link
Contributor

bors-servo commented Oct 3, 2018

Testing commit de8fae1 with merge 7fa2b2c...

bors-servo added a commit that referenced this pull request Oct 3, 2018
Fix flickering on Android

This is particularly noticeable in debug builds on WebGL pages, but also appears during startup when loading normal pages. requestRender() causes Android to swap buffers under the assumption that the buffer contains a fully rendered frame, but when calling it from `doFrame` there are no guarantees that Servo has finished compositing yet. This causes stale buffers to be composited instead, leading to flickering at startup when there is no content, or general jerkiness on pages using animation callbacks as previous frames replace current frames.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21796
- [x] These changes do not require tests because no android integration tests.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21842)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 3, 2018

@bors-servo bors-servo merged commit de8fae1 into servo:master Oct 3, 2018
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
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.

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