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

An API to compress raw image data? #376

Closed
RReverser opened this issue Jan 14, 2021 · 14 comments · Fixed by #482
Closed

An API to compress raw image data? #376

RReverser opened this issue Jan 14, 2021 · 14 comments · Fixed by #482
Labels
I-Minor Issues that would be nice to have fixed, but are the lowest priority T-Feature Requests for a new feature to be added

Comments

@RReverser
Copy link
Contributor

Right now, in Squoosh, OxiPNG is the only codec where we need to encode the raw image data we already have (RGBA) into PNG with browser API, only to pass to OxiPNG, which will decompress it back.

I wonder if there could be a way to expose an API that would accept raw RGBA byte array and try to compress that directly. This could make it both faster and smaller by excluding the decoder from the process.

@TPS
Copy link

TPS commented Feb 8, 2021

Would something like https://github.com/ShyykoSerhiy/canvas-png-compression help in the interim by allowing a level-0/1 compressed PNG as the input?

@RReverser
Copy link
Contributor Author

@TPS IIUC, that's completely different codec, so no.

@TPS
Copy link

TPS commented Feb 8, 2021

Sure, but you're aiming to (1) speedup the process & (2) skip compressing the PNG 2×, right? The 2nd part would involve implementing this RFE, but the 1st could be kludged by something like that, since a level 0 or 1 compression is very fast, & then real recompression could be done by OxiPNG. Else, you're using the browser's PNG API, which is unnecessarily compressing the PNG, when all you need is the format to send to OxiPNG.

@RReverser
Copy link
Contributor Author

@TPS I see what you're saying.

But the real goal here is not as much / not only to speed up the process, but to reduce amount of code loaded in the browser.

Speed-wise this initial compression via browser's API and decompression by OxiPNG is already a relatively fast part of the overall encoding compared to all the combinations that OxiPNG attempts, while size-wise adding one more dependency means only increasing the size even further.

@shssoichiro
Copy link
Owner

This may be worth adding. I don't think it would overly complicate the API and should be reasonably straightforward to implement, as the new method could pass along the image data to the existing internals.

Clearly I haven't had the greatest amount of free time, but I would accept a PR for this (or I'll get around to adding it eventually).

@shssoichiro shssoichiro added I-Minor Issues that would be nice to have fixed, but are the lowest priority T-Feature Requests for a new feature to be added labels Mar 17, 2021
@RReverser
Copy link
Contributor Author

Clearly I haven't had the greatest amount of free time, but I would accept a PR for this (or I'll get around to adding it eventually).

FWIW it's not a high priority on my list either, just something that seems worthwhile adding at some point in the future. (which is another way of saying I'm not sure I'll get around to a PR very soon either :) )

@Zerowalker
Copy link

Is this issue/request about passing raw byte data and having it encoded with oxipng?
I can't really tell if that's what this is about or if it's something else.

I am looking for the first and trying to hack my way through it,
but it seems to be a bit more complex as oxipng does more than just recompress,
so not sure where it's actually doing the recompress part only and not messing with other stuff.

@andrews05
Copy link
Collaborator

I've been thinking about what such an API might look like. I figure if we use the existing PngImage struct it might look like this:

pub fn optimize_from_raw(raw: &PngImage, opts: &Options) -> PngResult<Vec<u8>>;

pub struct PngImage {
    /// The headers stored in the IHDR chunk
    pub ihdr: IhdrData,
    /// The uncompressed, optionally filtered data from the IDAT chunk
    pub data: Vec<u8>,
    /// The palette containing colors used in an Indexed image
    /// Contains 3 bytes per color (R+G+B), up to 768
    pub palette: Option<Vec<RGBA8>>,
    /// The pixel value that should be rendered as transparent
    pub transparency_pixel: Option<Vec<u8>>,
    /// All non-critical headers from the PNG are stored here
    pub aux_headers: IndexMap<[u8; 4], Vec<u8>>,
}

/// Headers from the IHDR chunk of the image
pub struct IhdrData {
    /// The width of the image in pixels
    pub width: u32,
    /// The height of the image in pixels
    pub height: u32,
    /// The color type of the image
    pub color_type: ColorType,
    /// The bit depth of the image
    pub bit_depth: BitDepth,
    /// The compression method used for this image (0 for DEFLATE)
    pub compression: u8,
    /// The filter mode used for this image (currently only 0 is valid)
    pub filter: u8,
    /// The interlacing mode of the image
    pub interlaced: Interlacing,
}

We might want to make some changes to these structs to make them more appropriate here:

  • Remove compression and filter from IhdrData. I believe these are entirely unused.
  • Change PngImage.data to be necessarily unfiltered, not even including the filter byte for each line. I don't think this is actually used for filtered data at any point, so it shouldn't be a problem.

What do you think @RReverser @Zerowalker? Would this work for your use cases?

@RReverser
Copy link
Contributor Author

What do you think @RReverser @Zerowalker? Would this work for your use cases?

Sorry, it's been a while and I no longer work on the project where it was relevant :( TL;DR that was for squoosh.app, where we pass images from canvas ImageData (aka raw RGBA byte array) to various image codecs, but I can't dig in to verify API details at this point anymore.

@TPS
Copy link

TPS commented Dec 17, 2022

@RReverser Is it possible for you to tag who currently might be interested over @ Squoosh? 🙇🏾‍♂️ (Maybe @surma &/or @jakearchibald, but I'm not sure.)

@andrews05
Copy link
Collaborator

Hey all, I've just posted a draft PR #482. I want to make sure it's an appropriate solution that will work for whatever use case you may have, so please check it out and post any feedback you have.

@jakearchibald
Copy link
Contributor

I'm still interested in this, but it isn't urgent for us, since we can encode it to PNG before passing it to Oxi.

@TPS
Copy link

TPS commented May 15, 2023

Now that #482 (& a bevy of other performant PRs) has landed, what do you think, @jakearchibald?

@RReverser
Copy link
Contributor Author

@jakearchibald is out of Google now too 😅 If you want to implement this in a PR, I'm sure one of us will be happy to review it though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-Minor Issues that would be nice to have fixed, but are the lowest priority T-Feature Requests for a new feature to be added
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants