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

Added serialization-deserialization and FITS output. #2212

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

Conversation

sunipkm
Copy link

@sunipkm sunipkm commented Apr 23, 2024

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

Here is a summary of changes:

  • Images may now contain optional metadata (minor).
  • Implemented serialization and deserialization for the DynamicImage object for transport over the network, with Zlib compression of the image container (minor).
  • Images can now be saved as compressed/uncompressed FITS files if the fitsio feature flag is set (breaking).
    • If the fitsio feature is enabled, imposes the additional WriteImage trait bound on the Primitive trait.
      This trait bound (and the fitsio feature) is optional to compile image crate on wasm targets, since fitsio crate requires the cfitsio library.
    • Removes the Primitive trait implementations for usize, isize and u64. fitsio::images::WriteImage is not implemented for these types, and these pixel types could be deemed unusable since image does not serialize these types natively, and any of the image encoders/decoders do not support these types.
  • Exposed the 16-bit buffer types (e.g. Gray16Image, minor).

@fintelia
Copy link
Contributor

fintelia commented Apr 23, 2024

Thanks for your interest in image-rs!

Changes of this scale need to be discussed before we can get to the stage of reviewing a specific PR.

We also strongly prefer breaking up changes as much as possible because it makes them faster to review and reduces the amount of back-and-forth interaction to get any individual change merged. Each of your bullet point summary items likely warrant separate discussion issue and (if we decided the feature was something that belonged in this crate) its own PR.

@sunipkm
Copy link
Author

sunipkm commented Apr 23, 2024

That’s fair, glad to know you are interested. I will break this up into three and submit tomorrow:

  1. The “metadata”.
  2. Serialization-deserialization.
  3. FITS IO.

@fintelia
Copy link
Contributor

Before working on PRs could you create issues to discuss each of those? They all seem useful but I'm not yet sold that they are all things we want to add to this specific crate

@sunipkm
Copy link
Author

sunipkm commented Apr 29, 2024

Through our discussion, I see how metadata is challenging. Storing FITS images also appear to be outside the scope of the image crate, because of the niche use case, and more importantly, the C dependency. However, I think serde is important, and doable, and we should discuss that.

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

2 participants