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

Info refactorings #554

Closed
wants to merge 5 commits into from
Closed

Info refactorings #554

wants to merge 5 commits into from

Conversation

kornelski
Copy link
Contributor

@kornelski kornelski commented Dec 22, 2024

  • There's no need to have ParseChunkData return to reading after reallocating. It wants the whole chunk in memory, and chunk sizes are known ahead of time, so the vec can be allocated without resizing it.
  • Since the chunk vec can have the exact size needed now, Info can mem::take it.
  • Chunks can be parsed lazily, using sBIT as an example.
  • I've added warnings to the StreamingDecoder to collect errors from optional chunks. Unfortunately it can't easily be in Info, because Info supports clone, and DecodingError does not (io::Error can't).
  • Bumped MSRV to 1.65 (99.7% support) to have let else.

@fintelia
Copy link
Contributor

This PR is doing a bunch of different things, and I think would be easier to review as multiple separate PRs.

A few specific points:

  • I'm not totally sold on the warnings concept. Is this something that would be actually used, or just added maintenance burden on our end?
  • I want to think through the lazy parsing a bit more. For something like iCCP, it seems appealing to avoid decompressing the chunk unless requested. If we add a Seek bound, it may may sense to defer even loading the chunk contents unless requested. But sBIT is at most 4 bytes. We already get a bit carried away allocating a whole Vec (with 24 bytes of overhead!) to store it, I'm not sure what we gain by delaying the validity checks

@kornelski kornelski closed this Dec 22, 2024
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.

2 participants