Windows JPEG support #815

timknip · 2016-09-24T16:11:36Z

This PR adds JPEG support on windows.

Basically a duplicate of #458, but this one needs explicit options to be be set on npm install and node-gyp rebuild. The explicit options make sure nothing breaks.

It needs libjpeg-turbo or libjpeg-turbo64 installed.

Added the jpeg_root variable to binding.gyp (default: c:/libjpeg-turbo64).
Also jpeg62.dll will be copied to build/Release

Install:
node-gyp rebuild --jpeg_root=C:\libjpeg-turbo64 --with_jpeg
npm install --jpeg_root=C:\libjpeg-turbo64 --with_jpeg

Tested on win8.1 x64 node 6.2.1, Visual Studio 2015

zbjornson · 2016-09-24T17:52:34Z

binding.gyp

+              }],
+              'include_dirs': [
+                '<(jpeg_root)/include'
+              ],
              'libraries': [
                '-l<(GTK_Root)/lib/jpeg.lib'


Maybe I've botched my GTK dir, but I don't have a (GTK_Root)/lib/jpeg.lib, so I get a linker error. Did you add this file manually?

#458 changes it to '-l<(jpeg_root)/lib/jpeg-static.lib', but that does not build.

If I change it to '-l<(jpeg_root)/lib/jpeg.lib', it builds, but see comment in main discussion.

@zbjornson You're right, jpeg.lib isn't in GTK, so need the one from libjpeg-turbo.

That compiles. But... there's a nasty issue.

img.src = fs.readFileSync('foo.jpg') works
img.src = foo.jpg doesn't work

The problem is that libjpeg-turbo was compiled against msvcrt.lib, whilst Visual Studio uses msvcrt{VS_VERSION}.lib... Apparently this makes jpeg_read_header hang / crash... See http://stackoverflow.com/questions/4092251/alternatives-to-jpeg-read-header-libjpeg

I'll try fix Image.cc ... but in any case: it seems more involved then previously thought ;-)

Worked around the img.src ="foo.jpg" issue... See below.

zbjornson · 2016-09-24T18:00:09Z

Glad to see someone working on jpeg support for Windows, thanks!

I'm not sure this fully works. Per the comment here, apparently jpeg_stdio_src is not available from jpeg62 (you're copying jpeg62.dll to the destination, but linking against jpeg.lib). Currently there's no test for the code that uses jpeg_stdio_src, and if I add one that I think should work, node hangs.

Could you please add this test to see if the CI builds it on linux?

--- a/test/image.test.js
+++ b/test/image.test.js
@@ -9,6 +9,7 @@ var Canvas = require('../')

 var png_checkers = __dirname + '/fixtures/checkers.png';
 var png_clock = __dirname + '/fixtures/clock.png';
+var jpg_face = __dirname + '/public/face.jpeg';

 describe('Image', function () {
   it('should require new', function () {
@@ -40,6 +41,31 @@ describe('Image', function () {
     assert.strictEqual(320, img.height);
   });

+  it('Image set src to JPEG', function () {
+    var img = new Image
+      , onloadCalled = 0;
+
+    assert.strictEqual(null, img.onload);
+    assert.strictEqual(false, img.complete);
+
+    img.onload = function () {
+      onloadCalled += 1;
+      assert.strictEqual(img.src, jpg_face);
+    };
+
+    img.onerror = function (e) {
+      console.error("ERROR", e); // temporary...
+    };
+
+    img.src = jpg_face;
+    assert.strictEqual(1, onloadCalled);
+    assert.strictEqual(img.src, jpg_face);
+
+    assert.strictEqual(true, img.complete);
+    assert.strictEqual(320, img.width);
+    assert.strictEqual(320, img.height);
+  });
+
   it('test Image#onload multiple times', function() {
     var img = new Image
       , onloadCalled = 0;

zbjornson · 2016-09-24T18:03:11Z

Also:

The explicit options make sure nothing breaks.

Hmm, is there a case when having libjpegturbo installed and findable would break? It's nice to not have to pass a flag to add jpeg support (a la the script in #458). If anyone is installing canvas as something other than a primary dependency (e.g. they depend on package that depends on canvas), it'll be hard to pass the flag along.

timknip · 2016-09-25T11:08:05Z

@zbjornson

Latest commit includes your patch for image.test.js
npm test --with_jpeg=true passes.

apparently jpeg_stdio_src is not available from jpeg62

I edited Image.cc in a way that jpeg_stdio_src is never used on windows. When loading a jpeg from file Image::loadJPEGFromBuffer is used. The hit is that CAIRO_VERSION_MINOR>=10 must be used. Luckily the Cairo shipped with GTK2 is version 10. (hmmm, what is the reason of CAIRO_VERSION_MINOR>=10 ?, guess I can simply use Image::loadJPEGFromBuffer without the check for >= 10 ?)

it'll be hard to pass the flag along.

~~Agree... Do you have a suggestion? Its possible of course to check for libjpeg-turbo using some node script.~~

EDIT: see below

timknip · 2016-09-25T11:43:31Z

@zbjornson Ok reworked Image.cc a bit so it will not use jpeg_stdio_src on windows. See here, Its simply using Image::loadJPEGFromBuffer when loading from file.

Furthermore I used win_jpeg_lookup.js from #458 in binding.gyp to check for c:\libjpeg-turbo or c:\libjpeg-turbo64. So, the flags are not needed anymore.

Hmm, is there a case when having libjpegturbo installed and findable would break?

Only when libjpeg-turbo is not installed under C:\ I guess.

Seems the only difference with #458 is usage of jpeg.lib instead of jpeglib-static.lib and my changes to Image.cc to not use jpeg_stdio_src on windows (as jpeg_stdio_src isn't available as this comment explains:

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().

Note: this PR hence fixes the problem with Visual Studio 2015 as mentioned by you in #458.

Compilation succeeds both with VS2013 and VS2015. So guess it'll build on any recent VS version.

npm test also succeeds

zbjornson · 2016-09-25T19:11:58Z

👍 Good solution. Compiles for me on Win 7 / VS2015 and tests pass. Browser JPEG tests have a black background but, if I remember, that's normal... Thanks!

zbjornson · 2016-10-17T03:27:33Z

@LinusU when you're free would you mind taking a look at this please? It looks good to me, is semver-minor and would be super nice to finally get settled. ;)

(Edit: maybe even semver patch, if you consider missing jpeg support a bug. -- not major at least.)

LinusU

Looks good! Just a few nits, I would be very happy if you dropped the 50b080b commit entirely. I'll create a release commit when I cut a new release, as it is now I can't merge this because the version number is outdated...

@zbjornson thanks for the ping 💌

LinusU · 2016-10-17T06:04:42Z

package.json

@@ -1,7 +1,7 @@
 {
  "name": "canvas",
  "description": "Canvas graphics API backed by Cairo",
-  "version": "1.5.0",
+  "version": "1.5.1",


Please don't bump the version number, I'll do that as part of the release process

LinusU · 2016-10-17T06:06:51Z

test/image.test.js

+    };
+
+    img.onerror = function (e) {
+      console.error("ERROR", e); // temporary...


How about onerrorCalled = 0, onerrorCalled += 1 and assert.strictEqual(onerrorCalled, 0)?

@LinusU Missed your post ;-) I'll prep a fresh PR based on latest master when I find some time. Probably somewhere later this week.

Thank you 💌

LinusU · 2016-10-17T06:10:05Z

test/image.test.js


 describe('Image', function () {
+  this.timeout(5000);


This is quite a long time, is it really needed?

chearon · 2016-11-07T14:15:02Z

just curious, how come there need to be changes to the way JPEGs are loaded from a file on Windows? I was able to get JPEG support on Windows by only modifying the build and not the source, using MSYS2

zbjornson · 2016-11-07T16:19:40Z

@chearon see reason in my first comment and code comment. To me, the minor change to the source is preferable to changing the build chain to use msys, which is a much less common tool set. (Although your precompiled binaries make that invisible for users of them.)

chearon · 2016-11-07T20:21:47Z

Oh sorry, did not realize that was the entire discussion...interesting problem, and it seems like the MSYS2 build would have the same problem since it links with msvcrt, but I can't figure out [how to tell] which C library node is using (is it statically built in?)

zbjornson · 2016-11-07T20:52:24Z

No worries :). MSVCRT is statically linked to node, yes. (I think this is the same problem that fs-ext is facing (a, b, c).)

timknip · 2016-11-08T19:23:32Z

@LinusU @zbjornson Please check #841. I'm closing this one.

timknip2 added 2 commits September 24, 2016 17:48

windows jpeg support, use npm install --with_jpeg=true

9fedc86

v1.5.1

50b080b

zbjornson reviewed Sep 24, 2016

View reviewed changes

win32/jpeg: force Image::loadJPEGFromBuffer when loading from file

0700b76

adding win_jpeg_lookup.js

024f30c

LinusU reviewed Oct 17, 2016

View reviewed changes

timknip mentioned this pull request Nov 8, 2016

windows jpeg support #841

Merged

timknip closed this Nov 8, 2016

zbjornson mentioned this pull request May 1, 2017

[Error: error while reading from input stream] #856

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows JPEG support #815

Windows JPEG support #815

timknip commented Sep 24, 2016 •

edited

Loading

zbjornson Sep 24, 2016

timknip Sep 25, 2016

timknip Sep 25, 2016

zbjornson commented Sep 24, 2016

zbjornson commented Sep 24, 2016

timknip commented Sep 25, 2016 •

edited

Loading

timknip commented Sep 25, 2016 •

edited

Loading

zbjornson commented Sep 25, 2016

zbjornson commented Oct 17, 2016 •

edited

Loading

LinusU left a comment

LinusU Oct 17, 2016

LinusU Oct 17, 2016

timknip Nov 7, 2016

LinusU Nov 7, 2016

LinusU Oct 17, 2016

chearon commented Nov 7, 2016

zbjornson commented Nov 7, 2016

chearon commented Nov 7, 2016

zbjornson commented Nov 7, 2016

timknip commented Nov 8, 2016

Windows JPEG support #815

Windows JPEG support #815

Conversation

timknip commented Sep 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbjornson commented Sep 24, 2016

zbjornson commented Sep 24, 2016

timknip commented Sep 25, 2016 • edited Loading

timknip commented Sep 25, 2016 • edited Loading

zbjornson commented Sep 25, 2016

zbjornson commented Oct 17, 2016 • edited Loading

LinusU left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chearon commented Nov 7, 2016

zbjornson commented Nov 7, 2016

chearon commented Nov 7, 2016

zbjornson commented Nov 7, 2016

timknip commented Nov 8, 2016

timknip commented Sep 24, 2016 •

edited

Loading

timknip commented Sep 25, 2016 •

edited

Loading

timknip commented Sep 25, 2016 •

edited

Loading

zbjornson commented Oct 17, 2016 •

edited

Loading