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

Trust image requests in their dirtyness and verify with an extra assertion #2829

Merged
merged 3 commits into from Jun 21, 2018

Conversation

@kvark
Copy link
Member

commented Jun 19, 2018

This is a follow-up to #2796 and #2774 that should fix https://bugzilla.mozilla.org/show_bug.cgi?id=1468713
The use case (supposedly) happening here is:

  • during an image request, we figured that the texture has been evicted, so we ignore the dirty rectangle and don't use any intersection result with the viewport
  • during the uploading of textures, we try to intersect the original dirty rectangle with the viewport and realize that it's not actually visible. Problem here is not taking into account the forced upload switch, which is now fixed.

Instead of ignoring a request, we are now asserting for the dirty rect visibility result to match our expectation made during request_image.

r? @staktrace
Note: the second commit is cosmetic

Edit: the third commit fixes the coordinate space of the dirty rectangle. When it reaches our texture cache, there is no longer any knowledge of the full image size if it's tiled, so we correctly transform it into the local space. This fixes the slice addressing later on when the image needs to be uploaded.


This change is Reviewable

@kvark kvark changed the title Track upload flag in image requests [WIP] Track upload flag in image requests Jun 19, 2018
@kvark kvark force-pushed the kvark:dirty branch from 530000f to 36a941d Jun 19, 2018
@kvark kvark changed the title [WIP] Track upload flag in image requests Trust image requests in their dirtyness and verify with an extra assertion Jun 19, 2018
@kvark

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2018

@staktrace

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2018

@kvark this looks good and fixes the crash. There is another crash on the same page but I'll chase that down separately. r+

@kvark

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2018

@staktrace thanks for verifying! I do wonder about that other crash though, if it's related or not...
@bors-servo r=staktrace

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2018

📌 Commit 36a941d has been approved by staktrace

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2018

⌛️ Testing commit 36a941d with merge 68a2481...

bors-servo added a commit that referenced this pull request Jun 20, 2018
Trust image requests in their dirtyness and verify with an extra assertion

This is a follow-up to #2796 and #2774 that should fix https://bugzilla.mozilla.org/show_bug.cgi?id=1468713
The use case (supposedly) happening here is:
  - during an image request, we figured that the texture has been evicted, so we ignore the dirty rectangle and don't use any intersection result with the viewport
  - during the uploading of textures, we try to intersect the original dirty rectangle with the viewport and realize that it's not actually visible. Problem here is not taking into account the forced upload switch, which is now fixed.

Instead of ignoring a request, we are now asserting for the dirty rect visibility result to match our expectation made during `request_image`.

r? @staktrace
Note: the second commit is cosmetic

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

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2018

💔 Test failed - status-taskcluster

@kvark kvark force-pushed the kvark:dirty branch from 36a941d to 89b4b12 Jun 20, 2018
@kvark kvark changed the title Trust image requests in their dirtyness and verify with an extra assertion [WIP] Trust image requests in their dirtyness and verify with an extra assertion Jun 20, 2018
@kvark kvark force-pushed the kvark:dirty branch from 89b4b12 to 377c8d3 Jun 20, 2018
@kvark kvark force-pushed the kvark:dirty branch from 377c8d3 to acb7a43 Jun 21, 2018
@kvark kvark changed the title [WIP] Trust image requests in their dirtyness and verify with an extra assertion Trust image requests in their dirtyness and verify with an extra assertion Jun 21, 2018
@kvark

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2018

@kvark

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2018

(as per IRC, last commit is reviewed by Glenn)
@bors-servo r=staktrace,gw3583

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2018

📌 Commit acb7a43 has been approved by staktrace,gw3583

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2018

⌛️ Testing commit acb7a43 with merge cf98ad4...

bors-servo added a commit that referenced this pull request Jun 21, 2018
Trust image requests in their dirtyness and verify with an extra assertion

This is a follow-up to #2796 and #2774 that should fix https://bugzilla.mozilla.org/show_bug.cgi?id=1468713
The use case (supposedly) happening here is:
  - during an image request, we figured that the texture has been evicted, so we ignore the dirty rectangle and don't use any intersection result with the viewport
  - during the uploading of textures, we try to intersect the original dirty rectangle with the viewport and realize that it's not actually visible. Problem here is not taking into account the forced upload switch, which is now fixed.

Instead of ignoring a request, we are now asserting for the dirty rect visibility result to match our expectation made during `request_image`.

r? @staktrace
Note: the second commit is cosmetic

Edit: the third commit fixes the coordinate space of the dirty rectangle. When it reaches our texture cache, there is no longer any knowledge of the full image size if it's tiled, so we correctly transform it into the local space. This fixes the slice addressing later on when the image needs to be uploaded.

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

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: staktrace,gw3583
Pushing cf98ad4 to master...

@bors-servo bors-servo merged commit acb7a43 into servo:master Jun 21, 2018
3 checks passed
3 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@kvark kvark deleted the kvark:dirty branch Jun 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.