Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a boolean operator that performs bitwise boolean operations between images #501

Merged
merged 12 commits into from
Jul 11, 2016
Merged

Conversation

mhirsch
Copy link
Contributor

@mhirsch mhirsch commented Jul 8, 2016

This pr adds a boolean operator to sharp that takes a second input image, and performs a selected bitwise boolean operation on a pixel-per-pixel basis between the two images. boolean accepts both files and buffers, much like overlayWith, and uses the same boolean operators as bandbool. It relies on the underlying vips function boolean, which handles casting data types and padding the right-hand operand with zeros to match the size of the left-hand operand.

Get VIPS Boolean operatoin type from string
*/
VipsOperationBoolean GetBooleanOperation(std::string opStr) {
if(opStr == "and" ) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rather handy vips_enum_from_nick function should allow this to be simplified:

return static_cast<VipsOperationBoolean>(
  vips_enum_from_nick(nullptr, VIPS_TYPE_OPERATION_BOOLEAN, opStr.data())
);

@lovell
Copy link
Owner

lovell commented Jul 8, 2016

I've added a few comments inline, thanks for this Matt.

@mhirsch
Copy link
Contributor Author

mhirsch commented Jul 8, 2016

I took a crack at the "passing as a vector of buffers" improvement you suggested. I stored the node name and buffer size so its easier to iterate over the vector in the constructor. I think, since the constructor runs in the main thread, we could get away without storing the buffer length, but I thought I would avoid the ambiguity.

AsyncQueueWorker(new PipelineWorker(callback, baton, queueListener, bufferIn, overlayBufferIn));

std::vector<BufferContainer> saveBuffers;
saveBuffers.push_back({"bufferIn", bufferIn, baton->bufferInLength});
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we only append buffers to the vector where their length is >0 then we don't need to store the length.

@mhirsch
Copy link
Contributor Author

mhirsch commented Jul 9, 2016

@lovell I tried a couple of methods of finding a pointer that is constant between the constructor and HandleOKCallback, and didn't find something that worked. In 4c7f339 I made the mistake of not passing the buffer by reference, but fixing this, it appears that the iterator object is actually declared on the stack (from the memory address) and differs between range loops while the buffer element is on the heap. I wasn't aware that that's how c++11 range loops worked -- so I learned something. In e2cda35, I expected going to a standard for loop over the length of the std::vector, and getting the address of the vector element to work. However, it seems the referenced object has been copied at some point. I realize there is some ambiguity about which vector is being referenced in the constructor due to the class member saveBuffers having the same name as the constructor argument, but I changed the names and the pointers still differed. I'm not sure if this has to do with some obscure fact of a Local<Object> object, or copying std::vectors but I couldn't find a solution and I'm out of time for tonight. The most recent commit goes back to the BufferContainer method of filling the vector.

I think this is going a little off topic from the boolean operator. Can we merge this one, and then work out a better system for passing in buffers to be saved in a later commit?

I would like to be able to reference this mechanism, whatever it is, in a future commit to pass buffers in for other operators that take images.

@coveralls
Copy link

coveralls commented Jul 11, 2016

Coverage Status

Coverage decreased (-0.1%) to 97.706% when pulling c4f42f0 on mhirsch:boolean into 83d8847 on lovell:master.

@lovell lovell merged commit d17e8d3 into lovell:master Jul 11, 2016
@lovell
Copy link
Owner

lovell commented Jul 11, 2016

Thank you Matt!

@mhirsch mhirsch deleted the boolean branch July 11, 2016 09:36
@lovell
Copy link
Owner

lovell commented Jul 11, 2016

I was able to simplify* the SaveTo/GetFromPersistent logic a little in fee3d88 (and e10aeb2 for reasons of d'oh) by using an index-based approach.

*assuming people consider a more functional style to be simpler :-/

@lovell lovell added this to the v0.15.1 milestone Jul 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants