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

cmake & latest node #13

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

cmake & latest node #13

wants to merge 11 commits into from

Conversation

Julusian
Copy link

@Julusian Julusian commented Jun 2, 2019

This builds off the work of another fork (https://github.com/briskycat/node-tjpeg)

EDIT: This fork is published as @julusian/jpeg-turbo and will be updated until this PR is merged.

Changes:

  • Update libjpeg-turbo to 2.0.2 (Thanks to briskycat)
  • Replace node-gyp with cmake-js (libjpeg-turbo 2 needs cmake) (Thanks to briskycat)
  • Add typescript definitions
  • Update ci to use newer node versions
  • Fix build issues with node 12
  • Replace prebuilt-bindings with prebuild
  • Fixed some segfaults

I have published a build of this as @julusian/jpeg-turbo v0.5.1, so feel free to use that package to do any testing. That is also populated with a set of prebuilds https://github.com/Julusian/node-jpeg-turbo/releases/tag/v0.5.1, so should be quick and easy to use.

CI is testing that this builds in each of node 8, 10 and 12 (except windows which is using 11 instead of 12).
I have tested it myself on Windows 10 (node 10), macos (node 8) and ubuntu (node 8)

@haakonnessjoen
Copy link

The fork tjpeg seemed unstable when tested. But might be that the api has changed?

@Julusian
Copy link
Author

Julusian commented Jun 2, 2019

I found a couple of segfaults in the node bindings, which could be the instability you found?
In particular 1640a19#diff-c3ffe5b9396eb0f8646374fdf35e0b3fL86 was consistently causing a crash if a preallocated buffer was supplied.
And another crash if the source buffer was too short.

@haakonnessjoen
Copy link

Could you do some testing with windows on node 9? If I don’t remeber totatally wrong, companion segfaulted when I tried to upgrade to tjpeg.

@Julusian
Copy link
Author

Julusian commented Jun 2, 2019

sure, I can do that.
Ill write some unit tests too. It wont be hard to write some, there arent many api methods to do it for

@Julusian
Copy link
Author

Julusian commented Jun 2, 2019

@haakonnessjoen There are some unit tests in this now, and I have also done some manual tests for node 9 on windows, and it is working fine.

I intend to use this library myself with my StreamDeck XL, both through companion and my own stuff, so I will happily look into segfaults or bugs if any are found.

@Julusian
Copy link
Author

@sorccu #8 (comment)
Let me know if you need anything from me to help you review this.
The commits are pretty isolated to an area of changes per commit, so reviewing a commit at a time may make it easier.Or I could try and split it up into multiple isolated PRs?
To verify usage is the same, perhaps it would help to run the tests I wrote against 0.4.2.

@koral--
Copy link

koral-- commented Jul 13, 2020

@Julusian OpenSTF maintainer here. Thanks for your work, sorry for no response for a long time and FYI this repository is not planned to be maintained as well as OpenSTF.
However, we plan to replace node-jpeg-turbo with @julusian/jpeg-turbo in its successor - DeviceFarmer.

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.

3 participants