Open Shnatsel opened 2 months ago
Another possible item would be adding BufRead
and/or Seek
bounds for the decoder. The image crate already requires them, so no further breakage would be required there.
BufRead
: https://github.com/image-rs/image-png/discussions/416#discussioncomment-8088740Seek
: https://github.com/image-rs/image-png/issues/510, though it could also be helpful for removing the need for PngDecoder::with_limits
in the image crate.Do we want to merge https://github.com/image-rs/image-png/pull/421? It seems the consensus is to go with https://github.com/image-rs/image-png/pull/493 instead, but we need to commit to one of these options.
I am fine with either approach.
I think that https://github.com/image-rs/image-png/pull/421 results in a slightly cleaner API (no read_...
vs next_...
discrepancy; can remove Row
and InterlacedRow
structs). OTOH, https://github.com/image-rs/image-png/pull/493 may still be better overall:
png
crate helps with buffer management. (e.g. it was easier to start SkPngRustCodec
because of this)I think that we agree that these changes will be beneficial for performance. OTOH, maybe we can/want to wait to measure the actual impact in Chromium usage (especially since avoiding the extra copy also requires some Chromium-side work). I am totally fine with waiting (the new benchmark results would hopefully come before the end of 2024; nothing is blocked on my side without these PRs - I can always patch them in as long as there is a consensus that one of them will get merged eventually).
We'll want to get these changes done for the semver-breaking v0.18 release:
503
400 (already has a duplicate now: #516 )
Open questions:
522