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

image: Make image encoding methods consistent across JPG, GIF, and PNG #12572

Closed
jimdoescode opened this issue Sep 10, 2015 · 6 comments
Closed

image: Make image encoding methods consistent across JPG, GIF, and PNG #12572

jimdoescode opened this issue Sep 10, 2015 · 6 comments

Comments

@jimdoescode
Copy link

@jimdoescode jimdoescode commented Sep 10, 2015

Greetings.

As far as I can tell, Go currently supports encoding an image.Image into 3 image file formats; JPEG, GIF and PNG.

Each format has its own encode method. Both jpeg.Encode and gif.Encode are fairly similar but with different Options structs (jpeg.Options, gif.Options). However png.Encode does not have an Options requirement in its method signature. png.Encode actually uses a separate struct, png.Encoder, which has an Encode method on it, so png.Encode is really just calling png.Encoder.Encode.

Problem
There is no uniform way to encode an image.Image. Also encoding is done in the various sub-packages (image/jpeg, image/gif, image/png) while decoding is done in the image package, so two inverse functions don't exist on the same package levels.

Proposal
I propose that the GIF and JPEG packages be done the same way as PNG, and that an interface be created; image.FormatEncoder, which would wrap all of the _.Encoder.Encode methods. Then an image.Encode(w io.Writer, m Image, enc FormatEncoder) error function be added. See below for my suggested changes to the image packages.

//package "image/jpeg"
type Encoder struct {
    Quality int
}

func (enc *Encoder) Encode(w io.Writer, m Image) error {
    //Do encoding
}
//package "image/gif"
type Encoder struct {
    NumColors int
    Quantizer draw.Quantizer
    Drawer draw.Drawer
}

func (enc *Encoder) Encode(w io.Writer, m Image) error {
    //Do encoding
}
//package "image"
type FormatEncoder interface {
    Encode(io.Writer, Image) error
}

func Encode(w io.Writer, m Image, enc *FormatEncoder) error {
    return enc.Encode(w, m)
}

Notes
If you look at the _.Encoder structs for gif and jpeg, they have the same properties as the old _.Options structs used by the old Encode functions.

Consequences
png.Encode, gif.Encode, gif.Options, jpeg.Encode, and jpep.Options would become deprecated in favor of using image.Encode and passing in the relevant encoder struct.

Any explicit encoding would need to invoke the relevant Encoder struct to call the Encode method on it (it's not a single function call anymore).

Next Steps
I have most of these changes already finished. I need to write a few tests to verify things work as expected, and wanted to get sign off before proceeding further.

@ianlancetaylor ianlancetaylor changed the title Make image encoding methods consistent across JPG, GIF, and PNG image: Make image encoding methods consistent across JPG, GIF, and PNG Sep 10, 2015
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 10, 2015

Loading

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Sep 10, 2015
@adg
Copy link
Contributor

@adg adg commented Sep 10, 2015

Note that we can't deprecate the existing APIs. They're protected by our compatibility promise. So these new APIs would need to be introduced alongside the existing ones.

There is no uniform way to encode an image.Image.

To me this doesn't seem like sufficient justification for adding more API surface here (which could be more confusing than anything else). People that want a general image encoding interface can write the few lines of code to provide one. But I'd be interested to hear what @nigeltao thinks.

Loading

@nigeltao
Copy link
Contributor

@nigeltao nigeltao commented Sep 11, 2015

I can certainly see the appeal, and I'm not ruling it out, but I'm not overly excited about uniform ways to encode.

The image package (not the image/foo packages) has a Decode function, and the RegisterFormat mechanism, because it's common to want to a decode an image, whether a file on disk or downloaded from the network, without knowing what exact format it is beforehand.

But for encoding, in my experience, you usually know exactly what format to encode in, because there's usually only one. Or if you really need to encode multiple formats, it's not hard to write this in your package main:

if encodePNG {
png.Encode(etc)
} else {
jpeg.Encode(etc)
}

Also, as adg said, if you really want a FormatEncoder interface, it's very easy to do so in your own code, outside of the image packages.

Sure, in hindsight, it might have been nice if the image/* packages' encoding APIs were made consistent before we froze for Go 1.0, but at this point, I don't think there's much net benefit in explicitly deprecating something now.

If we were to introduce a new type, I'd call it image.Encoder instead of image.FormatEncoder, but there's also a related idea of introducing an image.Decoder type or an image.DecodeImage(io.Reader, *image.DecodeImageOptions) function to add options to the decoding process, such as specifying a destination buffer, or a maximum decoded size. There is more discussion on issue #5050. Perhaps there's a larger problem to be solved.

BTW, if you're not already aware of them, there's also BMP, WEBP and TIFF codecs under golang.org/x/image.

Loading

@jimdoescode
Copy link
Author

@jimdoescode jimdoescode commented Sep 11, 2015

Hmm the compatibility promise says "It is intended that programs written to the Go 1 specification will continue to compile and run correctly, unchanged, over the lifetime of that specification." It makes no mention of deprecating functionality, which would still maintain backwards compatibility and thus the promise. PHP is a good example of similar behavior, granted they do remove deprecated functionality after awhile (read several years). I'd assume the same would happen when the Go 2 spec arrives, whenever that is :P

It seems the larger issue to be solved is how do you work with a frozen specification if you're not willing to deprecate things. I'm not sure how to answer that and until it gets sorted out I guess this change is a non-starter.

@nigeltao I was aware of those additional codecs but chose not to address them since they aren't part of core. I suspect it wouldn't be hard to make them match the interface I defined though.

Loading

@nigeltao
Copy link
Contributor

@nigeltao nigeltao commented Sep 11, 2015

I'm not saying that we can't deprecate the old functions. I'm just saying that, I can appreciate the motivation, but I think that the benefit is marginal.

Loading

@jimdoescode
Copy link
Author

@jimdoescode jimdoescode commented Sep 11, 2015

Alright

Loading

@golang golang locked and limited conversation to collaborators Sep 22, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants