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

libjpeg-turbo for windows #458

Closed
wants to merge 1 commit into from
Closed

Conversation

vweevers
Copy link
Contributor

This PR adds JPEG support to canvas under Windows. See also #427. Instructions for user are simple: install libjpeg-turbo to it's default location.

A batch script tests if the directory exists, so nothing breaks when libjpeg-turbo is not installed.

@scarletsky
Copy link

@vnsavage 👍 This is great, thank you!

@kkoopa
Copy link
Contributor

kkoopa commented Oct 22, 2014

LGTM
This should be merged and the documentation updated.

@kangax
Copy link
Collaborator

kangax commented Feb 20, 2015

@vweevers could you please resubmit?

@vweevers
Copy link
Contributor Author

Rebased the PR.

@kangax
Copy link
Collaborator

kangax commented Feb 22, 2015

/cc @rvagg @TooTallNate

@rvagg
Copy link
Contributor

rvagg commented Feb 23, 2015

Soooo .. libjpeg-turbo isn't just a Windows thing, does it make any sense in enabling support for this on non-Windows too or does gtk make that difficult?

This also needs docs before merging IMO because it introduces another awkward yak to shave on Windows to get this running (already painful enough!).

Also @kangax fwiw I'd take @kkoopa's +1 as meaning it's worthy of merging, he should probably even be a collaborator here, moreso than me at least.

@vweevers
Copy link
Contributor Author

@rvagg this isn't about libjpeg_-turbo_ per se. Only about JPEG support, which other systems already have. I'd rather see a quick merge and then let turbo support for non-windows be a separate issue.

Couple of thoughts about the docs, which I would be happy to update:

  • It's no longer necessary to add C:\GTK\bin to PATH, those instructions can be removed?
  • Most of the other stuff is about node-gyp, isn't it easier to refer to the node-gyp docs for that?

@rvagg
Copy link
Contributor

rvagg commented Feb 23, 2015

I agree it wouldn't be worth holding this up but if turbo really is that much faster then it night be worth supporting for other platforms too. Perhaps someone could ticket this?

Re docs: I'm of two minds on the node-gyp overlap, on one hand it's easier to have a single source of docs on how to do compiles on Windows so pointing to nodr-gyp makes sense but on the other hand a large number of bug reports on native addons I maintain are exactly because people didn't follow the links and install as they are supposed to. People don't read if you give them too much to read (me included). I might leave that for others to judge, perhaps you have some thoughts @kangax?

@kangax
Copy link
Collaborator

kangax commented Feb 25, 2015

I'm indifferent on this

@vweevers
Copy link
Contributor Author

vweevers commented May 4, 2015

I have cleaned up and updated the docs.

@zbjornson
Copy link
Collaborator

@vweevers This works for me, but I'd add a note to the libjpeg-turbo installation section saying (again) that architectures must match (32/64 bit), and that if you install 64 bit, either libjpeg_root in binding.gyp needs to be changed or the installation directory needs to be renamed from C:/libjpeg-turbo64 to C:/libjpeg-turbo.

@vweevers
Copy link
Contributor Author

@zbjornson Good point. I've since learned one can also do node-gyp rebuild --GTK_Root=C:/CustomGTKLocation --libjpeg_root=... This seems cleaner than modifying the binding.gyp. Also, I just noticed the download pages for Windows are dead. Stated reason:

It is expected that people who build installers for GTK+ applications for Windows bundle GTK+ with them.

Hakuna matata, the wiki can directly link to the binaries instead.

I will update the wiki asap, and maybe see if I can add a util script that checks if either C:\libjpeg-turbo or C:\libjpeg-turbo64 exists, based on node's bitness.

@vweevers
Copy link
Contributor Author

Rebased and updated PR to support 64 bit libjpeg-turbo. Can this be merged? It's been a year ;)

@LinusU
Copy link
Collaborator

LinusU commented Nov 21, 2015

Hi @vweevers, sorry for the delay on this, lets make it happen!

Could you please squash the commits into one and rebase on the latest master? I'll promise to review it when it's done.

Am I correct in assuming that the wiki has already been updated with the instructions? It looks like it.

@zbjornson
Copy link
Collaborator

I would love to see this merged...
@LinusU if @vweevers doesn't have time to squash, would you be up for using git rebase -i to do the squash yourself instead? (info)

@vweevers
Copy link
Contributor Author

Thanks for the ping @zbjornson, I forgot about this. I'll rebase in a minute

@fnlctrl
Copy link

fnlctrl commented Aug 2, 2016

so.... why isn't this pr merged?

@chearon
Copy link
Collaborator

chearon commented Aug 2, 2016

I think it's scheduled for version 2, doesn't seem like it would break backwards compatibility though

@LinusU
Copy link
Collaborator

LinusU commented Aug 2, 2016

I would be open to merging this now, as long as it doesn't require anything that wasn't previously required

@zbjornson
Copy link
Collaborator

zbjornson commented Aug 2, 2016

@LinusU yep, this proceeds quietly without JPEG support if libjpeg is not present.

However, I just learned that this doesn't appear to compile with VS2015 because reason. That means if someone has VS2015 and libjpeg installed then they will no longer be able to build.

The fixes provided there:

If you're using the TurboJPEG API, you can link against turbojpeg.dll instead of turbojpeg-static.lib.

No

If you're using the libjpeg API, you can link against jpeg62.dll instead of jpeg-static.lib, provided that your application isn't calling jpeg_stdio_src() or jpeg_stdio_dest().

Those APIs are used

If you need to use jpeg-static.lib or turbojpeg-static.lib, you'll need to either build your application with an older version of Visual C++ or build libjpeg-turbo using Visual C++ 2015.

Boo

Can look at using gnuwin's jpeg instead, (edit) or using libjpegturbo for all platforms.

@LinusU
Copy link
Collaborator

LinusU commented Aug 2, 2016

Ugh, that's a shame...

This is probably a really dump question but doesn't GTK include a JPEG library on windows? I have a vague memory but it's probably wrong...

Can look at using gnuwin's jpeg instead, (edit) or using libjpegturbo for all platforms.

Searching around I think that I'm leaning towards gnuwin's jpeg, as I would prefer to now introduce another dependency on the other platforms...

edit: @zbjornson I will trust your instinct on this since you are running Windows and your other contributions have been stellar ✨

@chearon
Copy link
Collaborator

chearon commented Aug 5, 2016

Since I really wanna get an MSYS build going, I think I just got gyp/VS2015 to compile against the MSYS/MinGW version of libjpeg-turbo. You just have to use pacman -S mingw-w64-x86_64-libjpeg-turbo-1.4.2-2 to get the DLL, but then you have to generate your own .lib for VS by doing all this. The MSYS version of it has its own dependencies too so you have to also copy over those DLLs into the release folder.

You can get all of node-canvas's features (pango, gif, free type, etc) working that way. You end up with more DLLs in the release folder but it's probably better than the GTK.zip people are using now

libjpeg-turbo.zip

@timknip timknip mentioned this pull request Sep 24, 2016
@LinusU
Copy link
Collaborator

LinusU commented May 3, 2017

Ehh, I just merged #841, is this the same thing or is this using another library? Sorry, I'm not using Windows at all these days so I'm not in the loop...

ping @zbjornson and @chearon, some thoughts on how to proceed would be appreciated :)

@zbjornson
Copy link
Collaborator

Merging #841 was correct, it fixes the issue that came up in this PR :)

@LinusU
Copy link
Collaborator

LinusU commented May 3, 2017

Awesome, closing this then 👍

@LinusU LinusU closed this May 3, 2017
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.

9 participants