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

Fix and update code #33

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Fix and update code #33

wants to merge 9 commits into from

Conversation

bluddy
Copy link
Contributor

@bluddy bluddy commented Jan 22, 2020

  • Update to work with decompress 1.0.0
  • Tested by reading and writing the same PNG file.
  • Simplified: since we already have an input/output abstraction, there's no need to functorize the PNG writer. Make it symmetric with reader.
  • Fix bug in writer: close wasn't being called on the channel, leading to 0 size output files.

Make writer symmetric with reader.
Get rid of unnecessary functorization (we already abstract with chunk
reader/writer)
(Would be great to provide as option for high performance)
@bluddy
Copy link
Contributor Author

bluddy commented Mar 27, 2020

Can I have some feedback on this PR?

@cfcs
Copy link
Collaborator

cfcs commented Mar 27, 2020

I believe these are already covered by my gigantic unmerged GIF PR (perhaps modulo the write) which also changes various abstractions in order to support animated GIFs / progressive rendering, and that is why I don't feel like merging this since it'd be a painful rebase.

I don't know how @rlepigre feels, and to be clear I'm not opposing this PR, just don't feel motivated to deal with it myself. :(

@rlepigre
Copy link
Owner

I had a quick look and this looks good to me. I'd be happy to merge this after having a closer look, but I don't want to make @cfcs's like difficult with #24. Moreover, I really don't have much time to work on this project currently, and so I really don't want to make any promise.

@cfcs
Copy link
Collaborator

cfcs commented Mar 27, 2020

@rlepigre & @bluddy: How would you two feel about if I:

  • cherry-pick the relevant changes from @bluddy's present PR into the GIF PR
  • since the GIF PR is still not quite production quality, make sure the ImageLib_unix module still calls out to imagemagick (edit: for GIF) to try to keep existing users happy
  • update + merge the GIF PR
  • then cut an opam release

@rlepigre
Copy link
Owner

Sounds good to me! Thanks!

@cfcs
Copy link
Collaborator

cfcs commented Apr 10, 2020

I haven't forgotten about this, having trouble upgrading to decompress.1.1.0, I will speak to @dinosaure about it.

@yomimono
Copy link
Contributor

As a big fan of the gif_prelim branch, let me know if I can help out.

@dinosaure dinosaure mentioned this pull request May 14, 2020
@cfcs cfcs mentioned this pull request May 17, 2020
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.

4 participants