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

Refactor optimize_raw #670

Merged
merged 6 commits into from
Jan 29, 2025
Merged

Refactor optimize_raw #670

merged 6 commits into from
Jan 29, 2025

Conversation

andrews05
Copy link
Collaborator

@andrews05 andrews05 commented Jan 25, 2025

Code always tends to get messy over time. I've found the optimize_raw function increasingly harder to read, particularly after the addition of fast mode, so I've taken some time to refactor and simplify it.

One change of note here is the main compression trials now use the Evaluator. This means verbose output is a little different which is shown below.

There is no change to performance or output size.

-vvo2: master

Processing: tests/files/rgba_8_should_be_palette_4.png
    500x400 pixels, PNG format
    8-bit RGB + Alpha, non-interlaced
    IDAT size = 2757 bytes
    File size = 18109 bytes
Eval: 4-bit Indexed (5 colors)      None       1837 bytes
Eval: 8-bit Indexed (5 colors)      None       1988 bytes
Eval: 4-bit Indexed (5 colors)      Bigrams   >1837 bytes
Eval: 8-bit Indexed (5 colors)      Bigrams   >1837 bytes
Transformed image to 4-bit Indexed (5 colors), non-interlaced
Evaluating: 2 filters
Eval: 4-bit Indexed (5 colors)      Sub       >1810 bytes
Eval: 4-bit Indexed (5 colors)      Entropy   >1810 bytes
Trying: None
    zc = 11  f = None      1583 bytes
Found better combination:
    zc = 11  f = None      1583 bytes
    IDAT size = 1583 bytes (1174 bytes decrease)
    file size = 16962 bytes (1147 bytes = 6.33% decrease)
16962 bytes (6.33% smaller): Running in pretend mode, no output

-vvo2: PR

Processing: tests/files/rgba_8_should_be_palette_4.png
    500x400 pixels, PNG format
    8-bit RGB + Alpha, non-interlaced
    IDAT size = 2757 bytes
    File size = 18109 bytes
Eval: 4-bit Indexed (5 colors)      None       1837 bytes
Eval: 8-bit Indexed (5 colors)      None       1988 bytes
Eval: 4-bit Indexed (5 colors)      Bigrams   >1837 bytes
Eval: 8-bit Indexed (5 colors)      Bigrams   >1837 bytes
Transformed image to 4-bit Indexed (5 colors), non-interlaced
Evaluating 2 filters
Eval: 4-bit Indexed (5 colors)      Sub       >1810 bytes
Eval: 4-bit Indexed (5 colors)      Entropy   >1810 bytes
Trying filter None with zc = 11
1610 bytes
Found better result:
    zc = 11, f = None
    IDAT size = 1583 bytes (1174 bytes decrease)
    file size = 16962 bytes (1147 bytes = 6.33% decrease)
16962 bytes (6.33% smaller): Running in pretend mode, no output

-vvZo5: master

Processing: tests/files/rgba_8_should_be_palette_4.png
    500x400 pixels, PNG format
    8-bit RGB + Alpha, non-interlaced
    IDAT size = 2757 bytes
    File size = 18109 bytes
Eval: 8-bit Indexed (battiato sort) None       1821 bytes
Eval: 4-bit Indexed (5 colors)      None       1657 bytes
Eval: 8-bit Indexed (mzeng sort)    None       1821 bytes
Eval: 8-bit Indexed (5 colors)      None       1821 bytes
Eval: 8-bit Indexed (battiato sort) Bigrams   >1821 bytes
Eval: 4-bit Indexed (5 colors)      Bigrams   >1657 bytes
Eval: 8-bit Indexed (mzeng sort)    Bigrams   >1657 bytes
Eval: 8-bit Indexed (5 colors)      Bigrams   >1657 bytes
Transformed image to 4-bit Indexed (5 colors), non-interlaced
Trying: 8 filters
    zc = zopfli  f = Brute     1562 bytes
    zc = zopfli  f = Sub      >1562 bytes
    zc = zopfli  f = Bigrams  >1562 bytes
    zc = zopfli  f = None      1407 bytes
    zc = zopfli  f = Up       >1407 bytes
    zc = zopfli  f = MinSum   >1407 bytes
    zc = zopfli  f = BigEnt   >1407 bytes
    zc = zopfli  f = Entropy  >1407 bytes
Found better combination:
    zc = zopfli  f = None      1407 bytes
    IDAT size = 1407 bytes (1350 bytes decrease)
    file size = 16786 bytes (1323 bytes = 7.31% decrease)
16786 bytes (7.31% smaller): Running in pretend mode, no output

-vvZo5: PR

Processing: tests/files/rgba_8_should_be_palette_4.png
    500x400 pixels, PNG format
    8-bit RGB + Alpha, non-interlaced
    IDAT size = 2757 bytes
    File size = 18109 bytes
Eval: 8-bit Indexed (battiato sort) None       1821 bytes
Eval: 4-bit Indexed (5 colors)      None       1657 bytes
Eval: 8-bit Indexed (mzeng sort)    None       1821 bytes
Eval: 8-bit Indexed (5 colors)      None       1821 bytes
Eval: 8-bit Indexed (battiato sort) Bigrams   >1657 bytes
Eval: 4-bit Indexed (5 colors)      Bigrams   >1657 bytes
Eval: 8-bit Indexed (mzeng sort)    Bigrams   >1657 bytes
Eval: 8-bit Indexed (5 colors)      Bigrams   >1657 bytes
Transformed image to 4-bit Indexed (5 colors), non-interlaced
Trying 8 filters with zopfli, zi = 15
Eval: 4-bit Indexed (5 colors)      Brute      1589 bytes
Eval: 4-bit Indexed (5 colors)      Bigrams    1641 bytes
Eval: 4-bit Indexed (5 colors)      Sub        1711 bytes
Eval: 4-bit Indexed (5 colors)      None       1434 bytes
Eval: 4-bit Indexed (5 colors)      Up         1764 bytes
Eval: 4-bit Indexed (5 colors)      MinSum     1760 bytes
Eval: 4-bit Indexed (5 colors)      BigEnt     1742 bytes
Eval: 4-bit Indexed (5 colors)      Entropy    1748 bytes
Found better result:
    zopfli, zi = 15, f = None
    IDAT size = 1407 bytes (1350 bytes decrease)
    file size = 16786 bytes (1323 bytes = 7.31% decrease)
16786 bytes (7.31% smaller): Running in pretend mode, no output

@andrews05
Copy link
Collaborator Author

@AlexTMjugador Thanks for merging the APNG update, hope you're ready for another one 😄

I've got one minor fix which I'll post after this and then we can probably do another release.

@ace-dent
Copy link

ace-dent commented Jan 26, 2025

Thanks @andrews05 - I do (slightly) prefer the original verbose output. I occasionally write scripts to extract a result (e.g. filter tested), so for some these changes may break their workflow. If no one speaks up, I'm sure it's absolutely fine. 👍

Oh- and if you are adding zopfli iterations, as per #658 (comment), that would be an awesome addition!! 🤞

@andrews05
Copy link
Collaborator Author

andrews05 commented Jan 27, 2025

Thanks @andrews05 - I do (slightly) prefer the original verbose output. I occasionally write scripts to extract a result (e.g. filter tested), so for some these changes may break their workflow. If no one speaks up, I'm sure it's absolutely fine. 👍

Is there something in particular you think could be improved, or is it just a matter of updating the script to handle the new output? I’m afraid the output isn’t something the user can rely on to be “stable” and changes between versions (even minor versions) can be expected.

Oh- and if you are adding zopfli iterations, as per #658 (comment), that would be an awesome addition!! 🤞

Unfortunately no, this is just showing the value of the zi parameter which defaults to 15. As noted in that thread though, you can parse zopfli’s debug output to try and find what you’re looking for.

@ace-dent
Copy link

Unfortunately no, this is just showing the value of the zi parameter

Ah. That's not really changing after we set the CLI parameters, right?... So surely it will just always report our initial -zi set?... Unless in future we can set a brute iteration parameter...

Copy link
Collaborator

@AlexTMjugador AlexTMjugador left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, it looks good to me!

On a related note, I don't believe changing the output status text format should be considered a breaking change for semver purposes, as such output was never meant to serve as a non-human interface to begin with. However, introducing a JSON-formatted output, a stable, script-friendly format toggle, or something along those lines in the future could help with use cases that depend on parsing status messages.

src/deflate/mod.rs Outdated Show resolved Hide resolved
src/deflate/mod.rs Outdated Show resolved Hide resolved
@andrews05
Copy link
Collaborator Author

andrews05 commented Jan 29, 2025

Thanks @AlexTMjugador, good tip on write!. I've changed ColorType to use that as well.

@ace-dent Yes, that's correct. It's just like how we show zc = 12 or similar for libdeflate.

@AlexTMjugador AlexTMjugador merged commit bbde68d into shssoichiro:master Jan 29, 2025
12 checks passed
@andrews05 andrews05 deleted the refactor branch January 29, 2025 20:21
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.

3 participants