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

Serialization-Deserialization for DynamicImage using PNG and TIFF #2224

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

Conversation

sunipkm
Copy link

@sunipkm sunipkm commented May 2, 2024

Addresses #2215.

Changes:

  1. RGB32F and RGBA32F support implemented to facilitate serialization of all variants of DynamicImage to standard formats (PNG for integer pixels, TIFF for float32 pixels).
  2. DynamicImage objects are encoded to PNG, or, encoded to TIFF and Zlib::Deflate compressed, and base64-encoded for serialization. Inversion is performed for deserialization.

Copy link
Contributor

@fintelia fintelia left a comment

Choose a reason for hiding this comment

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

I left a few comments, but I'd really encourage you to try to break up PRs more. A couple line PR can often be merged with a single glance once the tests pass. A couple dozen line PR might take a round or two of back-and-forth. When you start getting to multiple hundreds of lines long, they can take many rounds and have a high risk of stalling out

@@ -55,13 +55,17 @@ rgb = { version = "0.8.25", optional = true }
tiff = { version = "0.9.0", optional = true }
zune-core = { version = "0.4.11", default-features = false, optional = true }
zune-jpeg = { version = "0.4.11", optional = true }
base64 = "0.22"
serde = "1.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather have serde be an optional feature. That would also let us make it depend on the png and tiff features

@@ -310,6 +339,14 @@ fn u8_slice_as_u16(buf: &[u8]) -> ImageResult<&[u16]> {
})
}

fn u8_slice_as_f32(buf: &[u8]) -> ImageResult<&[f32]> {
bytemuck::try_cast_slice(buf).map_err(|err| {
ImageError::Parameter(ParameterError::from_kind(ParameterErrorKind::Generic(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is changing the encoder interface. There should be no requirement that the buffer is over-aligned

Copy link
Author

Choose a reason for hiding this comment

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

4-byte alignment is required only for RGB32 images (which are being added in), does that break the encoder interface?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Other parts of the crate allow 1-byte aligned buffers for storing 16-bit integers or 32-bit floats. For instance, the HDR decoder which decodes f32's into a byte slice.

The reason we allow it is that you can't easily create an over-aligned Vec<u8> and we don't want to force users to store image data in typed slices before passing them to encoders/decoders. And it would be too easy of a gotcha to overlook when using the API

};
use std::{fmt, io::{Cursor, Write}};

impl<'de> Deserialize<'de> for DynamicImage {
Copy link
Contributor

Choose a reason for hiding this comment

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

This feel like over complicating things. We could instead use serde-derive with a proxy struct:

mod serialize {
    /// Proxy struct for serializing a DynamicImage
    #[derive(Serialize, Deserialize)]
    struct DynamicImage(Vec<u8>);

    impl<'de> Deserialize<'de> for crate::DynamicImage {
        fn deserialize<D: Deserializer<'de>>(deserializer: D) -> Result<DynamicImage, D::Error> {
            let img = DynamicImage::deserialize(deserializer)?;
            crate::load_from_memory(&img.0).map_err(de::Error::custom)
        }
    }

    ...
}

"tiff" => {
use flate2::read::ZlibDecoder;
use std::io::Read;
let mut decoder = ZlibDecoder::new(imgdata.as_slice());
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to use compression in the TIFF case, but I think that should be added as a feature to the tiff crate rather than doing it specially here.

Copy link
Author

Choose a reason for hiding this comment

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

I am adding a factory-style ‘with_compression’ method to the ‘TiffEncoder’ to maintain API compatibility - ‘new’ defaults to uncompressed.

let res =
deserializer.deserialize_struct("DynamicSerialImage", FIELDS, SerialBufferVisitor)?;

let mut imgdata = STANDARD_NO_PAD.decode(res.data.as_bytes()).map_err(|e| {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather skip the base64 coding step. The efficient serialization formats like bincode actually get worse sending base64 strings rather than the raw bytes

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.

2 participants