Issues round-tripping data with ByRefArg and List* wrappers

[GoogleCodeExporter]

I wanted to raise this because I don't have a quick solution, but would
like to see if there was some way to deal with it more gracefully.

The current ctypes helpers for wrapping (ByRefArg, ListByRef, ListPOINTER
and ListPOINTER2) work very well when the source of the data originates in
Python and is thus naturally in native Python data types.  They don't do so
well (and in fact some time preclude) using data as returned from OpenCV
itself.

For example, given a contour:

    polygon = cvApproxPoly(contour, sizeof(CvContour), storage,
                           CV_POLY_APPROX_DP, 100)
    points = (CvPoint * polygon.total)()
    cvCvtSeqToArray(polygon, points, cvSlice(0, polygon.total))

I've now got the points in a C-array, and would like to draw it, but I
can't just do:

    cvPolyLine(frame, [points], cvScalarAll(0))

even though that's most efficient, because the wrapper uses ListPOINTER2
which requires that the inner element be a tuple/list.  Instead I have to:

    cvPolyLine(frame, [tuple(points)], cvScalarAll(0))

but this requires walking the C array points to turn it into a Python level
tuple, but then ListPOINTER2 creates a brand new ctypes array data type,
re-walks my tuple and re-creates the C array.  That's great if all I had
was a Python tuple to start with, but for returned data (that may be
largish arrays), it's a bunch of unneeded time and space.

And unlike some other simplified wrappers, I can't bypass this without
defining my own wrapping function since the issue is with the core
definition of _cvPolyLine (and use of ListPOINTER2) and not the higher
level Python cvPolyLine wrapper.

ByRefArg isn't necessary as inefficient but it can still require
unnecessary temporary ctypes wrapper object creation.  If I receive a
pointer to the correct type of object from OpenCV, I can't pass it directly
back to another function but have to first create a ctypes object from
which byref() will just yield the same address.

An example of this is using cvLoad where I don't need access to the actual
data type (e.g., I don't need to access matrix data), but just want to pass
the returned object back to another function like Undistort2.  It also
covers my recent histogram patches, where dereferencing ".contents" of
hist.bins would be unnecessary if ByRefArg would pass hist.bins through
unchanged, since it's already the proper reference.

Originally I tried some changes for trying to identify if the parameter was
the proper data type, passing through unchanged in that case but it can be
tricky (e.g., the cvLoad case might not pass that test).  

So maybe the better approach is to be more limiting in what the wrappers
perform extra processing on, and in other cases just pass through the
parameter leaving it up to the caller to get it right or get ctypes level
errors.  Since their "assistance" is really designed for when you
specifically have one of the wrapper objects, it should be easy enough to
test against.  That would also avoid the need for special checking of None,
since it would pass through just like other unexpected types.  

Anyway, figured I'd throw it out - I don't want to interfere with the help
the wrappers do provide in the vast majority of cases but would like to be
able to preserve efficient round tripping of OpenCV created objects as well
if possible.

-- David

PS: Slightly related, but barring some changes such as above, should
ListByRef and ListPOINTER have errors if the parameter is something other
than expected, or perhaps better, return the parameter as supplied? 
Otherwise, a non-tuple/list parameter ends up being turned silently into
None/NULL when passed on to OpenCV, which bit me once or twice.

Original issue reported on code.google.com by db3l.em...@gmail.com on 9 Jan 2009 at 12:53

[GoogleCodeExporter]

Hey, so I had just thought of the idea of explicitly checking for valid types 
to wrap
rather than those to pass through when writing the initial comment above, so I 
spent
some time trying that approach, and while it touched more than I expected, I 
think it
came out pretty well.  I've attached a patch, which is a decent size, but
fundamentally just does:

ListPointer*, ListByRef and ByRefArg now check (isinstance) if the parameter is 
an
instance of the configured type, and only then does wrapping occur.  Otherwise 
the
parameter is returned unchanged and left for ctypes to deal with.  For the List*
guys, that can either be at the outer level (not a list/tuple) or for individual
elements within that list/tuple.

The fallout from that change required other changes, but I actually think in 
some
areas it actually improves things.  Because of the above the following other 
changes
were made:

1. Base classes for some structure definitions were modified to declare a 
hiearchy to
permit them to match appropriately in function definitions.  CvMat, IplImage,
_CvSeqStructure, and CvSparseMat now inherit from CvArr so they match, for 
example,
for any function taking CvArr_r.  Also, I switched to inheritance for the CvSeq
hierarchy (which naturally inherits the _fields_ from the base classes) rather 
than
manually constructing _fields_.  This means that CvSet, CvGraph, CvChain, 
CvContour,
and CvContourTree inherit from CvSeq, CvChainPtReader inherits from CvSeqReader,
CvSubdiv2D inherits from CvGraph and CvGraphVtx2D inherits from CvGraphVtx.  I 
think
the inheritance is actually clearer than the last setup.
2. Any of the *Release* calls that used the form c_void_p(addressof(self)) which
presumed a byref() was going to occur are changed to use XXX_p(self) where XXX 
is the
data type of the object.  This permits the supplied reference to self to pass 
the
type test in the wrappers, preserving the byref().
3. cvSeqPush (and its use in _cvSeqStructure) needing tweaking.  It was odd 
since
unlike it's C definition, it was defined to take a CvArr_r but none of the other
CvSeq* methods were (they used the actual definition of c_void_p).  And the one 
demo
using it was pushing a CvPoint which certainly isn't a CvArr, so as best I can 
see
the only use of CvArr_r was to genericlly get byref() to be called.  I reverted
SeqPush back to c_void_p, and had the append() method of _cvSeqStructure handle 
the
conversion of a Python side object to a c_void_p.  This seems more appropriate 
to me
as the physical call takes the c_void_p but the Python-friendly append takes any
ctypes-opencv object (or random object for that matter).

With the above, my prior problem points all work - I can supply a filled array 
of
points from cvCvtSeqToArray to cvPolyLine without a problem, or take the result 
of
cv.Load() for an array and pass it directly to Undistort2 without any casting.  
The
patch also includes a reversion of the histogram hist.bins.contents change back 
to
just hist.bins since that works as well.

My own recent code all continues to run properly.  All of the demos (aside from 
the
few requiring a video feed to test, which I don't currently have - I need to 
find a
reasonable avi file somewhere) are running properly.  One demo (minarea) 
requires one
change to handle the SeqPush change, and is included in this patch.  I modified 
it to
use CvPoint_p to push a pointer to the CvPoint, but perhaps it would be better 
to
switch to using the append() method as in squares.py

Let me know what you think.

-- David

PS: Since I walked through all the demos testing this, a number of the demos 
that
don't destroy their windows (edge, demhist, contours, distrans, ffilldemo) 
before
exiting are generating exceptions on my system - this happens with the current 
trunk
version (none of my changes) as well.  I think they used to be ok, so there may 
be
something going on somewhere in some of the recent changes, but I haven't had a
chance to look further.  Oh, and also, small buglet - watershed crashes if you 
try to
run the algorithm without drawing something - it needs a safety check to exit 
out
early if comp_count is 0.

Original comment by db3l.em...@gmail.com on 9 Jan 2009 at 4:35

Attachments:

• reference.diff

[GoogleCodeExporter]

Sorry for my late response, and thanks a lot for all the efforts you have made. 
I
have been busy with preparing a rebuttal for my thesis examination. I am only 
half
way done though. I will quickly update other patches and will come back to this 
one
when I have more proper time. I hope you don't mind.

Minh-Tri

Original comment by pmtri80@gmail.com on 9 Jan 2009 at 5:08

[GoogleCodeExporter]

Late response?  I only submitted it hours ago :-)  Take your time...

Original comment by db3l.em...@gmail.com on 9 Jan 2009 at 5:11

[GoogleCodeExporter]

While working through some notes I had jotted about issues encountered while 
working
on my own code, just wanted to note that thsi patch cleaned up one of those 
issues,
as another example.  As defined _cvMinMaxLoc() uses ByRefArg for min_val and 
max_val.
 Although the cvMinMaxLoc wrapper lets me pass None in for one of those (if I'm not
interested in them), the underlying ByRefArg wrapper was previously rejecting 
it. 
With the patch in this issue, that None passes through cleanly and works fine.

Original comment by db3l.em...@gmail.com on 13 Jan 2009 at 10:21

[GoogleCodeExporter]

Sorry for the delay. My Windows went unstable in the last few days. I couldn't 
fix
it, and ended up reinstalling the whole computer. Things are getting back to 
normal now. 

I've just had a quick look your patches and they are really cool. I like some 
ideas
you've implemented in the patches. I'll start patching the code immediately.

Original comment by pmtri80@gmail.com on 14 Jan 2009 at 1:47

[GoogleCodeExporter]

Patched file 'reference.diff'. I like your solutions for ByRefArg and List* 
wrappers,
and the improved definitions of the Cv* classes. I think the docstrings of the 
new
ByRefArg and List* wrappers should also include descriptions for these 
additions.
I'll do that soon.

As for the contour example you gave in the first post, I was wondering if you 
would
like to consider the 'asarray' helper function I wrote to convert a CvSeq into 
a list
of elements. Your example can be rewritten as (I haven't tested):

    polygon = cvApproxPoly(contour, sizeof(CvContour), storage, CV_POLY_APPROX_DP, 100)
    cvPolyLine(frame, [points.asarray(CvPoint)], cvScalarAll(0))

Original comment by pmtri80@gmail.com on 14 Jan 2009 at 2:07

• Changed state: Fixed

[GoogleCodeExporter]

The class hierarchy you have implemented is really useful. Not only does it 
solve the
problems with the data returned from OpenCV, it solves some of my issues in my 
end,
too. Thanks a lot for the patch.

Original comment by pmtri80@gmail.com on 14 Jan 2009 at 2:15

[GoogleCodeExporter]

Yes, I hadn't really located all of the pure Python methods you had implemented 
back
when that original code for that example was written, so if I just wanted the 
point
values, asarray would be nice.  I've also recently begun using hrange() on the
contours to simplify walking them - dealing with the fact that the first 
contour is a
full object, but then you have to dereference h_next.contents, which I had been
handling manually.

I'm not sure asarray is ideal for this specific example though, since I can keep
everything on the OpenCV/C side of the fence with two calls (cvCvtSeqToArray and
PolyLine) with maximum performance.  Using asarray would round-trip through a 
Python
list as well as using a sequence reader so a function call overhead for each 
point in
the sequence.

Now if I was going to work with the sequence in Python rather than just hand it 
back
intact to OpenCV, asarray() would certainly simplify things.

Original comment by db3l.em...@gmail.com on 14 Jan 2009 at 2:34