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

Tracking issue for semver-breaking v0.18 #508

Open
Shnatsel opened this issue Sep 27, 2024 · 2 comments
Open

Tracking issue for semver-breaking v0.18 #508

Shnatsel opened this issue Sep 27, 2024 · 2 comments

Comments

@Shnatsel
Copy link
Contributor

Shnatsel commented Sep 27, 2024

We'll want to get these changes done for the semver-breaking v0.18 release:

Open questions:

@fintelia
Copy link
Contributor

fintelia commented Sep 28, 2024

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.

@anforowicz
Copy link
Contributor

Do we want to merge #421? It seems the consensus is to go with #493 instead, but we need to commit to one of these options.

I am fine with either approach.

I think that #421 results in a slightly cleaner API (no read_... vs next_... discrepancy; can remove Row and InterlacedRow structs). OTOH, #493 may still be better overall:

  • Some clients may not care about copying - in these cases its nice if png crate helps with buffer management. (e.g. it was easier to start SkPngRustCodec because of this)
  • Avoiding breaking changes is valuable even if it results on a slightly more verbose/clunkier API (and it's not that bad anyway)

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

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

3 participants