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

Dual purpose and implicit state in PngData #145

Closed
kornelski opened this issue Nov 13, 2018 · 1 comment · Fixed by #158
Closed

Dual purpose and implicit state in PngData #145

kornelski opened this issue Nov 13, 2018 · 1 comment · Fixed by #158
Labels
I-Minor Issues that would be nice to have fixed, but are the lowest priority T-Techdebt Internal changes to improve software maintainability

Comments

@kornelski
Copy link
Contributor

kornelski commented Nov 13, 2018

PngData has both idat_data and raw_data, which are related, but used in different contexts and handled as independent fields.

This is problematic, because:

  • It's possible to modify raw_data without invalidating idat_data. Currently the code has to keep track of that "manually" through a flag passed around. That feels fragile.

  • Copying of PngData copies both of them, even when it's just for trials/reductions which are going to use only one of them.

  • Alpha filter trials get filtered_image version of the idat that needs to be recompressed better later, but there's no way to store the filtered version, so later recompression does filtering again (that's very minor though).

I'm not entirely sure how this should be handled, but there must be a better way. Some ideas:

  • Have separate CompressedPng and UncompressedPng objects, with only either raw or idat, and try to use them throughout the code accordingly.
  • Have one body as enum Compressed/Uncompressed/Both
  • Have getters/setters on PngData that clear idat_data when raw_data is set or borrowed mutably.
@shssoichiro shssoichiro added I-Minor Issues that would be nice to have fixed, but are the lowest priority T-Techdebt Internal changes to improve software maintainability labels Nov 14, 2018
kornelski added a commit to kornelski/tmp-pr-oxipng that referenced this issue Jan 14, 2019
kornelski added a commit to kornelski/tmp-pr-oxipng that referenced this issue Jan 14, 2019
shssoichiro pushed a commit that referenced this issue Jan 27, 2019
* Fix verbose message

* No cloning when restoring original data

* Make reductions return a new uncompressed image

Partially fixes #145

* Async reduction evaluator

* Assert

* Faster bit depth check

* Also try 4-bit depth for small-depth images

* Skip test when using miniz

* Ensure palette is trimmed after depth reduction

Fixes #159

* Fudge factor for reductions to prefer better reductions even if gzip estimation says otherwise
@shssoichiro shssoichiro reopened this Jan 27, 2019
@kornelski kornelski reopened this Jan 27, 2019
@andrews05
Copy link
Collaborator

andrews05 commented Oct 10, 2023

In addition to the improvements made in #158 above, there have been further improvements in #482 as optimisation is now performed on the PngImage rather than the PngData. There's probably still some tidy ups that could be done but I think the main concerns here are now resolved. (Correct me if I'm wrong @kornelski)

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-Techdebt Internal changes to improve software maintainability
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants