Skip to content

/ webrender

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

Ensure non-zero picture task size #2490

Merged
merged 1 commit into from Mar 5, 2018
Merged

Ensure non-zero picture task size #2490

merged 1 commit into from Mar 5, 2018

Conversation

@kvark
Copy link
Member

kvark commented Mar 5, 2018 •
edited

Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1442718

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc0d35306d006a09ddb0cf7db453a97fa9bf8dca

Another try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11b5d65d0e682c277992c1cea5d4048227184118

r? @staktrace

This change is Reviewable
@staktrace
Copy link
Contributor

staktrace commented Mar 5, 2018

According to the comment the rounding was intentional to pass Gecko reftests. So presumably there will be reftest failures from this change? Let's wait and see what the try push results look like.
@kvark
Copy link
Member Author

kvark commented Mar 5, 2018

Yeah, might be safer to just round up to 1 only from 0, if the try push is not good.
@kvark kvark force-pushed the kvark-content-rect branch from 27c031a to 9a470c5 Mar 5, 2018
@kvark
Copy link
Member Author

kvark commented Mar 5, 2018

@staktrace PTAL, another try push in flight
@staktrace
staktrace approved these changes Mar 5, 2018
View changes
webrender/src/picture.rs
// technically correct. Ideally we should ceil() here, and ensure that
// the extra part pixel in the case of fractional sizes is correctly
// handled. For now, just use rounding which passes the existing
// Gecko tests.

This comment has been minimized.

Sign in to view
@staktrace

staktrace Mar 5, 2018

Contributor

The comment should be updated to indicate that width/height of 0 causes badness, hence the max'ing with 1. But other than that, LGTM
@kvark
Ensure non-zero picture task size
Loading status checks…
bd1b633
@kvark kvark force-pushed the kvark-content-rect branch from 9a470c5 to bd1b633 Mar 5, 2018
@kvark
Copy link
Member Author

kvark commented Mar 5, 2018

Fixed now, thanks for the review!
@bors-servo r=staktrace
@bors-servo
Copy link
Contributor

bors-servo commented Mar 5, 2018

📌 Commit bd1b633 has been approved by staktrace
@bors-servo
Copy link
Contributor

bors-servo commented Mar 5, 2018

⌛ Testing commit bd1b633 with merge 4cb9af1...
bors-servo added a commit that referenced this pull request Mar 5, 2018
@bors-servo
Auto merge of #2490 - servo:kvark-content-rect, r=staktrace
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
4cb9af1 
Ensure non-zero picture task size

Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1442718

~~Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc0d35306d006a09ddb0cf7db453a97fa9bf8dca~~

Another try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11b5d65d0e682c277992c1cea5d4048227184118

r? @staktrace

<!-- 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/2490)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 5, 2018

💔 Test failed - status-travis
@kvark
Copy link
Member Author

kvark commented Mar 5, 2018

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

here we go again...
@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Mar 5, 2018

⌛ Testing commit bd1b633 with merge 8b4f0cc...
bors-servo added a commit that referenced this pull request Mar 5, 2018
@bors-servo
Auto merge of #2490 - servo:kvark-content-rect, r=staktrace
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
8b4f0cc 
Ensure non-zero picture task size

Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1442718

~~Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc0d35306d006a09ddb0cf7db453a97fa9bf8dca~~

Another try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11b5d65d0e682c277992c1cea5d4048227184118

r? @staktrace

<!-- 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/2490)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 5, 2018

💔 Test failed - status-travis
@kvark
Copy link
Member Author

kvark commented Mar 5, 2018

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Mar 5, 2018

⌛ Testing commit bd1b633 with merge 3f1ff07...
bors-servo added a commit that referenced this pull request Mar 5, 2018
@bors-servo
Auto merge of #2490 - servo:kvark-content-rect, r=staktrace
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
3f1ff07 
Ensure non-zero picture task size

Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1442718

~~Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc0d35306d006a09ddb0cf7db453a97fa9bf8dca~~

Another try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11b5d65d0e682c277992c1cea5d4048227184118

r? @staktrace

<!-- 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/2490)
<!-- Reviewable:end -->
@glennw
Copy link
Member

glennw commented Mar 5, 2018

@kvark @staktrace I think it's fine to merge this as a bandaid fix, but I don't think it's the "correct" fix.

It seems like we should detect a zero-sized picture, and not be trying to add it to the texture cache at all?

That would probably be more efficient since we'd then skip building that picture, adding batches for it, allocating target space etc.
@kvark
Copy link
Member Author

kvark commented Mar 5, 2018 •
edited

@glennw I don't think we should ignore it here. It's not zero-sized, it's just less than 0.5 sized.
As your comment states, the proper use would be ceil(), and that's what we do here just for a very specific near-zero case instead of the general one.
@glennw
Copy link
Member

glennw commented Mar 5, 2018

@kvark Ah, I see - that makes sense. Thanks!
@bors-servo
Copy link
Contributor

bors-servo commented Mar 5, 2018

☀️ Test successful - status-appveyor, status-taskcluster, status-travis
Approved by: staktrace
Pushing 3f1ff07 to master...
@bors-servo bors-servo merged commit bd1b633 into master Mar 5, 2018
6 checks passed
6 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
Taskcluster (push) TaskGroup: success
Details
Tidelift Dependencies checked
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Mar 5, 2018
Merged
@kvark kvark deleted the kvark-content-rect branch Mar 5, 2018
@Darkspirit Darkspirit mentioned this pull request Mar 5, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@staktrace staktrace

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@kvark @staktrace @bors-servo @glennw
You can’t perform that action at this time.