ofxOpenCv for 0061 - feedback needed

Hi all,

There is a new ofxOpenCv update. Besides several bugfixes
I also made some functional changes. These changes are related to new
features of the 006 release that caused some confusion.

If you can give the new code a spin that would be great:
http://file.stefanix.net/ofxOpenCv-for-0061.zip

Things to double check for:

  • does the scale conversion work for the floatImage type
  • does it compile on the iPhone now
  • does your ROI code produce the same results still

[CHANGES]

Region of Interest (ROI) is less dominant and more like it’s
implemented in openCV. Before changing the ROI was kind of like saying
“now image pretend you are this subsection”. This was a bit confusing
to many in some regards. So now the member vars width/heigh always
reflect the size of the allocated image, never change to the ROI size.
Also setFromPixels(…), getPixels(), and draw(…) ignore any ROI. If
you want the previous behavior you can use setRoiFromPixels(…),
getRoiPixels(), and drawROI(…) specifically.

ROIs or image sizes have to match for all the methods that operate on
two images. Previously the behavior was to determine the overlapping
area (ROI intersection) and operate on that subpart. While this was
cool it was also a bit confusing when you weren’t expecting the
behavior. It was probably a bit too implicit. It can still be done
manually with a bit of manual code by using a helper method:
getIntersectionROI(…)

[FIXES]

  • scale conversion bugs with ofxCvFloatImage
  • swapTemp bug in conjunction with ROI
  • color Image to planar failed to call flagImageChanged

[ADDITIONS]

  • assignment operator for ofxCvShortImage
  • iPhone #defs

Happy Hacking,

thx,

nice change

do you now how can i make that any contour, or the rectacngle that paint the contour, can be an object, i am playing with, the addon of memo MSAPhysics, and want make thins like hit the ball with may hands or something like that, make interaccion with the camera…

Thx…

Hello.
I need some help. I want send via OSC the centroid of the blob detection.
How can i recieve the coordinates x, y of the centroid of the blob?
THanks for your answer.

0912222136 It’s ok i’ve done it:

for (int i = 0; i < contourFinder.nBlobs; i++){
ofxOscMessage centerX;
centerX.setAddress( “centroidX” );
centerX.addIntArg( i ); // add nBlob
centerX.addFloatArg( contourFinder.blobs[i].centroid.x );
osc_sender.sendMessage( centerX );

ofxOscMessage centerY;
centerY.setAddress( “centroidY” );
centerY.addIntArg( i );// add nBlob
centerY.addFloatArg( contourFinder.blobs[i].centroid.y );
osc_sender.sendMessage( centerY );
contourFinder.blobs[i].draw(0,0);
}

Hi

There are some interesting functions like adaptive threshold and foreground/background segmentation algorythms which could be wrapped in ofxopenCV.

  
//--------------------------------------------------------------------------------  
void ofxCvGrayscaleImage::adaptiveThreshold( int value, int neighbor) {  
	//cvThreshold( cvImage, cvImageTemp, value, 255, CV_THRESH_BINARY );  
	cvAdaptiveThreshold( cvImage, cvImageTemp, value,  
                                  CV_ADAPTIVE_THRESH_MEAN_C,  
                                  CV_THRESH_BINARY,  
                                  neighbor,  
                                  0);  
	swapTemp();  
}  

-> http://forum.openframeworks.cc/t/foreground–background-segmentation-with-opencv/1713/0

greetings ascorbin

When I compile this, I get a ton of conversion warnings. For example: passing floats to allocate() or memcpy() when they ask for ints.

Also, it completely breaks the project I’m working on :slight_smile: But that’s very likely due to the difference in what getWidth() means and how ROI works.

Here’s a modified version that doesn’t have any of the warnings.

Also, I feel like if every time we use getWidth, getHeight and the roi ofRectangle that we have to cast them to ints… maybe they should be ints to start with?

It’s kind of strange, for example, that width and height are stored internally as ints but returned by getWidth and getHeight as floats.

And if ofRectangle is the reason that roi is made of floats, I vote either ofxOpenCv uses its own rectangle type (ofxCvRoi) that is int-based, or that ofRectangle becomes ofRectangle.

Finally, I would love to see ofxCvImage(ofImage& mom), ofxCvImage::clone(ofImage& mom) and ofxCvImage::clone(ofxCvImage& mom), rather than writing out allocate() and setFromPixels()

Also, thanks for all the great work so far Stefan :slight_smile:

ofxOpenCv-no-warnings.zip

Ok, this will be the last consecutive post – if I have any other thoughts I’ll add them here :slight_smile:

This would be nice:

  
  
ofxCvImage::pushROI(int x, int y, int w, int h);  
  

Also, carrying on the theme of constructors from above, why can I do this:

  
  
ofxCvImage a;  
...  
a.setROI(...);  
ofxCvImage b;  
ofRectangle& roi = a.getROI();  
b.allocate(roi.width, roi.height);  
b = a;  
  

But if I do the following, I get an ROI mismatch:

  
  
ofxCvImage a;  
...  
a.setROI(...);  
ofxCvImage b(a);  
  

@ascorbin
yeah, i was already considering adaptiveThreshold but then felt the background subtraction methods are quite comprehensive to hastily add them. Anyways, it’s a good suggestion for upcoming releases and it would be good to collect them in the feature request forum. This way it does not get lost.

@kylemcdonald
I am not sure about the implicit cast warnings. The reason why we haven’t addressed them in ofxOpenCv is that not everybody gets them. With osx gcc you don’t get them, for example. Do you know which compiler have them? Is it mainly VS on windows? I guess there is a way to enable them in gcc under osx. Still trying to figure out how to do it.

I’m not sure which compilers do and don’t give the warnings, but I’m working with GCC in Code::Blocks on Windows. Wouldn’t the better solution be to not do the implicit casts in the first place?

hehe yes! easier said than done when you don’t get warnings :wink:

[quote author=“stefanix”]hehe yes! easier said than done when you don’t get warnings :slight_smile: If you’d like I’d be glad to help by uploading non-warning versions like I did above.

But really, I think to make width and height ints (firstly) and maybe use something other than ofRectangle (or a typed ofRectangle) for ROIs.

ok, just merged the casts into the trunk for now. Also, I find adding a ofxCvRectangle with ints for internal use is worth considering. I am just not sure at the moment if it’s not too confusing to have both rectangle types.

Regarding the clone() functionality, this is kind of what I have in mind:

  
  
void clone(ofxCvImage& from, ofxCvImage& to) {  
	to.clear();  
	to.allocate(  
		(int) from.getWidth(),  
		(int) from.getHeight());  
	to = from;  
}  
  
// same thing, with ofImage  
void clone(ofImage& from, ofxCvImage& to) {  
	to.clear();  
	to.allocate(  
		(int) from.getWidth(),  
		(int) from.getHeight());  
	to.setFromPixels(  
		from.getPixels(),  
		(int) from.getWidth(),  
		(int) from.getHeight());  
}  
  

(These are helper functions I’ve been using). It would be implemented in ofxCvImage rather than as global functions.

On a separate note, since cvResize offers a few different modes, and it would be nice to have whichever you specify be used when calling resize(). Something like this would be great:

  
  
// ofxCvConstants.h  
#define OFX_OPENCV_NN CV_INTER_NN // nearest neighbor  
#define OFX_OPENCV_BILINEAR CV_INTER_LINEAR // bilinear  
#define OFX_OPENCV_AVERAGE CV_INTER_AREA // pixel-area resampling  
#define OFX_OPENCV_BICUBIC CV_INTER_CUBIC // bicubic  
  
// ofxCvImage.h  
class ofxCvImage ... {  
public:  
  virtual void resize( int w, int h, int resizeMode = ofxCvImage::resizeMode) = 0;  
  static void setResizeMode(int resizeMode);  
...  
}  
  
// ofxCvImage.cpp  
int ofxCvImage::resizeMode = OFX_OPENCV_BILINEAR;  
void ofxCvImage::setResizeMode(int resizeMode) {  
  ofxCvImage::resizeMode = resizeMode;  
}  
  
// ofxCvColorImage.cpp (for example)  
void ofxCvColorImage::resize(int w, int h, int resizeMode) {  
...  
  cvResize(original, temp, resizeMode);  
...  
}  
  

If you’d prefer, I could post these in feature requests instead.

Thinking about it some more, I think ofRectangles are used way too often to merit typing them. It’s probably a better idea to keep using them for ROIs, and just make all the necessary casts in ofxOpenCv. It’s just a bit misleading to think that you can define a real number valued ROI.

One more random thought: why does ofxCvImage have:

  
  
virtual void  setFromPixels( unsigned char* _pixels, int w, int h ) = 0;  
  

But not:

  
  
virtual void  operator = ( unsigned char* _pixels );  
  

Is it just being worried that the _pixels won’t fit with the ofxCvImage being modified? I feel like it would make sense to implement it as:

  
  
void  ofxCvImage::operator = ( unsigned char* _pixels ) {  
  setFromPixels(_pixels, (int) width, (int) height);  
}  
  

Sorry if this is a lot of stuff, I’ve just been using ofxOpenCv very heavily recently so I’m really aware of all these little things :slight_smile:

1.) I am not sure why you would need clone(). We have assignment and copy constructor. Aren’t they doing the same thing.
2.) I think adding a optional argument for resize should be fine. I would rather avoid having modes though.
3.) yes, I think that was the reason for omitting an assignment operator for pixels. We can default the width height. would that be really useful. What do people think?

Not quite. clone() is allocating the necessary space and then assigning the pixels. = is just assigning the pixels.

If changing = is an option, I would prefer that = allocates and assigns, while something else (say, setFromPixels()) just assigns (i.e., the current behvaior of = ).

As I understand the = operator, it should create a deep copy of another object… so it’s unintuitive to me that you’d need to allocate space before doing a copy.

[quote author=“stefanix”]
2.) I think adding a optional argument for resize should be fine. I would rather avoid having modes though.[/quote]

Awesome. Out of curiosity, why would you rather avoid modes? Consider the way OpenCV has been wrapped for Processing http://ubaa.net/shared/processing/opencv/opencv-interpolation.html

[quote author=“stefanix”]
3.) yes, I think that was the reason for omitting an assignment operator for pixels. We can default the width height. would that be really useful. What do people think?[/quote]

I vote: yes, useful and intuitive :slight_smile:

the problem with allocating on something like an operator for an image, is that, it’s expensive, and we noticed tons of times people doing non trivial operations every frame (especially with resizing, and doing unnecessary reallocation every frame). My feeling is that if it’s something like an operator, (let me set a equal to b, etc) it should be lightweight, and hard to use improperly and in the specific case of opencv, I think that users should have to think a bit more about allocation…

alot of the way things are now, is based on watching people use it for a while and trying to make decision that lead to best practices, etc.

take care,
zach

Then maybe this is a good compromise:

  
  
void clone(ofxCvImage& from, ofxCvImage& to) {  
   if(from.getWidth() != to.getWidth() ||  
      from.getHeight() != to.getHeight()) {  
      to.clear();  
      to.allocate(  
         (int) from.getWidth(),  
         (int) from.getHeight());  
   }  
   to.setFromPixels(  
      from.getPixels(),  
      (int) from.getWidth(),  
      (int) from.getHeight());  
}  
  

Lightweight when it can be, heavyweight when it must be, intuitive either way. I’ve seen this if(different sizes){reallocate} code show up repeatedly in actual ofxOpenCv use, so it might be better to encapsulate it in operator=.

cool! that’s a good clone :slight_smile:

while there are places where reallocation occurs (ofxCvContour) but that’s pretty often the same size. = is much more likely to be used in a non optimal way. I’ve seen it in action (ie, I think at some point it used to work like this and I got freaked out by how slow seemingly normal looking code could be because of all the “implicit” reallocations)… just trying to help people write better code :slight_smile:

take care!
zach

kyle, I just double-checked. The copy constructor should be allocating the assigned image (unless there is a bug :wink:

You can do something like this:

  
  
ofxCvGrayscaleImage imageA;  
imageA.allocate(320,240);  
//do something with image  
  
ofxCvGrayscaleImage imageB = imageA;      
     //invokes the copy constructor  
     //not assignment operator  
  
  

The last line does essentially what clone does (and it’s super explicit that you are allocating a new image.) We don’t have any implicit resizing in it but I think that is a good thing. It goes along zach’s argument and that it generally leads to better code when the user is aware about the image sizes.

This is the code that is called BTW:

  
  
ofxCvGrayscaleImage::ofxCvGrayscaleImage( const ofxCvGrayscaleImage& _mom ) {  
    init();  
    if( _mom.bAllocated ) {  
        // cast non-const,  to get read access to the mon::cvImage  
        ofxCvGrayscaleImage& mom = const_cast<ofxCvGrayscaleImage&>(_mom);  
        allocate( (int)mom.getWidth(), (int)mom.getHeight() );  
        cvCopy( mom.getCvImage(), cvImage, 0 );  
    } else {  
        ofLog(OF_LOG_NOTICE, "in ofxCvGrayscaleImage copy constructor, mom not allocated");  
    }  
}  
  

Ok, keeping in mind what you’ve both said (Stefan + Zach)… I suppose my more fundamental question is: why is the copy constructor different than operator =? Or, given this:

  
  
ofxCvColorImage gray1;  
gray1.allocate(256, 256);  
  

Why are all these things different:

  
  
// works fine  
ofxCvGrayscaleImage gray2 = gray1;  
  
// same as above  
ofxCvGrayscaleImage gray2(gray1);  
  
// runtime error (needs to be allocated)  
ofxCvGrayscaleImage gray2;  
gray2 = gray1;  
  

Also, given this:

  
  
ofxCvColorImage color;  
color.allocate(256, 256);  
  

Why are these things different:

  
  
// compile error  
ofxCvGrayscaleImage gray = color;  
  
// different compile error  
ofxCvGrayscaleImage gray(color);  
  
// runtime error (needs to be allocated)  
ofxCvGrayscaleImage gray;  
gray = color;  
  

Clearly, an image needs two fundamental abilities: allocation, pixel copying. This can be augmented by conversion (e.g., color to single grayscale). This leaves a few questions:

1 Should the copy constructor do the same thing as operator=?
2 Should the copy constructor or operator= simply replicate behavior of allocate() or setFromPixels()?
3 Should conversion between image types be implicit or explicit?

I think:

1 Yes, it’s confusing when operator= and the copy constructor mean different things (especially because you can use operator= to mean copy constructor).
2 No, if you want to allocate or setFromPixels(), you should use those functions. The copy constructor and operator= should be smart: allocate when necessary, followed by copying and…
3 …implicit conversion.

On a related different topic – but again, more of a feature request – convertToGrayscalePlanarImage(ofxCvGrayscaleImage& gray, int plane) would be helpful when you only need one of the planes (e.g.: saturation).