Skip to content

Support ICO decoding mid stream#2895

Merged
197g merged 4 commits intoimage-rs:mainfrom
RunDevelopment:ico-start-0
Apr 4, 2026
Merged

Support ICO decoding mid stream#2895
197g merged 4 commits intoimage-rs:mainfrom
RunDevelopment:ico-start-0

Conversation

@RunDevelopment
Copy link
Copy Markdown
Member

@RunDevelopment RunDevelopment commented Apr 1, 2026

Based on this comment, I investigated which decoders assume that they start reading the reader at stream position 0. I found that ICO, BMP, and TIFF make that assumption and fail to decode (or decode incorrectly) when the assumption is broken.

In this PR, I fixed ICO. I did this by adjusting the code to support arbitrary starting positions for the reader.

However, I am not sure if this is the best approach. It adds non-trivial complexity. Looking at TIFF, the code making the assumption may not even be part of image. So I was also considering another approach: Make the assumption true. We could make a thin wrapper around the reader which offsets the stream position such that decoders start at their zero. This has the advantage that (almost) no code changes to decoders are necessary, which makes it trivial to support decoders implemented elsewhere.

@197g
Copy link
Copy Markdown
Member

197g commented Apr 1, 2026

Yes, I'm aware of the TIFF case. Currently working on that but due to the ability to have a wrapper outside and in particular for a read_at-based file this being trivial I don't think it's a priority bug for anyone. I could be convinced otherwise by seeing that in the wild. We should definitely fix this but not at considerable cost.

@RunDevelopment
Copy link
Copy Markdown
Member Author

We should definitely fix this but not at considerable cost.

Agreed.

I think I'll implement the wrapper-around-the-reader approach. The wrapper type can be reused across implementations, so this should be less complexity over all.

@fintelia
Copy link
Copy Markdown
Contributor

fintelia commented Apr 2, 2026

Another option is to just return an error at the start if the stream position isn't zero

@RunDevelopment
Copy link
Copy Markdown
Member Author

It's an option if you document this behavior for all relevant functions and types (prob missed a few). This should of course include an up-to-date list of all formats with this restriction, an explanation of it, and how to work around it.

Seems to me that it's easier to fix it and add a test to ensure everything's correct.

Copy link
Copy Markdown
Member

@197g 197g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do believe that the wrapper is the right direction. Particularly reviewing the implementation it seems simple enough to provide it. Specifically because there are a few comments that I'd like to have changed; but they were easy enough to spot. Can you add comments indicating some of the reasoning for the implementation while doing that?

The only weirdness is that formats that are positioned will always go through the type even if their offset is 0. This is more of a maintenance/performance perspective, Rust will only add transparently defaulted methods I assume but still explicit support for new methods requires modifying the wrapper.

@RunDevelopment
Copy link
Copy Markdown
Member Author

Alright, I implement the offset-tracking approach.


Note for merger: All 3 approaches I implemented so far are their own commit, so please squash & merge when merging. (I did this so we can easily switch between different approaches.)

@197g
Copy link
Copy Markdown
Member

197g commented Apr 4, 2026

Sure, I'm okay with either approach. I'm going to add it to the list of reasons against adding the reader to the decoder structs, especially in the dependent crates. The more years of Rust I spend writing the less appealing it is that the ecosystem seems to have converged on this approach (or did it ever do anything else? Especially for deflate and so on, providing new forms of Read was on-vogue very early on with lots of grief when finally async runtimes diverged into different traits).

@197g 197g merged commit f953f6a into image-rs:main Apr 4, 2026
31 checks passed
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

Successfully merging this pull request may close these issues.

3 participants