C++ interface for CvCapture, CvVideoWriter?

Hello,

After some of the recent changes in highgui (btw, I’m very pleased to see it developing fast) I found that the Windows branch is broken. For now I fixed it somehow so it compiles again, but I think it’s probably a good time to do some “refactoring”. Here is why. When I surfed through the code, I found some places that worry me or just look ugly:

1)      cvCreate{File|Camera}Capture calls specific functions that both allocate memory for CvCapture (or “derived” structures) and initialize it. On the other hand, cvReleaseCapture calls CvCapture::close “virtual method”, which only deinitializes the capture structure, but does not deallocate it, and then calls cvFree(). This asymmetry is already a bit dangerous, and may become plain wrong if/when we introduce some plugin mechanism for highgui.

In fact, some sort of plugin is already implemented on Windows. Ffmpeg is wrapped into ffopencv*.dll library that is loaded on-fly.

2)      CvVideoWriter. Almost the same problem with close as in CvCapture, but more severe. That is, in some particular implementations of the writer the close functions (renamed to [i]cvReleaseVideoWriter_*) take double pointer (!!!) whereas the function is declared as taking single pointer. For some reason, it works now, but this is certainly a bug.

3)      That may seem as minor complaint, but comparing to image codecs, all this code with manually initialized “virtual function tables”, with cvReleaseCapture etc. checking if some methods are NULL or if there is enough number of virtual methods etc. looks a bit ugly.

To address these problems, I suggest to introduce the base classes, or even structures for CvCapture and CvVideoWriter. The external API will remain absolutely the same. So the following could be placed to _highgui.h, or some dedicated _cvcap.h:

----------

struct CvCapture

{

virtual ~CvCapture() {};

               virtual double getProperty(int propId) /* probably with “const” suffix */ { return 0; }

               virtual void setProperty(int propId, double propVal) { };

               virtual int /* or bool */ grabFrame() { return 1; };

               virtual IplImage* retrieveFrame() { return 0; }

               virtual IplImage* queryFrame() { return grabFrame() >= 0 ? retrieveFrame() : 0; }

};

CvCapture* cvCreateFileCapture_VFW( const char* filename );

CvCapture* cvCreateCameraCapture_XYZ(…);

…

/* cvcap_vfw.cpp:  */

…

struct CvFileCapture_VFW : public CvCapture

{

CvFileCapture_VFW( const char* filename );

virtual ~CvFileCapture_VFW();

…

};

CvCapture* cvCreateFileCapture_VFW( const char* filename ) { return new CvFileCapture_VFW(…); }

…

// and similarly for video writers:

struct CvVideoWriter

{

virtual ~CvVideoWriter() {};

               virtual int /* or bool */ writeFrame(const CvArr* image);

};

CvVideoWriter* cvCreateVideoWriter_VFW(const char* filename, CvSize size, /*… */) ;

…

/* cvcap_vfw.cpp */

…

struct CvVideoWriter_VFW : public CvVideoWriter

{

CvVideoWriter_VFW( /*…*/ );

virtual ~CvVideoWriter_VFW();

…

};

CvVideoWriter* cvCreateVideoWriter_VFW(const char* filename, CvSize size, /*… */)  { return new CvVideoWriter_VFW(…); }

----------

This should completely solve the problems 2) and 3). The problem 1) will become less pronounced and can be completely solved by introducing extra virtual methods CvCapture::release() and CvVideoWriter::release() that will basically do “delete this;” (that is, “delete” will be called from the same shared library/plugin as the corresponding “new” in cvCreate*, thus the same runtime & heap will be used) . Introducing CvCapture/CvVideoWriter::operator new/delete is another possible solution. But for now neither of these is necessary; so we can do it later (unless we find even better solution by that time).

If there is no objections, I can make the necessary changes and debug the Windows-related part.

Regards,

Vadim

Hello Mark, Nils, Xavier, all,

I’ve just committed to CVS all the modified cvcap_*.cpp and _highgui.h.

VFW and FFMPEG seem to work, did not check video writers yet though.

In case of Windows-related modules or small modules the changes are substantial - most of the code has been put into the methods. Whereas in cvcap_qt, cvcap_v4l and cvcap_gstreamer the changes are minimal; I tried to avoid any risk and thus added C++ classes “on top” of the C structures and functions (because now I do not have access to Linux/Mac box to check if it even compiles or not). Please, feel free to modify them further as much as you want.

Regards,

Vadim

Hello,

After some of the recent changes in highgui (btw, I’m very pleased to see it developing fast) I found that the Windows branch is broken. For now I fixed it somehow so it compiles again, but I think it’s probably a good time to do some “refactoring”. Here is why. When I surfed through the code, I found some places that worry me or just look ugly:

1)      cvCreate{File|Camera}Capture calls specific functions that both allocate memory for CvCapture (or “derived” structures) and initialize it. On the other hand, cvReleaseCapture calls CvCapture::close “virtual method”, which only deinitializes the capture structure, but does not deallocate it, and then calls cvFree(). This asymmetry is already a bit dangerous, and may become plain wrong if/when we introduce some plugin mechanism for highgui.

In fact, some sort of plugin is already implemented on Windows. Ffmpeg is wrapped into ffopencv*.dll library that is loaded on-fly.

2)      CvVideoWriter. Almost the same problem with close as in CvCapture, but more severe. That is, in some particular implementations of the writer the close functions (renamed to [i]cvReleaseVideoWriter_*) take double pointer (!!!) whereas the function is declared as taking single pointer. For some reason, it works now, but this is certainly a bug.

3)      That may seem as minor complaint, but comparing to image codecs, all this code with manually initialized “virtual function tables”, with cvReleaseCapture etc. checking if some methods are NULL or if there is enough number of virtual methods etc. looks a bit ugly.

To address these problems, I suggest to introduce the base classes, or even structures for CvCapture and CvVideoWriter. The external API will remain absolutely the same. So the following could be placed to _highgui.h, or some dedicated _cvcap.h:

----------

struct CvCapture

{

virtual ~CvCapture() {};

               virtual double getProperty(int propId) /* probably with “const” suffix */ { return 0; }

               virtual void setProperty(int propId, double propVal) { };

               virtual int /* or bool */ grabFrame() { return 1; };

               virtual IplImage* retrieveFrame() { return 0; }

               virtual IplImage* queryFrame() { return grabFrame() >= 0 ? retrieveFrame() : 0; }

};

CvCapture* cvCreateFileCapture_VFW( const char* filename );

CvCapture* cvCreateCameraCapture_XYZ(…);

…

/* cvcap_vfw.cpp:  */

…

struct CvFileCapture_VFW : public CvCapture

{

CvFileCapture_VFW( const char* filename );

virtual ~CvFileCapture_VFW();

…

};

CvCapture* cvCreateFileCapture_VFW( const char* filename ) { return new CvFileCapture_VFW(…); }

…

// and similarly for video writers:

struct CvVideoWriter

{

virtual ~CvVideoWriter() {};

               virtual int /* or bool */ writeFrame(const CvArr* image);

};

CvVideoWriter* cvCreateVideoWriter_VFW(const char* filename, CvSize size, /*… */) ;

…

/* cvcap_vfw.cpp */

…

struct CvVideoWriter_VFW : public CvVideoWriter

{

CvVideoWriter_VFW( /*…*/ );

virtual ~CvVideoWriter_VFW();

…

};

CvVideoWriter* cvCreateVideoWriter_VFW(const char* filename, CvSize size, /*… */)  { return new CvVideoWriter_VFW(…); }

----------

This should completely solve the problems 2) and 3). The problem 1) will become less pronounced and can be completely solved by introducing extra virtual methods CvCapture::release() and CvVideoWriter::release() that will basically do “delete this;” (that is, “delete” will be called from the same shared library/plugin as the corresponding “new” in cvCreate*, thus the same runtime & heap will be used) . Introducing CvCapture/CvVideoWriter::operator new/delete is another possible solution. But for now neither of these is necessary; so we can do it later (unless we find even better solution by that time).

If there is no objections, I can make the necessary changes and debug the Windows-related part.

Regards,

Vadim

--------------------------------------------------------------------

Closed Joint Stock Company Intel A/O

Registered legal address: Krylatsky Hills Business Park, 

17 Krylatskaya Str., Bldg 4, Moscow 121614, 

Russian Federation

This e-mail and any attachments may contain confidential material for

the sole use of the intended recipient(s). Any review or distribution

by others is strictly prohibited. If you are not the intended

recipient, please contact the sender and delete all copies.

Adi,

I think, such a change will be useful.

Regarding possible optimizations. Because of the covariation matrix properties you do not need to compute both eigen values and choose min. It will be just:

double smallEig = (Gxx + Gyy – sqrt((Gxx – Gyy)*(Gxx – Gyy) + Gxy*Gxy)))*0.5;

Regards,

Vadim

Hi,

  For a project I've been working on, I needed to know the confidence of the LK tracker result.
Although the status vector gives a binary answer, it is not always enough.

One way to know if the tracked point is not on a corner but on an e.g. is by checking the eigenvals of the 2x2 derivative matrix.
A small eigenvalue means that the matrix is badly conditioned and that either this is an edge (a single small eigenval == aperture effect), or that this is a smooth featureless region (2 small eigen vals).

These situation can happen when the points are not from GoodFeaturesToTrack (which is heavy) or after tracking point over a long time period where drift can happen.

I made a small non-API breaking change to cvlkpyramid.cpp:

*** cvlkpyramid.new.cpp Wed Apr 30 14:10:31 2008
--- cvlkpyramid.org.cpp Thu Aug 31 19:14:36 2006
***************
*** 541,561 ****
                      }

                      D = Gxx * Gyy - Gxy * Gxy;
-
                      if( D < DBL_EPSILON )
                      {
                          pt_status = 0;
                          break;
                      }
-
-                     //////////////////////////////////////////////////////////////////////////
-                     // Adi Shavit - 2008.04
-                     double smallEig  = MIN( -(sqrt(Gyy*Gyy-2*Gxx*Gyy+4*Gxy*Gxy+Gxx*Gxx)-Gyy-Gxx)/2,
-                                              (sqrt(Gyy*Gyy-2*Gxx*Gyy+4*Gxy*Gxy+Gxx*Gxx)+Gyy+Gxx)/2);
-
-                     pt_status = MAX(1,cvRound(smallEig/(jsz.height*jsz.width)));
-                     //////////////////////////////////////////////////////////////////////////
-
                      D = 1. / D;

                      prev_minJ = minJ;
--- 541,551 ----
***************
*** 582,594 ****
              }

              featuresB[i] = v;
!
!             //////////////////////////////////////////////////////////////////////////
!             // Adi Shavit - 2008.04
!             status[i] = (char)MIN(pt_status,255);
!             // status[i] = (char)pt_status; // original line
!             //////////////////////////////////////////////////////////////////////////
!
              if( l == 0 && error && pt_status )
              {
                  /* calc error */
--- 572,578 ----
              }

              featuresB[i] = v;
!             status[i] = (char)pt_status;
              if( l == 0 && error && pt_status )
              {
                  /* calc error */

It calculates the closed form solution for the 2x2 eigenvalues, when the Det != 0, and chooses the smaller value.
It then normalizes the eigval by the window size.
This allows the value to fit relatively comfortably in the status var which is (unfortunately) of type char.

The status char now returns 0 for bad tracking and non-zero value when the tracing succeeds as before.
However, the actual value of status is the normalized smallest eigenval.

The code can be a bit more optimized, but I'd like to know if this kind of change would be acceptible here?

What do you think?

Thanks,
Adi