Add joinChannel and toColourspace commands #513

mhirsch · 2016-07-17T04:16:24Z

This PR is a little large -- sorry for that. These two features are somewhat entangled, and I ended up writing toColourspace() in service of what I was trying to do with joinChannels().

Add feature toColourspace / toColorspace to write output in desired colorspace

This implements a feature very much like the one described in #218. This code is tested in colourspace.js and joinChannel.js, but by its nature adds the potential for a lot of complexity. I expect some additional tests will be required. This supersedes #503, which I'll close.

Add feature joinChannels to add additional image channels to the input image

This allows you to join additional channels onto the image. In some cases it is necessary to use toColourspace() to set the output to what you intend. For example, if you open a grayscale image and add three channels, by default that will be interpreted as an sRGB image with an alpha channel. It is necessary to also call toColourspace('cmyk') to ask the image to be interpreted as a CMYK image.

…olorspace Add feature joinChannels to add additional image channels to the input image

lovell · 2016-07-18T14:54:46Z

Hello, thanks Matt, this doesn't seem to compile at the moment due to the use of an unordered_set keyed on VipsInterpretation - see https://travis-ci.org/lovell/sharp/builds/145304825

mhirsch · 2016-07-18T15:39:03Z

I don't have xcode, so it's hard to say what the problem is. I tried changing unordered_map to map. There's a small performance penalty for using map but it probably won't matter.

lovell · 2016-07-19T13:34:44Z

.jshintrc

@@ -7,6 +7,7 @@
    "beforeEach": true,
    "afterEach": true,
    "describe": true,
-    "it": true
+    "it": true,
+    "Promise": true


Promises aren't natively supported in older versions of Node - please use var Promise = require('bluebird'); in any files that need it.

Ok -- that makes sense. Thanks

lovell · 2016-07-19T13:49:50Z

The Buffer-or-filesystem approach of joinChannels feels a bit clunky. Does your use-case require both? Perhaps we could support one only to start with, e.g. Buffer only?

lovell · 2016-07-19T13:51:33Z

docs/api.md

+* CMYK: 0: Magenta, 1: Cyan, 2: Yellow, 3: Black, 4: Alpha
+
+Buffers may be any of the image formats supported by sharp: JPEG, PNG, WebP, GIF, SVG, TIFF or raw pixel image data. In the case of a RAW buffer, the `options` object should contain a `raw` attribute, which follows the format of the attribute of the same name in the `sharp()` constructor. See `sharp()` for details. See `raw()` for pixel ordering. 
+


Do we need to include information that the images to be joined must be the same dimension?

They actually don't have to be of the same dimension -- vips will expand the image to fit the largest channel.

lovell · 2016-07-19T13:54:09Z

This PR definitely proves I need to abstract away the image loading logic copypasta (filesystem vs compressed Buffer vs raw Buffer) as there are now four occurrences :)

mhirsch · 2016-07-19T14:30:46Z

If you were to abstract the image loading logic it would perhaps make it simpler to have buffers or filespaths as inputs. What in particular feels clunky about it? I tried to mimic the file or buffer input options in other functions that accept image input. Would it be easier if they weren't in an array -- i.e. you need to call joinChannels() once per channel?

use BluebirdPromise instead of Promise to support old versions of node

lovell · 2016-07-19T20:26:23Z

(Tomorrow) I'll attempt to extract the common logic (in both JS and C++) for image loading so it can be used by any operation that requires another image as part of its input.

mhirsch · 2016-08-08T06:08:39Z

@lovell I've updated this to be based on the pencil branch, taking advantage of your new OpenInput() system. This removes the awkward restriction of having to load either files or buffers and not both.

mhirsch · 2016-08-08T06:12:14Z

Actually -- what's the best way to handle this. Should I close this and open a new pull request against pencil?

lovell · 2016-08-08T20:58:13Z

Thanks for the updates - I'll be merging pencil to master in a couple of days so please leave this open until then :)

lovell · 2016-08-13T13:50:40Z

Matt, the upstream/master branch now has the CreateInputDescriptor logic - please can you rebase against it.

mhirsch · 2016-08-13T22:30:39Z

Yes -- today or tomorrow! Thanks!

…_toColourspace

mhirsch · 2016-08-14T06:22:01Z

When I git pull from your master in my master branch and run npm install and npm test I get the following:

/home/mhirsch/sharp/index.js:9
__cov_8cTnvYcP2ESJD5pY6tEvog.s['1']++;var path=require('path');__cov_8cTnvYcP2ESJD5pY6tEvog.s['2']++;var util=require('util');__cov_8cTnvYcP2ESJD5pY6tEvog.s['3']++;var stream=require('stream');__cov_8cTnvYcP2ESJD5pY6tEvog.s['4']++;var events=require('events');__cov_8cTnvYcP2ESJD5pY6tEvog.s['5']++;var semver=require('semver');__cov_8cTnvYcP2ESJD5pY6tEvog.s['6']++;var color=require('color');__cov_8cTnvYcP2ESJD5pY6tEvog.s['7']++;var BluebirdPromise=require('bluebird');__cov_8cTnvYcP2ESJD5pY6tEvog.s['8']++;var sharp=require('./build/Release/sharp');__cov_8cTnvYcP2ESJD5pY6tEvog.s['9']++;var versions={vips:sharp.libvipsVersion()};__cov_8cTnvYcP2ESJD5pY6tEvog.s['10']++;(function(){__cov_8cTnvYcP2ESJD5pY6tEvog.f['1']++;__cov_8cTnvYcP2ESJD5pY6tEvog.s['11']++;var libvipsVersionMin=require('./package.json').config.libvips;__cov_8cTnvYcP2ESJD5pY6tEvog.s['12']++;if(semver.lt(versions.vips,libvipsVersionMin)){__cov_8cTnvYcP2ESJD5pY6tEvog.b['1'][0]++;__cov_8cTnvYcP2ESJD5pY6tEvog.s['13']++;th

Error: Found libvips 8.3.2 but require at least 8.3.3
    at /home/mhirsch/sharp/index.js:9:996
    at Object.<anonymous> (/home/mhirsch/sharp/index.js:9:1265)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions.(anonymous function) [as .js] (/home/mhirsch/sharp/node_modules/istanbul/lib/hook.js:107:24)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Module.require (module.js:353:17)
    at require (internal/module.js:12:17)
    at Object.<anonymous> (/home/mhirsch/sharp/test/fixtures/index.js:5:13)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)
    at Object.Module._extensions.(anonymous function) [as .js] (/home/mhirsch/sharp/node_modules/istanbul/lib/hook.js:109:37)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Module.require (module.js:353:17)
    at require (internal/module.js:12:17)
    at Object.<anonymous> (/home/mhirsch/sharp/test/unit/alpha.js:4:16)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)
    at Object.Module._extensions.(anonymous function) [as .js] (/home/mhirsch/sharp/node_modules/istanbul/lib/hook.js:109:37)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Module.require (module.js:353:17)
    at require (internal/module.js:12:17)
    at /home/mhirsch/sharp/node_modules/mocha/lib/mocha.js:220:27
    at Array.forEach (native)
    at Mocha.loadFiles (/home/mhirsch/sharp/node_modules/mocha/lib/mocha.js:217:14)
    at Mocha.run (/home/mhirsch/sharp/node_modules/mocha/lib/mocha.js:485:10)
    at Object.<anonymous> (/home/mhirsch/sharp/node_modules/mocha/bin/_mocha:405:18)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)
    at Object.Module._extensions.(anonymous function) [as .js] (/home/mhirsch/sharp/node_modules/istanbul/lib/hook.js:109:37)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Function.Module.runMain (module.js:441:10)
    at runFn (/home/mhirsch/sharp/node_modules/istanbul/lib/command/common/run-with-cover.js:122:16)
    at /home/mhirsch/sharp/node_modules/istanbul/lib/command/common/run-with-cover.js:251:17
    at /home/mhirsch/sharp/node_modules/istanbul/lib/util/file-matcher.js:68:16
    at /home/mhirsch/sharp/node_modules/async/dist/async.js:486:20
    at /home/mhirsch/sharp/node_modules/async/dist/async.js:1074:13
    at /home/mhirsch/sharp/node_modules/async/dist/async.js:952:25
    at iteratorCallback (/home/mhirsch/sharp/node_modules/async/dist/async.js:997:17)
    at /home/mhirsch/sharp/node_modules/async/dist/async.js:847:20
    at /home/mhirsch/sharp/node_modules/async/dist/async.js:1071:17
    at LOOP (fs.js:1530:14)
    at nextTickCallbackWith0Args (node.js:420:9)
    at process._tickCallback (node.js:349:13)
npm ERR! Test failed.  See above for more details.

Is it possible that the new version of vips hasn't been pushed? Anyway, I merged your master back to this branch and pushed, though I haven't tested things yet.

lovell · 2016-08-14T07:46:32Z

Removing the sharp/lib and sharp/include directories should fix this. npm run clean will do this for you. Perhaps it should do this automagically when there's a version mismatch?

…ce is available

mhirsch · 2016-08-14T16:29:20Z

Yes, that fixes it. I don't think running it automatically sounds right. I wasn't aware npm had a clean command. Thanks ;)

Anyway, it appears all the tests pass on this merged branch. Does this look good to you?

lovell · 2016-08-15T08:13:04Z

index.js

+Sharp.prototype.joinChannel = function(images, options) {
+  if (Array.isArray(images)) {
+    images.forEach(function(image, index) {
+      this.options.joinChannelIn[index] = this._createInputDescriptor(image, options);


Should this use push() rather than be index based? This would allow .joinChannel(array1).joinChannel(array2).

Yes. Woops. I'll make that change.

lovell · 2016-08-15T08:25:39Z

Looking great, thanks. Always a pleasure to see a new feature PR with more lines of tests and documentation than the code to implement it :)

coveralls · 2016-08-16T03:50:14Z

Changes Unknown when pulling 6f79a99 on mhirsch:joinChannel_and_toColourspace into * on lovell:master*.

lovell · 2016-08-17T14:42:55Z

Thanks again Matt!

lovell · 2016-08-17T20:00:04Z

Added 7ada9db to fix a small InputDescriptor leak.

Matt Hirsch added 2 commits July 16, 2016 23:50

Add feature toColourspace / toColorspace to write output in desired c…

539f7f3

…olorspace Add feature joinChannels to add additional image channels to the input image

cleanup a few details

70a6985

mhirsch mentioned this pull request Jul 17, 2016

Add option to greyscale() to enable grayscale output #503

Closed

unordered_map -> map ?? maybe this will fix xcode build error?

8f5da58

Add ability to read raw buffers in joinChannel

6d1d1b3

lovell reviewed Jul 19, 2016
View reviewed changes

outputMode -> colourspace (variable name change)

68d6fa2

use BluebirdPromise instead of Promise to support old versions of node

lovell added the enhancement label Jul 19, 2016

Matt Hirsch added 2 commits July 20, 2016 13:09

Merge branch 'master' into joinChannel_and_toColourspace

b85de24

Merge branch 'master' into joinChannel_and_toColourspace

1b33fbf

lovell mentioned this pull request Jul 29, 2016

Slow resize + png for YCbCr images #481

Closed

Merge branch 'pencil' into joinChannel_and_toColourspace_pencil

bb18328

Add a test of invalid input for toColourspace

856c7d1

Merge branch 'master' of github.com:lovell/sharp into joinChannel_and…

c9849cf

…_toColourspace

fix bandbool unit test to create 1-channel image now that toColourspa…

b38bc03

…ce is available

lovell reviewed Aug 15, 2016
View reviewed changes

lovell added this to the v0.16.0 milestone Aug 15, 2016

Matt Hirsch added 2 commits August 15, 2016 23:34

fix problem where second joinChannel array is not added correctly

816204b

only set the image interpretation once in joinChannel

6f79a99

lovell merged commit 5c5d74a into lovell:master Aug 17, 2016

mhirsch deleted the joinChannel_and_toColourspace branch August 17, 2016 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add joinChannel and toColourspace commands #513

Add joinChannel and toColourspace commands #513

mhirsch commented Jul 17, 2016 •

edited

Loading

lovell commented Jul 18, 2016

mhirsch commented Jul 18, 2016

lovell Jul 19, 2016

mhirsch Jul 19, 2016

lovell commented Jul 19, 2016

lovell Jul 19, 2016

mhirsch Jul 19, 2016

lovell commented Jul 19, 2016

mhirsch commented Jul 19, 2016

lovell commented Jul 19, 2016

mhirsch commented Aug 8, 2016

mhirsch commented Aug 8, 2016

lovell commented Aug 8, 2016

lovell commented Aug 13, 2016

mhirsch commented Aug 13, 2016

mhirsch commented Aug 14, 2016

lovell commented Aug 14, 2016

mhirsch commented Aug 14, 2016

lovell Aug 15, 2016

mhirsch Aug 15, 2016

lovell commented Aug 15, 2016

coveralls commented Aug 16, 2016 •

edited

Loading

lovell commented Aug 17, 2016

lovell commented Aug 17, 2016

		* CMYK: 0: Magenta, 1: Cyan, 2: Yellow, 3: Black, 4: Alpha

		Buffers may be any of the image formats supported by sharp: JPEG, PNG, WebP, GIF, SVG, TIFF or raw pixel image data. In the case of a RAW buffer, the `options` object should contain a `raw` attribute, which follows the format of the attribute of the same name in the `sharp()` constructor. See `sharp()` for details. See `raw()` for pixel ordering.

Add joinChannel and toColourspace commands #513

Add joinChannel and toColourspace commands #513

Conversation

mhirsch commented Jul 17, 2016 • edited Loading

Add feature toColourspace / toColorspace to write output in desired colorspace

Add feature joinChannels to add additional image channels to the input image

lovell commented Jul 18, 2016

mhirsch commented Jul 18, 2016

lovell Jul 19, 2016

Choose a reason for hiding this comment

mhirsch Jul 19, 2016

Choose a reason for hiding this comment

lovell commented Jul 19, 2016

lovell Jul 19, 2016

Choose a reason for hiding this comment

mhirsch Jul 19, 2016

Choose a reason for hiding this comment

lovell commented Jul 19, 2016

mhirsch commented Jul 19, 2016

lovell commented Jul 19, 2016

mhirsch commented Aug 8, 2016

mhirsch commented Aug 8, 2016

lovell commented Aug 8, 2016

lovell commented Aug 13, 2016

mhirsch commented Aug 13, 2016

mhirsch commented Aug 14, 2016

lovell commented Aug 14, 2016

mhirsch commented Aug 14, 2016

lovell Aug 15, 2016

Choose a reason for hiding this comment

mhirsch Aug 15, 2016

Choose a reason for hiding this comment

lovell commented Aug 15, 2016

coveralls commented Aug 16, 2016 • edited Loading

lovell commented Aug 17, 2016

lovell commented Aug 17, 2016

mhirsch commented Jul 17, 2016 •

edited

Loading

coveralls commented Aug 16, 2016 •

edited

Loading