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

Implement a rows() iterator on SubImage #2300

Open
Shnatsel opened this issue Jul 30, 2024 · 6 comments
Open

Implement a rows() iterator on SubImage #2300

Shnatsel opened this issue Jul 30, 2024 · 6 comments
Labels
kind: API missing or awkward public APIs

Comments

@Shnatsel
Copy link
Contributor

Shnatsel commented Jul 30, 2024

Right now the ImageBuffer type has a rows() function to iterate over the rows of the image, but GenericImageView SubImage does not.

Working with rows rather than individual pixels is often much faster, because it lets you leverage SIMD to process multiple pixels at once. For example, #2295 would be trivial to fix given this API.

@fintelia
Copy link
Contributor

fintelia commented Jul 31, 2024

What would be the return type of this function? ImageBuffer::rows() gets good performance because it is able to return a wrapper on top of ChunksExact that works really well with SIMD. But GenericImageView currently doesn't make any guarantee pixel layout, so it would presumably have to return something different

@Shnatsel Shnatsel changed the title Implement a rows() iterator on GenericImageView Implement a rows() iterator on SubImage Jul 31, 2024
@Shnatsel
Copy link
Contributor Author

Ah, you are correct! With GenericImageView we have no guarantee that pixels in a row are laid out in memory contiguously.

We can however guarantee this about the SubImage type. I've updated the description to make this about SubImage.

@fintelia
Copy link
Contributor

The SubImage type is a wrapper around SubImageInner which is a wrapper around an GenericImageView (the struct itself doesn't have the trait bound, but the methods/impl blocks do have it).

I've gone in circles about this part of the API. First I think that it would be nice to constrain the layout of GenericImage. Then I realize that you can just use ImageBuffer directly if you want that, so I think about deprecating GenericImage. But that would break a lot of downstream code, so might as well just leave it around?

@Shnatsel
Copy link
Contributor Author

I don't think GenericImageView being that abstract makes a whole lot of sense. Per-pixels accesses will always be slow, so building an API solely around them feels rather pointless.

I think we need a type or a trait for a rectangular sub-image of an existing image, so that we could implement rows() on it. This is what's needed for operations like "make a copy of this region" (crop), "draw something in this region", etc. Naturally it can also be the entire image. We could change GenericImageView to be that, or add an entirely new type. Personally I'd rather change GenericImageView.

@ripytide
Copy link
Member

ripytide commented Aug 2, 2024

How often is image pixel data non-contiguous, and is it worth catering to the potential non-contiguous image use-cases at the cost of heavier genericness? Additionally, do crop and region drawing functions even need a SubImage type at all rather than simply taking a Rect type?

Perhaps we should simply remove/deprecate all the generic traits since they have dubious usefulness and could be misleading to users.

@kornelski
Copy link
Contributor

From perf perspective having a match on an enum per row is not too bad, as long as the innermost loop can be iterating over a slice (loop per pixel type).

@Shnatsel Shnatsel added the kind: API missing or awkward public APIs label Sep 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: API missing or awkward public APIs
Projects
None yet
Development

No branches or pull requests

4 participants