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

Add Rgb64FImage and Rgba64FImage #1802

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

Conversation

oberien
Copy link

@oberien oberien commented Oct 10, 2022

I license past and future contributions under the dual MIT/Apache-2.0 license, allowing licensees to choose either at their option.

@fintelia
Copy link
Contributor

Could you say a bit about what use cases you have for 64-bit floating point images?

@oberien
Copy link
Author

oberien commented Oct 10, 2022

I'm fiddling around with astrophotography stacking. I need to perform operations on tens on thousands of images. In that order float imprecision errors start to creep up. To reduce these losses, I'd like to work on f64 images.

Copy link
Member

@HeroicKatora HeroicKatora left a comment

Choose a reason for hiding this comment

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

Seems good to me, it doesn't complicate color representation too much and conversion where it was previously possible with f32 should work. LGTM.

@fintelia
Copy link
Contributor

I looked at this a bit more closely. It is already possible to work with 64-bit ImageBuffers either by writing out the full type name or by defining Rgb64FImage and Rgba64FImage aliases yourself.

The main thing this PR adds is a PixelWithColorType implementation and ColorType/DynamicImage variants for dealing with f64 images. Those additions make me somewhat more nervous because they grow the API surface yet might be used rarely/never. (It isn't clear to me they'd even help with your image stacking task to dynamically switch color types at runtime since the types involved should be known statically at compile time)

@HeroicKatora
Copy link
Member

The traits FromPrimitive and EncodableLayout aren't implemented on these types, yet. It could just be those for a start and not the ImageCanvas extension. This issue gives other precedent that the pure picture notion is too narrow and types beyond the defaults should be considered.

The concern about DynamicImage sounds valid, though. There's no guarantee that any of the operations we already define for it will compute in f64 themselves which could be confusing. That said, I'd treat these as a normal bug or even lower priority as GPU optimizations are surely worse for accuracy on textures than what we're doing. Being able to represent the values in the buffer is a necessary precursor to correctness here in any case and that at least allows them to be implemented externally. We've had DynamicImage as a non_exhaustive enumeration nd this sounds as good a time to make use of it as any.

@oberien
Copy link
Author

oberien commented Oct 12, 2022

Before creating this PR, I did in fact write my own type Rgb64FImage = ImageBuffer<Rgb<f64>, Vec<f64>>. However, some utility functions, helper functions and conversions aren't usable from a third party context due to the sealed traits SealedPixelWithColorType and EncodableLayout. That is why I though it was a good idea to add this directly to the image library.

In my particular use-case I was missing image conversion from RgbImage to Rgb64FImage (loading pngs) and vice versa (saving the result back as a png) and needed to manually implement these conversions.

Regarding adding DynamicImage::ImageRgb64F, in addition to the link regarding tiff files linked by @HeroicKatora, I plan on supporting FITS images in the future, which allow "unsigned 8-bit bytes, 16 and 32-bit signed integers, and 32 and 64-bit single or double precision floating point reals. FITS can also store 16 and 32-bit unsigned integers."

As FITS support is still quite a way in the future, I'd be fine with leaving out DynamicIage::ImageRgba?64F for now if that makes the PR easier to merge.

@fintelia
Copy link
Contributor

Adding an FromPrimitive should unlock ImageBuffer::convert which seems worthwhile. The EncodableLayout is only useful if we also add a corresponding PixelWithColorType implementation (because both are required to unlock ImageBuffer::{save, save_with_format, write_to}).

I view ColorType/DynamicImage as trying to be a small set of color types that are well supported throughout the crate, including both pairwise conversions between all of them and reasonable handling by the all the encoders and decoders.

More unusual color types can be used by interacting with the format specific crates directly, like how the TIFF crate already supports 64-bit float images. If applicable, we can also have decoders/encoders transparently convert images in unsupported color types into ones we do support. (You could argue this is happening with JPEG when YCbCr colors get mapped to RGB.)

In the case of Pillow, I believe that this is the analogous listing of color types.

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.

None yet

3 participants