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

proposal: image/png: Add HuffmanOnly compression levels. #59023

Open
HyeockJinKim opened this issue Mar 14, 2023 · 12 comments · May be fixed by #59024
Open

proposal: image/png: Add HuffmanOnly compression levels. #59023

HyeockJinKim opened this issue Mar 14, 2023 · 12 comments · May be fixed by #59024
Labels
Milestone

Comments

@HyeockJinKim
Copy link

HyeockJinKim commented Mar 14, 2023

Problem:

The HuffmanOnly option, which only performs Huffman encoding, is supported by the zlib library, but not by png using zlib.

Solution

Add a HuffmanOnly const to image/png.

Golang developer can pass the HuffmanOnly option with the code below and get a PNG with only huffman encoding.

png.Encoder{
    CompressionLevel: png.HuffmanOnly,
}
@HyeockJinKim HyeockJinKim linked a pull request Mar 14, 2023 that will close this issue
@gopherbot
Copy link

Change https://go.dev/cl/476017 mentions this issue: image/png: Add huffmanOnly compression level to png

@cherrymui cherrymui changed the title image/png: There is a missing zlib compression level in png compression. image/png: missing zlib compression level in png compression Mar 14, 2023
@cherrymui
Copy link
Member

Are you proposing adding a new API to the image/png package? (Adding an exported constant is a new API.) If so, you'll need to follow the proposal process, see https://golang.org/s/proposal . Thanks.

@cherrymui cherrymui added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. FeatureRequest labels Mar 14, 2023
@HyeockJinKim
Copy link
Author

Is it better to edit this issue (#59023) now as a template for a proposal?
Or should I follow the link below to create a new issue as a template for a proposal?
https://github.com/golang/proposal/blob/master/go2-language-changes.md

Thanks for your help.

@ianlancetaylor
Copy link
Contributor

@HyeockJinKim Either way is fine. Thanks.

@HyeockJinKim HyeockJinKim changed the title image/png: missing zlib compression level in png compression proposal: Go 2: Add HuffmanOnly compression levels to image/png. Mar 16, 2023
@gopherbot gopherbot added this to the Proposal milestone Mar 16, 2023
@HyeockJinKim
Copy link
Author

I've created it according to the proposal template. Can you review it? @ianlancetaylor

@HyeockJinKim
Copy link
Author

It would also be nice to work on supporting all zlib compression levels from 1-9, would you mind if I put that in the proposal as well?

@ianlancetaylor
Copy link
Contributor

Sure, you can update the proposal. But you used the wrong template. You used the one for a language or incompatible library change (Go 2) not the one for a simple additional proposal. May be easier to start a new issue at this point. Thanks.

@HyeockJinKim HyeockJinKim changed the title proposal: Go 2: Add HuffmanOnly compression levels to image/png. proposal: image/png: Add HuffmanOnly compression levels. Mar 18, 2023
@HyeockJinKim
Copy link
Author

I've rewritten it, please review it again. 😢

@ianlancetaylor ianlancetaylor removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. FeatureRequest labels Mar 19, 2023
@ianlancetaylor
Copy link
Contributor

CC @nigeltao

@nigeltao
Copy link
Contributor

Adding const HuffmanOnly CompressionLevel = -4 to package png LGTM.

Optionally, if you also want to implement this (below) in image/png/writer.go, then that LGTM too.

    // Positive CompressionLevel values are reserved to mean a numeric zlib
    // compression level, although that is not implemented yet.

In hindsight, it would have been nice if the numeric values were consistent (and, perhaps, that 0 meant "default"). But it's too late to change existing constants.

@HyeockJinKim
Copy link
Author

Shall I add to CL to receive compression levels of 1-9?

@nigeltao
Copy link
Contributor

I'm OK with that, but I'd also wait until there's a formal response to the proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

Successfully merging a pull request may close this issue.

5 participants