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

Ensure calling VertexDataTexture::update always guarantees texture() to succeed. #3185

Merged
merged 1 commit into from Oct 11, 2018

Conversation

@emilio
Copy link
Member

emilio commented Oct 11, 2018

Alternative is stop assuming there's a texture, which we may or may not want to
do (happy to do that, but I know Bobby has in-flight patches for a bunch of the
texture stuff, so this probably is less breaking).

Fixes #3184


This change is Reviewable

@emilio
Copy link
Member Author

emilio commented Oct 11, 2018

@emilio emilio requested review from bholley and kvark and removed request for bholley Oct 11, 2018
Copy link
Member

kvark left a comment

Asking for some clarifications

@@ -1232,7 +1232,7 @@ impl Device {
0,
desc.external,
desc.pixel_type,
pixels.map(texels_to_u8_slice),
pixels.and_then(texels_to_u8_slice),

This comment has been minimized.

@kvark

kvark Oct 11, 2018

Member

I don't think this is correct. If the slice is provided, we exactly X bytes of data. Just ignoring empty data slices and treating them as storage allocation without initialization is something we shouldn't need to do.

This comment has been minimized.

@emilio

emilio Oct 11, 2018

Author Member

Well, pixels.as_ptr() will return something close to null, but yeah, I guess it's not really needed if the size is zero.

This comment has been minimized.

@emilio

emilio Oct 11, 2018

Author Member

Reverted these changes. This was just me being overly cautious with &[T].as_ptr() returning something non-null, which has bit me in the past.

@emilio emilio force-pushed the emilio:reftest-panic branch from d2295d2 to 6a1c444 Oct 11, 2018
@kvark
kvark approved these changes Oct 11, 2018
@kvark
Copy link
Member

kvark commented Oct 11, 2018

@bors-servo
Copy link
Contributor

bors-servo commented Oct 11, 2018

📌 Commit 6a1c444 has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Oct 11, 2018

Testing commit 6a1c444 with merge e7ddcbe...

bors-servo added a commit that referenced this pull request Oct 11, 2018
Ensure calling VertexDataTexture::update always guarantees texture() to succeed.

Alternative is stop assuming there's a texture, which we may or may not want to
do (happy to do that, but I know Bobby has in-flight patches for a bunch of the
texture stuff, so this probably is less breaking).

Fixes #3184

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

kvark commented Oct 11, 2018

@bors-servo r-
let's not create 0x0 textures :)

@emilio emilio force-pushed the emilio:reftest-panic branch 3 times, most recently from 7ed06a7 to 98ddee0 Oct 11, 2018
@emilio emilio force-pushed the emilio:reftest-panic branch from 98ddee0 to 9940366 Oct 11, 2018
@kvark
kvark approved these changes Oct 11, 2018
Copy link
Member

kvark left a comment

I don't get why we'd want mem::uninitialized instead of mem::zeroed here, but the code is similar to another chunk we have below, so this is not a concern for this PR.

@kvark
Copy link
Member

kvark commented Oct 11, 2018

Thanks!
@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Oct 11, 2018

📌 Commit 9940366 has been approved by kvark

bors-servo added a commit that referenced this pull request Oct 11, 2018
Ensure calling VertexDataTexture::update always guarantees texture() to succeed.

Alternative is stop assuming there's a texture, which we may or may not want to
do (happy to do that, but I know Bobby has in-flight patches for a bunch of the
texture stuff, so this probably is less breaking).

Fixes #3184

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

bors-servo commented Oct 11, 2018

Testing commit 9940366 with merge a20c3da...

@bors-servo
Copy link
Contributor

bors-servo commented Oct 11, 2018

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

@bors-servo bors-servo merged commit 9940366 into servo:master Oct 11, 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
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

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