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

feat: remove Camlimages and migrate to raw C io #47

Merged
merged 14 commits into from
Jul 31, 2021
Merged

feat: remove Camlimages and migrate to raw C io #47

merged 14 commits into from
Jul 31, 2021

Conversation

eWert-Online
Copy link
Collaborator

@eWert-Online eWert-Online commented Jun 27, 2021

This PR is the work for removing camlimages and replacing it with linked in libraries and handwritten bindings.
Once this is done, we should not require any global library to run odiff.

One concern is, that the size of the binary and npm package will get too big, so I will keep track of the sizes at each step:

TODO size of binary
✅ PNG (main branch) 2.038.600 Byte
✅ JPG / JPEG 1.930.937 Byte
✅ BMP 1.935.285 Byte
✅ TIFF 1.941.285 Byte
XPM ??? Byte
  • Windows support
  • Linux support

@eWert-Online
Copy link
Collaborator Author

@dmtrKovalenko
I saw, that xpm needs libX11 as a dependency, which itself has a ton of other dependencies.
(I did once go down the rabbit hole to package all of its dependencies for esy and ended up with something like 12 packages)

So xpm support would need a ton of work (if at all possible) to be statically linked inside odiff.
And when its done, it will increase the binary size by a lot!

I did nearly finish bmp support locally and tiff should also be no problem.

My question would be:
Do we have a good reason to support xpm or could it be considered to just drop support for it with this PR?
As it also seems to be only used inside the X window system. So it isn't really a commonly used image format.

@dmtrKovalenko
Copy link
Owner

I took an xpm because there was no-effort support by camlimages. I think we can drop it for free.

Ideally in the future we should support avif

@eWert-Online
Copy link
Collaborator Author

Thats great!
Yeah, I was also thinking that AVIF and maybe WebP would be great additions.
Implementing it should also be not a big problem.

...leaving this here for future reference 😄
AVIF: https://github.com/AOMediaCodec/libavif
WebP: https://github.com/webmproject/libwebp

@eWert-Online eWert-Online marked this pull request as ready for review July 2, 2021 17:23
@eWert-Online eWert-Online changed the title WIP: Remove Camlimages and rewrite IO Remove Camlimages and rewrite IO Jul 2, 2021
@eWert-Online
Copy link
Collaborator Author

🎉🎉🎉 Its working on Windows!!!

@dmtrKovalenko
Copy link
Owner

You rock Torben!
image

That is perfect! I'll take a look on the code a little bit later and I will take a look on how we can easily produce 2 different binaries for release with and without included cross format

@eWert-Online
Copy link
Collaborator Author

Thank you 😄
Just so there is no misunderstanding, we are now even smaller in binary size, than we were with camlimages.
I am just wondering a bit, why you want to produce two releases?
Just so people who only use png will have an even smaller binary? 🙂

@eWert-Online
Copy link
Collaborator Author

I added tests for every IO type we currently have.
Do we still need the test.c, test.h and test.h.gch files in the root of the project?

@dmtrKovalenko
Copy link
Owner

We can remove them

@eWert-Online
Copy link
Collaborator Author

I didn't notice, that the tests were failing with "Out of memory" on windows ci. 😄

Weirdly enough, switching from caml_ba_alloc to caml_ba_alloc_dims did fix the error.
The only reason I can think of is a conversion error, which lead to windows wanting to allocate a bigarray which was way bigger than intended. With caml_ba_alloc_dims all conversion warnings are gone (we actually have 0 warnings now on all platforms) and everything works as expected.

Copy link
Owner

@dmtrKovalenko dmtrKovalenko left a comment

Choose a reason for hiding this comment

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

Thank you. Sorry I was so overwhelmed with work and finally get some time to test this and thank you for this awesome stuff.

@dmtrKovalenko dmtrKovalenko changed the title Remove Camlimages and rewrite IO feat: remove Camlimages and migrate to raw C io Jul 31, 2021
@dmtrKovalenko dmtrKovalenko merged commit 2faf582 into main Jul 31, 2021
@dmtrKovalenko dmtrKovalenko deleted the fix-41 branch July 31, 2021 08:12
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