Issue 129190043: code review 129190043: png: make the encoder configurable
|
Created:
9 years, 8 months ago by jeff.allen Modified:
9 years, 8 months ago CC:
nigeltao, ruiu, golang-codereviews Visibility:
Public. |
Descriptionpng: make the encoder configurable
In order to support different compression levels, make the
encoder type public, and add an Encoder method to it.
Fixes issue 8499.
Patch Set 1 #Patch Set 2 : diff -r 2cc71cd8fe3e466a2338b1fdf8053931d47a9e31 https://code.google.com/p/go #Patch Set 3 : diff -r 2cc71cd8fe3e466a2338b1fdf8053931d47a9e31 https://code.google.com/p/go #Patch Set 4 : diff -r 2cc71cd8fe3e466a2338b1fdf8053931d47a9e31 https://code.google.com/p/go #
Total comments: 19
Patch Set 5 : diff -r 59b1bb4bf0458d3e31aaff083c5381c9906f875c https://go.googlecode.com/hg/ #
Total comments: 1
Patch Set 6 : diff -r 07a722aece2db54bb4cccc58350b8657afbcdc35 https://go.googlecode.com/hg/ #MessagesTotal messages: 16
Hello Nigel, Please take a look at this API change and see if it would be acceptable for Go 1.4. Thanks, -jeff
Sign in to reply to this message.
Hi Nigel, Please take a look at this API change and see if it would be acceptable for 1.4. -jeff
Sign in to reply to this message.
FYI, the usual process is to run "hg mail", which I believe automatically CC's the golang-codereviews list. https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer.go File src/pkg/image/png/writer.go (right): https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer.... src/pkg/image/png/writer.go:17: // An Encoder holds the configuration of an encoding task. // Encoder configures encoding PNG images. https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer.... src/pkg/image/png/writer.go:19: Level CompressionLevel // The desired compression level. I'd change the first "Level" to "CompressionLevel", and then delete the redundant comment. https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer.... src/pkg/image/png/writer.go:23: cfg *Encoder I'd spell "cfg" as "config". https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer.... src/pkg/image/png/writer.go:448: // Encoder.Level to map to zlib.DefaultCompression. s/Level/CompressionLevel/ https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer.... src/pkg/image/png/writer.go:466: // Encode writes the Image m to w in PNG format, using the default I'd drop the "using the default configuration" as I think it's implied, and also there is no type called "configuration". https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer.... src/pkg/image/png/writer.go:474: // Encode writes the Image m to w in PNG format. Any Image may be encoded, but Since the comment has been copy/pasted above, I'd just write here: // Encode writes the Image m to w in PNG format. https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer.... src/pkg/image/png/writer.go:476: func (cfg *Encoder) Encode(w io.Writer, m image.Image) error { s/cfg/enc/ seems more sensible. https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer_... File src/pkg/image/png/writer_test.go (right): https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer_... src/pkg/image/png/writer_test.go:91: err := e1.Encode(&b1, m) I'd write if err := e1.Encode(&b1, m); err != nil { https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer_... src/pkg/image/png/writer_test.go:103: t.Error("no compression case is not small enough") t.Error("DefaultCompression encoding was larger than NoCompression encoding")
Sign in to reply to this message.
https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer.go File src/pkg/image/png/writer.go (right): https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer.... src/pkg/image/png/writer.go:40: ) I'd probably make these constants to be of type int and to have the same values as http://golang.org/pkg/compress/flate/#pkg-constants. Then you can remove levelToZlib function entirely because you can directly pass the compression level value to zlib.NewWriterLevel. It would also allow you to pass a non-listed compress level, such as 3 or 8. At least the current constants feels counter intuitive to me. (1 for NoCompress and 2 for BestSpeed?)
Sign in to reply to this message.
A broader question is whether or not the "config" value approach is better than just defining a new function: func EncodeLevel(w io.Writer, m image.Image, level int) error If there are many configurable things, the config value approach might be better, but here we only have one parameter.
Sign in to reply to this message.
On 2014/08/22 21:39:24, ruiu wrote:
> A broader question is whether or not the "config" value
> approach is better than just defining a new function:
>
> func EncodeLevel(w io.Writer, m image.Image, level int) error
>
> If there are many configurable things, the config value
> approach might be better, but here we only have one parameter.
I don't expect more paramters, but the Encoder type does allow for them. Another
thing with the Encoder type is that it satisfies interface{ Encode(io.Writer,
image.Image) }.
In hindsight, I would have done image/jpeg this way, instead of the jpeg.Options
type, but it's too late to change that. I'm OK with the current design of this
CL, but if others disagree then I'm willing to be persuaded otherwise.
Sign in to reply to this message.
https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer.go File src/pkg/image/png/writer.go (right): https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer.... src/pkg/image/png/writer.go:40: ) On 2014/08/22 21:25:27, ruiu wrote: > I'd probably make these constants to be of type int and to have the same values > as http://golang.org/pkg/compress/flate/#pkg-constants. Having DefaultCompression be zero has the nice property that, if the Encoder struct ever gains other fields, you can still initialize it as Encoder{ OtherField: foo }, and still use default compression. Yes, the compress/flate constants are different, to be the same as the C zlib API. In hindsight, I would have picked the more Go-like "0 means default" for Go, instead of "-1 means default; 0 means no compression", but it's too late to change that now.
Sign in to reply to this message.
> Having DefaultCompression be zero has the nice property that, if the Encoder
> struct ever gains other fields, you can still initialize it as
> Encoder{ OtherField: foo }, and still use default compression.
>
> Yes, the compress/flate constants are different, to be the same as the C zlib
> API. In hindsight, I would have picked the more Go-like "0 means default" for
> Go, instead of "-1 means default; 0 means no compression", but it's too late
to
> change that now.
Thank you for the explanation. Although it's not a strong opinion, I feel I'd
still prefer EncodeLevel over the config value since the former is (in my
opinion) slightly easier to use, and allows me to pass any compression level
between 0 - 9. For EncodeLevel the second argument is mandatory, so it has no
problem like the one that you described. I'm not sure how important it is
though. I'm clearly not the person to make a call. Please go ahead if it seems
okay to you guys.
Sign in to reply to this message.
PTAL https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer.go File src/pkg/image/png/writer.go (right): https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer.... src/pkg/image/png/writer.go:17: // An Encoder holds the configuration of an encoding task. On 2014/08/20 08:36:58, nigeltao wrote: > // Encoder configures encoding PNG images. Done. https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer.... src/pkg/image/png/writer.go:19: Level CompressionLevel // The desired compression level. On 2014/08/20 08:36:58, nigeltao wrote: > I'd change the first "Level" to "CompressionLevel", and then delete the > redundant comment. Done. https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer.... src/pkg/image/png/writer.go:23: cfg *Encoder On 2014/08/20 08:36:58, nigeltao wrote: > I'd spell "cfg" as "config". Done. https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer.... src/pkg/image/png/writer.go:448: // Encoder.Level to map to zlib.DefaultCompression. On 2014/08/20 08:36:58, nigeltao wrote: > s/Level/CompressionLevel/ Done. https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer.... src/pkg/image/png/writer.go:466: // Encode writes the Image m to w in PNG format, using the default On 2014/08/20 08:36:58, nigeltao wrote: > I'd drop the "using the default configuration" as I think it's implied, and also > there is no type called "configuration". Done. https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer.... src/pkg/image/png/writer.go:476: func (cfg *Encoder) Encode(w io.Writer, m image.Image) error { On 2014/08/20 08:36:58, nigeltao wrote: > s/cfg/enc/ seems more sensible. Done. https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer_... File src/pkg/image/png/writer_test.go (right): https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer_... src/pkg/image/png/writer_test.go:91: err := e1.Encode(&b1, m) On 2014/08/20 08:36:58, nigeltao wrote: > I'd write > > if err := e1.Encode(&b1, m); err != nil { Done. https://codereview.appspot.com/129190043/diff/60001/src/pkg/image/png/writer_... src/pkg/image/png/writer_test.go:103: t.Error("no compression case is not small enough") On 2014/08/20 08:36:58, nigeltao wrote: > t.Error("DefaultCompression encoding was larger than NoCompression encoding") Done.
Sign in to reply to this message.
https://codereview.appspot.com/129190043/diff/80001/src/pkg/image/png/writer_... File src/pkg/image/png/writer_test.go (right): https://codereview.appspot.com/129190043/diff/80001/src/pkg/image/png/writer_... src/pkg/image/png/writer_test.go:37: func encodeDecode(m image.Image) (out image.Image, err error) { I actually prefer how it was before. If you were going to change anything, I'd change: m, err = Decode(&b) etc to just return Decode(&b)
Sign in to reply to this message.
Hello nigeltao@golang.org, ruiu@google.com (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
LGTM.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=1b4900565b48 *** png: make the encoder configurable In order to support different compression levels, make the encoder type public, and add an Encoder method to it. Fixes issue 8499. LGTM=nigeltao R=nigeltao, ruiu CC=golang-codereviews https://codereview.appspot.com/129190043 Committer: Nigel Tao <nigeltao@golang.org>
Sign in to reply to this message.
This CL appears to have broken the windows-386 builder. See http://build.golang.org/log/3460428afab9902de33162dd875326783892dfcf
Sign in to reply to this message.
Maybe the constants should be negative (except Default still being zero), reserving positive number for literals in the future specifying a numeric level. On Wed, Aug 27, 2014 at 10:50 PM, <nigeltao@golang.org> wrote: > *** Submitted as > https://code.google.com/p/go/source/detail?r=1b4900565b48 *** > > png: make the encoder configurable > > In order to support different compression levels, make the > encoder type public, and add an Encoder method to it. > > Fixes issue 8499. > > LGTM=nigeltao > R=nigeltao, ruiu > CC=golang-codereviews > https://codereview.appspot.com/129190043 > > Committer: Nigel Tao <nigeltao@golang.org> > > > > https://codereview.appspot.com/129190043/ > > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. >
Sign in to reply to this message.
On Fri, Aug 29, 2014 at 12:52 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > Maybe the constants should be negative (except Default still being zero), > reserving positive number for literals in the future specifying a numeric > level. https://codereview.appspot.com/138860043
Sign in to reply to this message.
|
