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

Skip clip masks with tiled blobs, instead of crashing. #3219

Merged
merged 2 commits into from Oct 19, 2018

Conversation

Projects
None yet
4 participants
@gw3583
Collaborator

gw3583 commented Oct 19, 2018

We don't currently support tiled blobs for clip masks (see
issue #2852). This patch detects that case, and skips applying
a clip mask in that case. It's not ideal, but it's better
than crashing inside the resource cache, which is the current
behavior.


This change is Reviewable

Skip clip masks with tiled blobs, instead of crashing.
We don't currently support tiled blobs for clip masks (see
issue #2852). This patch detects that case, and skips applying
a clip mask in that case. It's not ideal, but it's better
than crashing inside the resource cache, which is the current
behavior.
@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 19, 2018

r? @nical or @jrmuizel

I think this workaround is OK - the only issue would be if there are any situations in the resource cache where we currently do handle limited cases of clip mask images with tiling that this would regress. Otherwise, it should at least stop the crash for now, until we implement the proper solution in #2852.

Pending try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdf14a03c74120efb6f08dfcd7046aab63bb552d

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 19, 2018

cc @Gankro @mattwoodrow We'll need to fix #2852 before we consider enabling tiling for all blobs 😞

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 19, 2018

The try run looks good.

@nical

nical approved these changes Oct 19, 2018

r=me with the comment about the boolean addressed.

@@ -760,7 +773,7 @@ impl ClipItemKey {
pub enum ClipItem {
Rectangle(LayoutRect, ClipMode),
RoundedRectangle(LayoutRect, BorderRadius, ClipMode),
Image(ImageMask),
Image(ImageMask, bool),

This comment has been minimized.

@nical

nical Oct 19, 2018

Collaborator

Please make the boolean a named field or add a comment, It takes a bit of looking around to find what it is for (batch.rs is the only place where the meaning is explicit, and it's not where I'd look first),

@emilio

This comment has been minimized.

Member

emilio commented Oct 19, 2018

I added the comment.

@bors-servo r=nical

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 19, 2018

📌 Commit 207cfec has been approved by nical

bors-servo added a commit that referenced this pull request Oct 19, 2018

Auto merge of #3219 - gw3583:clip-tile-workaround, r=nical
Skip clip masks with tiled blobs, instead of crashing.

We don't currently support tiled blobs for clip masks (see
issue #2852). This patch detects that case, and skips applying
a clip mask in that case. It's not ideal, but it's better
than crashing inside the resource cache, which is the current
behavior.

<!-- 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/3219)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 19, 2018

⌛️ Testing commit 207cfec with merge c72754d...

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 19, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: nical
Pushing c72754d to master...

@bors-servo bors-servo merged commit 207cfec into servo:master Oct 19, 2018

3 checks passed

Taskcluster (pull_request) TaskGroup: success
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