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

Switch to the rgb crate v0.9 pixel types and trait. #2259

Open
ripytide opened this issue Jun 13, 2024 · 5 comments
Open

Switch to the rgb crate v0.9 pixel types and trait. #2259

ripytide opened this issue Jun 13, 2024 · 5 comments

Comments

@ripytide
Copy link
Member

Although not actually released yet, when v0.9 is released it should have several Pixel traits that are just as powerful or more than the one used already in this crate. Similar to that from pixeli demonstrated in #2255.

It would also make this crate more interoperable with other image crates as the rgb pixel types are commonly used for interoperability.

This would be a breaking change so I'd suggest we implement it for v0.26 #2245. This may also conflict with #2239.

@kornelski
Copy link
Contributor

Having interoperability with the image types would be awesome. The image crate already depends on the rgb crate via AVIF encoder.

I plan to use the semver trick to keep structs in rgb v0.8 and v0.9 compatible. The v0.9 has an option to add a cleaned-up Pixel trait and other traits, to better align with the image crate.

@fintelia
Copy link
Contributor

The dynamic of having a contributor say "let's do X!" and then expecting the maintainers to figure out what impact the change would have on downstream users, future compatibility, etc. and write-up a full evaluation of the tradeoffs is exhausting.

Would a significant portion of our users have to make code changes if we did this change? How much effort would that be? Would they get good compiler error messages to guide them? What benefit would they get in return for that effort?

Would this create API inconsistencies with other parts of the crate? Or add new edge cases where the API produced confusing or incorrect output? Prevent other feature additions in the future? Make it more difficult to drive down the amount of unsafe code in the dependency tree?

Are there any alternatives we could do instead? Are there workarounds that users could take today to gain the benefits without us doing an API change at all?

@ripytide
Copy link
Member Author

ripytide commented Aug 26, 2024

The dynamic of having a contributor say "let's do X!" and then expecting the maintainers to figure out what impact the change would have on downstream users, future compatibility, etc. and write-up a full evaluation of the tradeoffs is exhausting.

I don't know where you got this impression from? I view anyone working on open source (including maintainers) as volunteers and as such I have no expectations of any work at all.

I am simply suggesting an idea that I think has a good probability of being an improvement to the crate in my opinion, isn't that what we all do with such feature request type issues?

Would a significant portion of our users have to make code changes if we did this change?

This would depend quite a bit of the specific nature of usage of the image library, if users are just loading and saving images in various formats then they wouldn't have to make any changes, but if they are doing pixel-level work then they probably would need to make changes.

How much effort would that be?

This would depend on how the rgb crate is integrated with the image crate, if we re-export the rgb types from where the old image pixel types used to be then the old imports will still work but point to the new pixel types. The pixel initialisation would have to change from Rgb([0, 0, 0]) to either Rgb {r: 0, g: 0, b: 0} or Rgb::new(0, 0, 0) though which is probably the most frequent code change required.

Would they get good compiler error messages to guide them?

I believe so yes, last time I tried migrating the image libraries pixel types the compiler was able to suggest both of the two new ways of defining a pixel (literal or new()).

What benefit would they get in return for that effort?

A more rust-modern powerful and uniform set of pixel types and traits (in my opinion), with more compatibility/interoperability/standardization with other rust crates. Take a look at the new rgb crates traits, they can do a lot of cool stuff not yet provided by the traits in the image crate, such as Pixel::map() which can map to a different component type, Pixel::to_array() which can return a pixel-specifically sized array, and more.

Would this create API inconsistencies with other parts of the crate? Or add new edge cases where the API produced confusing or incorrect output?

Do you have any specific areas in mind that this change might create inconsistencies with? I can't really see any at the moment from my limited experience with the image crate, but I'm sure if there are some they are likely to be found when implementing the change. The same applies to edge cases and confusing or incorrect output I think. It's probably all much easier to discuss on a case-by-case basis when implementing the PR.

Prevent other feature additions in the future?

This is a very tricky question to answer because it's very open-ended. I can't really think of any reasons why it might prevent other feature additions, but again I do only have limited experience with the library. It might also depend heavily on the specific type of feature being added.

Make it more difficult to drive down the amount of unsafe code in the dependency tree?

The only unsafe code in the eventual v1.0 rgb release will be inside the Pixel::as_array() and Pixel::as_array_mut() functions, but the way it is used it is safe. I don't think eliminating absolutely all unsafe code from inside every dependency is a great or feasible goal since even the standard library is chock-a-block full of unsafe code, I think it's more about writing safe abstractions around well-verified usages of unsafe code that can be used by users (which I think applies to both as_array() and as_array_mut()).

Are there any alternatives we could do instead?

You could copy-paste the pixel types/traits from the rgb crate into a module inside the image crate, but that would have the disadvantage of duplicated maintenance responsibility and would cancel any interoperability benefits.

Are there workarounds that users could take today to gain the benefits without us doing an API change at all?

Perhaps users could manually convert between image pixel types to rgb types, perhaps someone could even write a shim crate like image-rgb-conversion that offered traits to convert between them. That would require less work to be done on the image crate but would increase the work needing to be done by any users requiring interop between the two crates' pixel types.

@fintelia
Copy link
Contributor

Based on the increasing number of users and how many code changes would be needed, I think this would likely be the single most disruptive change this crate has ever made. I say that not to be discouraging or to indicate that we definitely won't do it, but to get across that this is not a change we can make lightly.

You don't have to be the one to write-up a detailed analysis of how the switch would work, and what the pro and cons would be. However, somebody would have to do so if we were going to make this change

@ripytide
Copy link
Member Author

ripytide commented Aug 28, 2024

You don't have to be the one to write-up a detailed analysis of how the switch would work, and what the pro and cons would be. However, somebody would have to do so if we were going to make this change

I thought that was what I've just done by answering all your questions in my previous comment?

Alternatively, I could create a draft-PR with the changeover implemented in it so we can do some empirical breaking change testing on some reverse dependencies which might be easier than attempting to predict all the effects ahead of time?

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

No branches or pull requests

3 participants