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

Load images from memory and image viewer example #90

Merged
merged 8 commits into from
Dec 4, 2019

Conversation

hecrj
Copy link
Member

@hecrj hecrj commented Nov 30, 2019

Closes #76.

Image viewer - Iced

This PR implements an image::Handle type which allows displaying images from data living in-memory and from the filesystem as well. Additionally, the PR adds basic image cache trimming to iced_wgpu.

An image viewer example has been built to showcase a simple way to load remote images asynchronously.

Changelog

Added

  • image::Handle type with from_path and from_memory methods.
  • image::Data enum representing different kinds of image data.
  • Color::from_rgb8 to easily build a Color from its hexadecimal representation.

Changed

  • Image::new takes an Into<image::Handle> now instead of an Into<String>.

Fixed

  • Image widget not keeping aspect ratio consistently.

New `image::Handle` opaque type uniquely identifying some `image::Data`,
allowing reliable caching.
@hecrj hecrj added the feature New feature or request label Nov 30, 2019
@hecrj hecrj added this to the 0.1.0 milestone Nov 30, 2019
@hecrj hecrj self-assigned this Nov 30, 2019
@hecrj hecrj changed the title Image viewer Load images from memory and image viewer example Nov 30, 2019
@hecrj hecrj mentioned this pull request Nov 30, 2019
@ejmahler
Copy link
Contributor

ejmahler commented Nov 30, 2019

  1. I tried to compile this branch, via the following cargo.toml entry, and got this compile error, on both rustc 1.39 and latest nightly: EDIT: False alarm, I had a very old cargo.lock that I had to delete

  2. From looking at the source, it looks like you're using the image-rs crate under the hood. Are you opposed to just doing a public export of the image crate? I think that will be vastly more flexible than forwarding to load_from_memory, and I suspect that if you don't, you're gong to end up recreating image-rs's public API to cover all the different ways they let users create images. It will also let you sidestep the problem of error handling for image formats, instead letting image-rs handle parsing errors.

If you're interested, I'd be glad to write a version of this that directly accepts an image-rs image.

@hecrj
Copy link
Member Author

hecrj commented Nov 30, 2019

From looking at the source, it looks like you're using the image-rs crate under the hood. Are you opposed to just doing a public export of the image crate?

One of the main focus of Iced is simplicity and I think the image crate can be quite daunting, especially if you are a beginner.

Additionally, we have to consider that this is a dependency of a specific renderer (which turns out to be the default) and Iced also supports the web. We need to control what do we expose and where.

For now, I'd like to gather use cases and keep things simple.

@ejmahler
Copy link
Contributor

ejmahler commented Dec 1, 2019

That's a reasonable approach.

There is one suggestion I have if you're planning to keep this API: right now from_bytes doesn't convey what format it expects the data to be in, but it also doesn't return a Result<> to let you know that you passed in an invalid format -- it just fails silently if you don't guess correctly. One way to help this is to rename from_bytes to decode_from_bytes or parse_from_bytes, which would convey the fact that we expect the image data to be in some sort of encoded image format.

Alternately, Image-rs's equivalent method is called load_from_memory, so maybe there's some benefit to using their naming scheme, and calling it the same.

@hecrj
Copy link
Member Author

hecrj commented Dec 1, 2019

@ejmahler I think we could call it image::Handle::from_memory. I think we could also have an image::Handle::from_rgba_pixmap or similar, at least just for native platforms.

However, we cannot return a Result here because iced_native does not really perform any decoding/parsing. We would have to move the image dependency to iced_native and expose an API useful enough for renderers to avoid duplicating work (i.e. parsing the Image again).

For now, an Image renderer implementation needs to be fault-tolerant. Just like users can provide a path to a file that does not exist, they can also provide invalid image data.

It's true that, in this case, iced_wgpu currently simply fails silently and does not show anything, but in the near future we should show an indicator that an image failed to load.

@ejmahler
Copy link
Contributor

ejmahler commented Dec 2, 2019

I was able to get iced working, and I made a proof of concept of this feature. check it out if you're curious:

https://github.com/ejmahler/SandpileFractal

You can see the actual rendering code where it converts an image buffer to an image handle here: https://github.com/ejmahler/SandpileFractal/blob/master/src/render.rs#L71

In this particular case, it would be more convenient to pass the uncompressed RGB data directly through to iced, but it doesn't look like it has any kind of performance impact. Or if there is one, it hidden by the fact that the work is being done in a background thread, so the UI keeps feeling responsive.

Screenshot:
image

@clarkmoody
Copy link
Contributor

@ejmahler Amazing!

@hecrj
Copy link
Member Author

hecrj commented Dec 2, 2019

@ejmahler That's awesome! I wonder if writing the image using BMP format would speed things up.

I guess we can avoid implementing pixmap methods for the time being, as advanced users like you can leverage the image crate like this. I think this satisfies the same use cases as your suggestion of exposing the image crate directly, but with a performance cost.

I don't think we should focus too much on performance yet, so I am okay with this for now.

@hecrj hecrj merged commit d1eb187 into master Dec 4, 2019
@hecrj hecrj deleted the feature/image-from-bytes branch December 4, 2019 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image::from_bytes factory
3 participants