870210 - Extend node-hubble to allow discovery of image sizes

Status: RESOLVED FIXED

Description

[David Humphrey (:humph)]

We would like to know when to use an image based on its size, when to crop it, etc.  Hubble is a perfect candidate for this.  I've written the code and tests, just need to fixup the readme, then I'll get this into review.

WIP here - https://github.com/mozilla/node-hubble/pull/13

Comment 1

[David Humphrey (:humph)]

Attachment: https://github.com/mozilla/node-hubble/pull/13

Finished updating README, this is ready for review

Comment 2

[Matthew Schranz [:mjschranz]]

Comment on attachment 747407 [details] [review]
https://github.com/mozilla/node-hubble/pull/13

The code looks very sane to me, but I'm getting test fails when running locally. Specifically:

1) should give proper sizes for JPEG files
2) should give proper sizes for GIF files

And assertion errors on status codes. This leaves me thinking I'm somehow missing a dependency for the tests? I'm unsure, but everything I needed installed successfully with npm.

Comment 3

[David Humphrey (:humph)]

Take a look at your node_modules/canvas/build/canvas.target.mk, and see if gyp found the right deps.  It should have defined HAVE_JPEG and HAVE_GIF.  Here's what I have after I build:

DEFS_Release := \
	'-D_DARWIN_USE_64_BIT_INODE=1' \
	'-D_LARGEFILE_SOURCE' \
	'-D_FILE_OFFSET_BITS=64' \
	'-DHAVE_FREETYPE' \
	'-DHAVE_JPEG' \
	'-DHAVE_GIF' \
	'-DBUILDING_NODE_EXTENSION'

Comment 4

[Matthew Schranz [:mjschranz]]

Yeah, I'm missing JPEG and GIF from that list. Rather than poke around more I'll let :jbuck look at it. Code R+ from me.

Comment 5

[David Humphrey (:humph)]

Comment on attachment 747407 [details] [review]
https://github.com/mozilla/node-hubble/pull/13

Kind sir, a moment of your time?

Comment 6

[Jon Buckley]

Comment on attachment 747407 [details] [review]
https://github.com/mozilla/node-hubble/pull/13

r+, although I'm not a huge fan of the node-canvas dependency. It'd be nice to have something pure JS... if only we had unlimited time!

Comment 7

[David Humphrey (:humph)]

https://github.com/mozilla/node-hubble/commit/e30151186941e841721312747b16bf4d4977e6d6

The node-canvas dep isn't a big deal for a server.  Basically, once it's doable in JS, node-canvas will switch to it too, so we'll all be happy.