Add greyscale option to threshold operation to control colourspace conversion before operation

[mhirsch]

This patch adds an optional boolean option that causes the threshold command to operate on each color channel independently, rather than first converting to a black and white image. Default behavior prior to the patch is unchanged.

[lovell]

Rather than apply the same threshold to all channels, perhaps the API should allow multiple thresholds, one for each (non-alpha) channel? This would, for example, allow things like thresholding non-linear sRGB in a more linear manner.

The threshold(threshold) function would be extended to allow the threshold parameter to be either a single number or an array of numbers. Does this make sense?

[mhirsch]

What you're proposing is actually the first way I thought about implementing this. However, I think the use case of applying a different threshold to each color channel is rare, and I thought it didn't warrant the added complexity of handling a mismatch between the length of the threshold vector and the number of image channels. If calling threshold with a single value implies that the image should be converted to a 1 channel image, does calling with two values mean it should be a two channel image? I suppose the easy answer is to throw an error if called with a vector and there is a vector-length / channel number mismatch. What do you think?

[lovell]

Yep, I agree, we definitely don't want to make this too complex until we have to.

How about adding something like threshold(threshold, [options]) to the existing operation, where options is an optional object containing a boolean greyscale attribute, defaulting to true?

This would make your use case look like threshold(value, { greyscale: false } ) whilst maintaining backwards compatibility with the existing/default { greyscale: true } behaviour.

In keeping with the "all variants of English" approach, greyscale and grayscale should both be valid here; don't mind which takes preference.

[mhirsch]

Can anyone explain what's going on with the appveyor test? It says this, which seems to have nothing to do with my new code:

bufferutil.cc
C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\limits(210): warning C4577: 'noexcept' used with no exception handling mode specified; termination on exception is not guaranteed. Specify /EHsc [C:\projects\sharp\node_modules\bufferutil\build\bufferutil.vcxproj]
win_delay_load_hook.c
C:\Program Files\nodejs\node_modules\npm\node_modules\node-gyp\src\win_delay_load_hook.c(34): error C2373: '__pfnDliNotifyHook2': redefinition; different type modifiers [C:\projects\sharp\node_modules\bufferutil\build\bufferutil.vcxproj]
C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\delayimp.h(134): note: see declaration of '__pfnDliNotifyHook2'
gyp ERR! build error
gyp ERR! stack Error: msbuild failed with exit code: 1

[mhirsch]

FYI I've made the requested changes to this branch.

[lovell]

The Appveyor Windows CI failures look like they are related to npm/npm#13199 - don't worry too much about them for now.

[lovell]

Should this say that the default for greyscale is true?

[mhirsch]

Oh, woops. Yes.

[lovell]

Thanks for the updates and the tests. I've added a couple of comments inline.

Please can you add a test case for the explicit use of { greyscale: true } to ensure it matches the default behaviour.

[lovell]

thresholdGrayscale: true ?

[coveralls]

Coverage increased (+0.01%) to 98.472% when pulling 7ea170f on mhirsch:color_threshold into 4b98dbb on lovell:master.

[lovell]

Thanks again for this - it will be in v0.15.1.