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

"truncated input" API change seems like a mess #101

Open
1 of 2 tasks
pjanx opened this issue Mar 7, 2023 · 3 comments
Open
1 of 2 tasks

"truncated input" API change seems like a mess #101

pjanx opened this issue Mar 7, 2023 · 3 comments

Comments

@pjanx
Copy link
Contributor

pjanx commented Mar 7, 2023

  • The PNG decoder still loops around "short read" in one place.
  • DecodeImage0 doesn't reflect this change (no DecodeImage_UnexpectedEndOfFile as far as I can tell).
  • All decoders have their own version of this error, is that necessary?
@nigeltao
Copy link
Collaborator

nigeltao commented Mar 8, 2023

The PNG decoder still loops around "short read" in one place.

Can you elaborate on (1) where is this one place and (2) why this is a problem?

If the answer to (1) is in decoder.do_decode_frame line 1248 then decoder.decode_frame should 'catch' it and turn it into "truncated input".

DecodeImage0 doesn't reflect this change (no DecodeImage_UnexpectedEndOfFile as far as I can tell).

Yeah, DecodeImage_UnexpectedEndOfFile ought to be unreachable now. We could probably remove it in v0.4.

All decoders have their own version of this error, is that necessary?

Go isn't Wuffs, but in the Go standard library, we returned a common error (io.ErrUnexpectedEOF) instead of each image decoder having their own version of this error. This was apparently confusing / surprising, so it's the other way round in Wuffs.

@pjanx
Copy link
Contributor Author

pjanx commented Mar 8, 2023

Can you elaborate on (1) where is this one place and (2) why this is a problem?

https://github.com/google/wuffs/blob/main/std/png/decode_png.wuffs#L1237

Inconsistency. I haven't retested what actually happens, I've just reread the code.

This was apparently confusing / surprising, so it's the other way round in Wuffs.

This particular argument doesn't convince me. The thread is about people trying to receive as much of an image as they can, and dealing with the possibility of receiving an error (warning) at the same time.

@nigeltao
Copy link
Collaborator

https://github.com/google/wuffs/blob/main/std/png/decode_png.wuffs#L1237

OK that (line 1237) rolls into line 1248 that I referenced above. I suppose we could make line 1248 return "#truncated input" directly, which would be a little cleaner. As I said, though, decoder.decode_frame should already 'catch' "$short read", so it shouldn't make a difference, in practice.

This particular argument doesn't convince me.

If you're not convinced then we'll agree to disagree. I'm just telling you why it's the way it is.

The thread is about people trying to receive as much of an image as they can, and dealing with the possibility of receiving an error (warning) at the same time.

There's two concerns in that thread. One is about trying to receive as much of an image as they can, as you said. Two is about what error you get when it's truncated (a generic io error versus an image-format-specific jpeg error) which is relevant to your "All decoders have their own version of this error, is that necessary?" in the OP. A quote from that thread: "How do I know that io.ErrUnexpectedEOF has anything to do with "wrong" JPEGs?"

nigeltao added a commit that referenced this issue Mar 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants