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

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

image: Decode drops interfaces #20851

carl-mastrangelo opened this issue Jun 29, 2017 · 7 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@carl-mastrangelo
Copy link
Contributor

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
@bradfitz bradfitz added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 29, 2017
@gopherbot
Copy link

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

@bradfitz bradfitz added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Nov 15, 2017
@bradfitz bradfitz modified the milestones: Go1.10, Go1.11 Nov 15, 2017
@bradfitz
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

@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
Copy link
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 A change that should be done early in the 3 month dev cycle. label Nov 28, 2018
@nigeltao
Copy link
Contributor

@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.

@bradfitz bradfitz modified the milestones: Go1.13, Go1.14 Apr 29, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

5 participants