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.

Sign up for GitHub

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

Jump to bottom

Allow updating images that are not raw buffers #1004

Merged
merged 4 commits into from
Mar 29, 2017
Merged

Allow updating images that are not raw buffers #1004

merged 4 commits into from
Mar 29, 2017

Conversation

nical
Copy link
Contributor

@nical nical commented Mar 22, 2017 •
edited by larsbergstrom
Loading

update_image currently takes a vector of bytes and the implementation expects this vector to be regular image data. We need to be able to update blob images as well.

This change is Reviewable

@nical
Copy link
Contributor Author

nical commented Mar 22, 2017

This PR is written on top of PR #1003, so the latter will show up in the diff until it lands.
With update_image taking ImageData instead of a bag of bytes, I think that it makes sense to allow updating external images (by removing the assertion), or at least explain why we can't update external image handles (if we can't).

r? @glennw

@glennw
Copy link
Member

glennw commented Mar 27, 2017

@nical Could you rebase this on master to remove that extra commit?

@nical
Copy link
Contributor Author

nical commented Mar 27, 2017

The extra commit is the followup nit fix from PR #1003, it is very small and somewhat close to what's happening in this PR so I put it here. Do you want me to extract it in a separate pull request ?

How do you feel about removing the assertion in resource_cache that prevents from updating an external image? I'd be happy to do it in this PR.
@JerryShih do you know if it makes sense to be able to update an external image handle (now that update_image can take any image type and not just an image buffer)?

@JerryShih
Copy link
Contributor

How do you feel about removing the assertion in resource_cache that prevents from updating an external image? I'd be happy to do it in this PR.
@JerryShih do you know if it makes sense to be able to update an external image handle (now that update_image can take any image type and not just an image buffer)?

@nical
Yes, it could be. But, the size and the format of new image should be the same as the original one. We haven't deal with the different size image in resource cache. And the update_image also needs to trigger the render_backend for the new display item. Do we have benefit with that?

@nical
Copy link
Contributor Author

nical commented Mar 28, 2017

I clarified things a bit by asserting that the format and size don't change, and I fixed the way tiling is passed: if the size was larger than the maximum texture size we used to force the tile size at 512 instead of using the tiling parameter, we also used to loose the tiling information in update_image.
I also made updating an image that doesn't exist a panic. Long term I think that we should gracefully recover from these with an error instead of panicking but just transparently creating the image seems like papering over bugs. I hope that servo does not rely on this behavior.

r? @glennw

glennw
glennw reviewed Mar 28, 2017
View reviewed changes
webrender/src/resource_cache.rs
// Tiled external images are not implemented.
&ImageData::ExternalHandle(_) => false,
&ImageData::ExternalBuffer(_) => false,
_ => { descriptor.width > limit || descriptor.height > limit }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a follow up, could we explicitly list all the variations here rather than _ ? I like the safety of a compiler bug when new enum variants are introduced.

@glennw glennw self-requested a review March 28, 2017 22:47
@glennw
Copy link
Member

glennw commented Mar 28, 2017

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 086f912 has been approved by glennw

@glennw
Copy link
Member

glennw commented Mar 29, 2017

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 80b500a has been approved by glennw

@bors-servo
Copy link
Contributor

⌛ Testing commit 80b500a with merge 8f92619...

bors-servo pushed a commit that referenced this pull request Mar 29, 2017
Auto merge of #1004 - nical:blob-img-updates, r=glennw
8f92619 
Allow updating images that are not raw buffers

```update_image``` currently takes a vector of bytes and the implementation expects this vector to be regular image data. We need to be able to update blob images as well.

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

☀️ Test successful - status-travis
Approved by: glennw
Pushing 8f92619 to master...

@bors-servo bors-servo merged commit 80b500a into servo:master Mar 29, 2017
This was referenced Mar 29, 2017
Merged
Merged
@staktrace staktrace mentioned this pull request Mar 30, 2017
Closed
@nical nical mentioned this pull request Mar 30, 2017
Merged
bors-servo pushed a commit that referenced this pull request Mar 30, 2017
Auto merge of #1036 - nical:tiled-external-buffer, r=kvark
8722163 
Tile external buffer images when they exceed the maximum texture size.

Fixes issue #1027, and addresses a review comment from PR #1004 about explicitly enumerating all variants instead of using ```_ => { ... }``` in this match expression.

r? @kvark

<!-- 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/1036)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glennw glennw glennw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nical @glennw @JerryShih @bors-servo