sorccu / node-jpeg-turbo

Limited libjpeg-turbo bindings for Node.js.
Other
39 stars 36 forks source link

Windows support + async compress/decompress #5

Closed empee closed 7 years ago

empee commented 8 years ago

Tested on Windows 8.1 (32bit) and Windows 10 (64bit) and it will compile as long as node + yasm + pre-gyp (+ all the requirements for it) are working right. (So it might fix #3 )

Also added async compress/decompress for convenience, didn't update the readme.md for those yet.

Also updated the libjpeg-turbo submodule to 1.4.2

sorccu commented 8 years ago

Wow, that's amazing! Any chance you could try to make the Appveyor integration work as well?

I would like to see a few changes. First, the API methods don't match the filenames right now. Of course that was already the situation, I had planned on adding both async and sync versions to the same file. In this case since everything's separate, it would make sense to rename <method>.cc files to <method>Sync.cc, and the <method>Async.cc files to <method>.cc.

Second, it's kind of a pain to have almost the same code twice. Would you happen to have the knowledge to make the sync API just wrap the async API? That way the binaries would be smaller, and more importantly, only one place would need to be changed.

sorccu commented 8 years ago

If the sync wrapper seems difficult to do, perhaps we should split this PR into two PRs. First Windows support, then async.

empee commented 8 years ago

I already have an idea how to do the async/sync code much cleaner without the need to repeat too much code..

Also about the appveyor, I'll see if I can figure out how to get it working, looked at their documentation and the node.js part should be pretty simple, the yasm.. Well, lets see.

I'll do another PR later when I get more of it sorted out, shouldn't be more than a day or two..

empee commented 8 years ago

Couple of questions / ideas: -Is there a reason why we return bpp and subsampling? User defines format -> they know bpp already and subsampling is an int so it wont tell anyone much (unless you plan to pass it back to compress) -Is options really necessary? What if we defaulted to rgba (what canvas etc. use)?

Dropping the bpp and subsampling return would just make the code a lot more clean and auto selecting format would make it also a bit simpler "pass it data and callback -> receive buffer, width, height, size" or in sync version "call it with data, receive object with buffer, width, height, size"

Also I have now combined sync/async, there's still a bit of repeated code but not as much as there was, mainly dealing with the "how to call decompress/return data to user"

sorccu commented 8 years ago

I wouldn't mind adding default options if none are passed, as long as you're still able to set them if you want. I'm not sure if I get how reducing the number of returned values would clean up the code, since the values are needed in calculations or need to be given to jpeg-turbo anyway. What's the difference between returning 4 values vs 6 values?

empee commented 8 years ago

You'll see.. Has to do with pointers & more pointers.. Don't need to pass bpp & subsampling around if they are just used by the decompress and not returned, just need to pass the (optional) format there and it can figure out the rest itself.

Btw. one question I forgot to ask, why are we returning data if user provides their own buffer? Isn't that kind of redundant since they kind of expect their own buffer to hold the data?

sorccu commented 8 years ago

Behaving the same way in both cases makes it easier to start or stop using a preallocated buffer, since you only need to change pretty much one line and don't have to change how you deal with the return value.

empee commented 8 years ago

I'll start poking the AppVeyor now .. Got other stuff done.. As a bonus I added callback support to bufferSize as well

empee commented 8 years ago

This is getting kind of ridiculous, everything else but x86 node 4.x compiles..

empee commented 8 years ago

There it is.. "All checks have passed" .. The 4.4.5 on x86 fails for some odd reason, works on x64 thou..

sorccu commented 7 years ago

Really sorry that it took so long but the PR has now been merged. Reason being that I splurged on a Parallels and Win 10 Pro license and was actually able to try it out.

I'll be making some more changes after this but overall it was quite flawless.

I'd like to offer you $50 from the OpenSTF donation fund for such an awesome contribution. Please send your PayPal email address to simo @ {the name of that GitHub organization} .io, or if not, feel free to recommend some other method.