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: Decode drops interfaces #20851

Open
carl-mastrangelo opened this Issue Jun 29, 2017 · 7 comments

Comments

Projects
None yet
5 participants
@carl-mastrangelo
Member

carl-mastrangelo commented Jun 29, 2017

What version of Go are you using (go version)?

1.8

Background: I am trying to register an image decoder for Webm (to make thumbnails). Commonly the reader passed to image.Decode() is an *os.File or a multipart.File.

What did you see instead?

The Reader passed to the decoder function (register with image.RegisterFormat) is almost never the same one as was passed to image.Decode. It is wrapped in a reader which also implements Peek (for looking at the first few bytes without accidentally consuming them.

The problem is that wrapping in this type drops all the other interfaces of the reader, which may be useful to the Decoder. For example, in my case, the image decoding will be delegated to an executable. It would be better to pass the file name to the subprocess rather than streaming it over stdin. The interface{Name() string} interface of *os.File, and the io.ReaderAt and io.Seeker shared by *os.File and multipart.File cannot be accessed.

It would be nice if the image package forwarded commonly used methods by optionally implementing them if the source reader did. For example:

type readernamer interface {
  reader
  Name() string
} 

which the image package would pass instead. I realize that would result in 2^n interfaces, but I don't see a way around it without exposing the original reader.

One thing worth noting: The peeker interface is unnecessary if the input reader implements Seeker. Peek can be implemented with Read and Seek

@bradfitz bradfitz added this to the Go1.10 milestone Jun 29, 2017

@gopherbot

This comment has been minimized.

gopherbot commented Jun 29, 2017

CL https://golang.org/cl/47255 mentions this issue.

@bradfitz bradfitz modified the milestones: Go1.10, Go1.11 Nov 15, 2017

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 15, 2017

My comment from the CL:

Seems like this adds a secret (undocumented) API & promise to the package, about readers with a Peek(int) ([]byte, error) method.

If we go ahead with your CL, we should at least document it.

/cc @nigeltao

@bradfitz bradfitz modified the milestones: Go1.11, Go1.12 Jun 20, 2018

@carl-mastrangelo

This comment has been minimized.

Member

carl-mastrangelo commented Nov 3, 2018

Friendly ping on this. If we document that ReaderAt and PeekReader's are passed through directly, would that be enough? It would certainly help in the case that the reader is *os.File, bytes.Reader, or multipart.File, which should covert common inputs.

@nigeltao

This comment has been minimized.

Contributor

nigeltao commented Nov 7, 2018

I'm a little confused. The OP talked about adding an interface for the Name method, and forwarding that on. But https://golang.org/cl/47255 doesn't do anything for Name. Are you now no longer proposing the readername interface (or, in general, 2^n new interfaces)?

@carl-mastrangelo

This comment has been minimized.

Member

carl-mastrangelo commented Nov 7, 2018

@nigeltao Right, I am not proposing the 2^n interfaces (maybe in Go 2 this can be fixed). Instead, I would like the original reader passed through unwrapped whenever possible. This allows punning the type more easily.

As a recent example of when this affected me, I made an ImageMagick Decoder for the Go image library. Imagick likes reading from the File directly, which means wrapping the *os.File in a readPeeker forces an extra copy when passing it to Imagick.

The CL I sent works around the need to call Peek(), if the Reader is also a ReaderAt.

@rsc

This comment has been minimized.

Contributor

rsc commented Nov 28, 2018

Too late for Go 1.12, but @nigeltao, any thoughts about the proposal embodied by the CL that @carl-mastrangelo pointed at? We could still decide now what to do for Go 1.13.

@rsc rsc modified the milestones: Go1.12, Go1.13 Nov 28, 2018

@rsc rsc added the early-in-cycle label Nov 28, 2018

@nigeltao

This comment has been minimized.

Contributor

nigeltao commented Nov 29, 2018

@rsc The CL (https://golang.org/cl/47255) no longer needs new interfaces as this original issue suggested, since it can type assert for the Peek or Seek methods. I think that @carl-mastrangelo can solve the underlying problem without any API changes.

In terms of the cl/47255 code review, the ball is in @carl-mastrangelo's court.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment