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

[wip] support reading generic imageS #154

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

edgarriba
Copy link
Member

No description provided.

GenericImage::L8(Image::<u8, 1>::new(size, img.into_luma8().to_vec())?)
}
image::ColorType::Rgb8 => {
GenericImage::Rgb8(Image::<u8, 3>::new(size, img.into_rgb8().to_vec())?)
Copy link
Contributor

Choose a reason for hiding this comment

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

One more change is into_vec (instead of to_vec) as noted in #149 (comment) . Just wanted to remind you in case you missed it. (I haven't tried it myself, so please ignore if it has any issues/negatives).

  • into_vec() consumes the image and gives us the inner vector without copy. to_vec, on the other hand, seems to point to slice::to_vec (according to rust-analyzer) which copies.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, my mistake

let image = read_image_any("../../tests/data/dog.jpeg")?;
// let image: Image<u8, 3> = read_image_any("../../tests/data/dog.jpeg")?;
let image: super::GenericImage = read_image_any("../../tests/data/dog.jpeg")?;
// NOTE: then how to access the size? we need to reimplment the methods ??
Copy link
Member Author

Choose a reason for hiding this comment

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

@emilmgeorge @jandremarais do you have any ideas here how to solve this ? Initially I wanted somehow that the read function could return the Image struct so that we can use directly the underlying api

Copy link
Contributor

Choose a reason for hiding this comment

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

How about returning a Box<dyn AnyImage> with

trait AnyImage {
    fn size(&self) -> ImageSize;
}

impl<T: SafeTensorType, const C: usize> AnyImage for Image<T, C> {
    fn size(&self) -> ImageSize {
        self.size()
    }
}

Copy link
Member Author

@edgarriba edgarriba Sep 30, 2024

Choose a reason for hiding this comment

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

it's another option, however wouldn't be difficult to scale/maintain this as soon as we increase the functionality in Image ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it would be more effort than the current enum approach but I can give it more thought. What if the AnyImage trait owns all the image functionality? Then we don't have to implement it twice

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think using traits to decouple the functionality may be a good direction, although I haven't fully thought it through.

If considering/implementing the above needs more time, another limited option for now, is to use the enum type only for returning and force the user to convert the enum to a concrete type before further operations. We can provide functions like into_rgb8() -> Result<Image<u8, 3>, XXError> etc. in impl GenericImage {}.

Based on use cases:

  • User who does not care about the pixel type at all
    Not easily possible with this limited solution. User has to either handle all image types with match (maybe macros?) or convert to one specific type (as below).

    Trait based solutions may work for this use case.

  • User who cares only about the final image type

    let gimg: super::GenericImage = read_image_any("../../tests/data/dog.jpeg")?;
    let gray = gimg.into_l8()?;
    // Any operation for Image<u8, 1>
  • User who wants to handle only specific type images

    let gimg: super::GenericImage = read_image_any("../../tests/data/dog.jpeg")?;
    if let GenericImage::Rgb8(image) = gimg {
       // Any operation for Image<u8, 3>
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

that sounds a good option. Also in this direction, i want start getting rid of image-rs and implement directly the decoders we want, which can lead to have a pool of free functions to read from different formats in the format of read_rgb8 , read_l8, etc. Possibly by curating first png, jpeg, exrand maybetiff` which are the most common formats used in ml/robotics

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants