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

image: Allow any kind of data that implements AsRef<[u8]> for the i… #1551

Merged
merged 2 commits into from Feb 17, 2023

Conversation

sdroege
Copy link
Contributor

@sdroege sdroege commented Nov 20, 2022

…mage data

It's not required anywhere for it to be a plain slice or a Vec and this makes it possible to use data allocated in a different way without copying.

Also store the data in an Arc to allow cheap cloning.

@13r0ck
Copy link
Member

13r0ck commented Nov 20, 2022

Image handle already uses an arc

pub struct Handle {

https://github.com/iced-rs/iced/blob/master/examples/pokedex/src/main.rs#L113

@sdroege
Copy link
Contributor Author

sdroege commented Nov 20, 2022

Ah I see, thanks! Then this can also become a plain Box<dyn _> instead.

Is the Clone impl on Data then actually needed? If it's needed this needs a bit more code (as Clone is not object-safe) but also not much of a problem.

native/src/image.rs Outdated Show resolved Hide resolved
sdroege and others added 2 commits February 17, 2023 14:23
…mage data

It's not required anywhere for it to be a plain slice or a `Vec` and
this makes it possible to use data allocated in a different way without
copying.
Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

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

Thanks for this!

I have moved the Arc from Handle to Bytes since the other fields in Data should be fairly cheap to Clone and we can get rid of the awkward trait this way.

@hecrj hecrj added feature New feature or request performance image labels Feb 17, 2023
@hecrj hecrj added this to the 0.8.0 milestone Feb 17, 2023
@hecrj hecrj enabled auto-merge February 17, 2023 13:42
@hecrj hecrj merged commit f75e020 into iced-rs:master Feb 17, 2023
ImageBytes(self.0.clone_boxed())
impl Bytes {
/// Creates new [`Bytes`] around `data`.
pub fn new(data: impl AsRef<[u8]> + Clone + Send + Sync + 'static) -> Self {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You don't really need the Clone here anymore as the Arc already gives you that :)
Do you want me to submit a PR for removing the trait bound or will you take care of the change?

Copy link
Member

Choose a reason for hiding this comment

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

Good catch. Just opened #1717.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@sdroege sdroege deleted the image-data-refcounting branch February 19, 2023 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request image performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants