Add conv command to apply arbitrary convolution kernel to image

[mhirsch]

This patch adds a conv operator to the sharp API so that an arbitrary convolution kernel can be applied to an image. The conv command has one argument that takes a structure containing all the ingredients for a vips convolution kernel, eg.:

{ 'width': N , 'height': M , 'scale': Z , 'offset': Y , 'kernel': [[ 1, 2, 3], [ 4, 5, 6], [ 7, 8, 9]] }

One part of the patch that might need a little work: I couldn't find any documentation on testing whether a VImage object is empty, so I made a bool called convKernelValid that keeps track of whether the user has issued a conv command when building the pipeline. It would be cleaner to track this with the VImage object itself.

[lovell]

Hello, thanks for making a good start on what will be a useful feature.

The instantiation of the VImage for the kernel needs to be within the PipelineWorker::Execute method to ensure any thread-local data lives on a libuv-managed thread (that we later invoke vips_thread_shutdown on), rather than the V8 main thread that the NAN_METHOD(pipeline) method runs on. This should avoid the need for the convKernelValid attribute.

Although the underlying operation is called vips_conv, I would prefer this behaviour be exposed via a function named convolve. The “conv" abbreviation can easily be confused with “conversion”, e.g. ImageMagick provides the convert tool.

I’d prefer the kernel attribute to be a flat array rather than an array of arrays. This more closely matches the underlying vips API and keeps the code simpler, e.g. removes inner loops. The API you’ve added (correctly) expects a width and height, so this can be used to validate the array length. Looping over the values in the kernel is still useful as we can add number validation here.

Please can you add tests to cover this work. I notice you added a couple of “make CI happy” commits. Were you able to run the tests locally as I’d expect these failures to show up there?

[mhirsch]

I'll move the initialization of the kernel VImage. Thanks.

I'm happy to change the name to convolve. I'll look into adding tests -- I haven't done that before. I also have now figured out how to run the tests locally, so I hope I can avoid future commits chasing the test results.

[lovell]

C++11 range based loops are used elsewhere so this should work. What was the error?

[mhirsch]

I was trying to fix the mysterious appveyor failure. I suppose this has nothing to do with it. I can go back on this commit.

[mhirsch]

@lovell thanks for all your feedback. Is there anything else left to do before you would be comfortable merging this?

[lovell]

Might it be possible to use new_from_memory instead here? NAN_METHOD(pipeline) already contains a loop over the values of the kernel array - if it built a char* instead of a std::vector the loop and pointer logic would no longer be required here.

[lovell]

Thanks for all the updates - one comment inline.

Are you able to use more extreme (or at least less tame) test cases, e.g. edge detection? The expected test cases are almost the same as the input so fixtures.assertSimilar won't be able to detect regression.

[coveralls]

Coverage increased (+0.04%) to 98.501% when pulling 3241314 on mhirsch:conv_operator into 4b98dbb on lovell:master.

[lovell]

Does this need to be deleted after use?

[mhirsch]

No, unique_ptr will delete the pointer it's managing when it goes out of scope: http://en.cppreference.com/w/cpp/memory/unique_ptr

[lovell]

Thanks for confirming. I've not had the pleasure of using unique_ptr until now and seeing a new without a delete gave me the usual twitch. It looks like a rather useful feature of C++11, so thanks for introducing it to sharp!

[lovell]

Thanks again Matt for all your work on this feature!

[lovell]

It's always interesting to discover more about what people are using this module for in the survey at #35 should you be able to share details publicly.

[mhirsch]

Thanks!