Fix cudacodec python by cudawarped · Pull Request #15957 · opencv/opencv

cudawarped · 2019-11-20T13:45:56Z

Merge with contrib: https://github.com/opencv/opencv_contrib/pull/2362/files

This pullrequest adds python bindings to cudacodec

Because cudacodec is in it's own namespace, python bindings are not currently generated for it. This PR fixes this and adds tests to confirm the new functionality is working.

alalek · 2019-11-20T14:44:10Z

modules/python/src2/hdr_parser.py

-                            decls.append(decl)
+                            if "cv.cudacodec.VideoReader.nextFrame" not in decl[0]:
+                                # Disable Mat decl for specific cudacodec function which can take zero args, to allow python call to
+                                # fall through to correct GpuMat decl


Try this instead:

ret, gpu_mat = .nextFrame(frame=cv.cuda_GpuMat())

or add separate wrapper for GPU-only (remove WRAP from nextFrame(OutputArray)):

CV_WRAP inline bool nextFrame(CV_OUT GpuMat& frame) { return nextFrame(OutputArray(frame); }

Hi,

The problem is that a zero argument function will not fall through to the CUDA bindings, therefore your suggestion

ret, gpu_mat = _reader.nextFrame(frame=cv.cuda_GpuMat())

is a good workaround, the only issue is that this is inconsistent with the documentation

nextFrame([, frame[, stream]]) -> retval, frame

which has frame as an optional input argument.

The alternative which removes the MAT bindings

CV_WRAP inline bool nextFrame(CV_OUT GpuMat& frame, Stream& stream = Stream::Null()) { return nextFrame(OutputArray(frame), stream); }

does not work when called by

ret,_ = _reader.nextFrame(gpu_mat)

because CV_OUT and OutputArray generate different python bindings. That is OutputArray results in

const char* keywords[] = { "frame", "stream", NULL }; if( PyArg_ParseTupleAndKeywords(args, kw, "|OO:cudacodec_VideoReader.nextFrame", (char**)keywords, &pyobj_frame, &pyobj_stream) && pyopencv_to(pyobj_frame, frame, ArgInfo("frame", 1)) && pyopencv_to(pyobj_stream, stream, ArgInfo("stream", 0)) )

where frame is an optional input argument. However CV_OUT results in

const char* keywords[] = { "stream", NULL }; if( PyArg_ParseTupleAndKeywords(args, kw, "|O:cudacodec_VideoReader.nextFrame", (char**)keywords, &pyobj_stream) && pyopencv_to(pyobj_stream, stream, ArgInfo("stream", 0)) )

where frame is output only.

Should I remove the additional checks as you suggested and allow the documentation inconsistency because the test case will highlight the required calling convention?

@alalek
The latest changes to the python bindings generation code are to ensure that

CV_WRAP inline bool nextFrame(CV_OUT GpuMat& frame, Stream& stream = Stream::Null()) { return nextFrame(frame, stream); }

will work with both

ret,_ = _reader.nextFrame(gpu_mat)

and

ret,gpu_mat = _reader.nextFrame()

Please run it locally - it doesn't work.

$ OPENCV_PYTHON_DEBUG=1 OPENCV_PYTEST_FILTER=test_cuda ./setup_vars.sh python ${OPENCV_SRC_DIR}/modules/ts/misc/run.py -a -t python3 ... test_cudacodec (test_cuda.cuda_test) ... OpenCV(4.1.2-dev) Error: The function/feature is not implemented (getGpuMat is available only for cuda::GpuMat and cuda::HostMem) in getGpuMat, file modules/core/src/matrix_wrap.cpp, line 359 skipped 'NVCUVID is not installed'

(generated overloads are in this order: "Mat", "GpuMat", "UMat")

This works somehow:

ret,gpu_mat = _reader.nextFrame(cv.cuda_GpuMat()) self.assertTrue(ret) print(type(gpu_mat)) # <== add check for this #print(cv.utils.dumpInputArray(gpu_mat)) # - no support for GpuMat

Is that with the addition of the function declaration you suggested?

CV_WRAP inline bool nextFrame(CV_OUT GpuMat& frame, Stream& stream = Stream::Null()) { return nextFrame(frame, stream); }

I should have mentioned that I have not created a PR for that yet.

The wrapper for nextFrame() in pyopencv_generated_types_content.h with that change locally for me is

using namespace cv::cudacodec; Ptr<cv::cudacodec::VideoReader> * self1 = 0; if (!pyopencv_cudacodec_VideoReader_getp(self, self1)) return failmsgp("Incorrect type of self (must be 'cudacodec_VideoReader' or its derivative)"); Ptr<cv::cudacodec::VideoReader> _self_ = *(self1); PyObject* pyobj_frame = NULL; GpuMat frame; PyObject* pyobj_stream = NULL; Stream stream=Stream::Null(); bool retval; const char* keywords[] = { "frame", "stream", NULL }; if( PyArg_ParseTupleAndKeywords(args, kw, "|OO:cudacodec_VideoReader.nextFrame", (char**)keywords, &pyobj_frame, &pyobj_stream) && pyopencv_to(pyobj_frame, frame, ArgInfo("frame", 1)) && pyopencv_to(pyobj_stream, stream, ArgInfo("stream", 0)) ) { ERRWRAP2(retval = _self_->nextFrame(frame, stream)); return Py_BuildValue("(NN)", pyopencv_from(retval), pyopencv_from(frame)); } return NULL;

If you agree with the changes I can start a PR in the contrib repo for the additional function declaration?

@alalek
Would it make more sense to have an additional flag to signal that the OutputArray is a GpuMat instead of adding additional overides? For example adding CV_GPU

CV_WRAP virtual bool nextFrame(CV_GPU OutputArray frame, Stream &stream = Stream::Null()) = 0;

and excluding the Mat bindings when it is present

if "CV_GPU" in arg_str: modlist.append("/G") arg_str = arg_str.replace("CV_GPU", "")

has_gpu_mat = any(flag == '/G' for var in decl[3] for flag in var[3]) if not has_gpu_mat: decls.append(decl)

I guess this could them be extended to work if there was a mixture of array types to a function.

excluding the Mat bindings

Perhaps reordering should be better.

if "CV_GPU" in arg_str:

Not sure if we need separate notation for that. Lets try to reuse this condition:

if self._generate_gpumat_decls and "cv.cuda" in decl[0]:

Can you expand on what you mean by reordering? If we move the Mat bindings below GpuMat then GpuMat.download() will fail.

move the Mat bindings below GpuMat

Yes, I mean this. Mat overload below GpuMat

then GpuMat.download() will fail

Is this for case with result of Mat type? (so we need to remove Mat completely)
This make sense for outputs (like OutputArray), but perhaps we should accept InputArray as numpy (Mat) array. Anyway, lets try with "excluding the Mat bindings".

Yes the problem is that OutputArray could be a Mat or GpuMat and currently there is no way to tell until the input is parsed. This works fine if the python function has input arguments because the correct bindings will be chosen when this happens.

Currently this also works when there are no input arguments because GpuMat.download() is the only function this applies to, so returning a Mat works. Unfortunately this fails for nextFrame(). Therefore if we reorder it will work for nextFrame() but fail for GpuMat.download().

This make sense for outputs (like OutputArray), but perhaps we should accept InputArray as numpy (Mat) array. Anyway, lets try with "excluding the Mat bindings".

I agree and I think that signaling that OutputArray is a GpuMat will allow input arrays which are a mixture of both types to be processed. Currently I don't think there are any cuda functions which take a mixture of Mat and GpuMat but I may be wrong.

…thon bindings that allow the argument to be an optional output in the same way as OutputArray.

cudawarped · 2019-11-29T15:09:21Z

or self.tp in ["GpuMat", "cuda::GpuMat"]

If the function signature includes the cuda namespace, i.e.

CV_WRAP inline bool nextFrame(CV_OUT cuda::GpuMat& frame, Stream &stream = Stream::Null()) { return nextFrame(OutputArray(frame), stream); }

the compilation will fail because self.tp==cuda_GpuMat.

Is there a way to tell the type of the InputArray, without using an additional flag (CV_GPU), in order create python bindings for functions like SURF_CUDA::uploadKeypoints(const std::vector<KeyPoint>& keypoints, GpuMat& keypointsGPU); which take a mixture of host and device vectors?

alalek · 2019-11-29T16:29:20Z

Just don't include cuda:: for GpuMat in OpenCV API.
"cuda::GpuMat" was added here. If it don't work, then lets remove that.

Mixture of parameters is not supported. Current overloads can handle all GpuMat or all Mat parameters, not mix of both. Trying to handle mix would explode generated code in 2**n times.
Anyway, SURF_CUDA::uploadKeypoints() should work (there is no InputArray at all).

cudawarped · 2019-11-29T16:43:16Z

Just don't include cuda:: for GpuMat in OpenCV API.

Fair enough, should it be documented somewhere, that cuda functions with only an OutputArray or InputArray need to be overloaded for the python bindings to work. And that the cuda namespace should be excluded from the declaration?

Is cuda::GpuMat in

or self.tp in ["GpuMat", "cuda::GpuMat"]

redundant?

Trying to handle mix would explode generated code in 2**n times.
Anyway, SURF_CUDA::uploadKeypoints() should work (there is no InputArray at all).

This was a bad example but I would have thought the size of the generated code would be the same, because you would have a single wrapper for the function containing a mixture of input types?

cudawarped · 2019-11-29T17:39:14Z

"cuda::GpuMat" was added here. If it don't work, then lets remove that.

Would you prefer

        return self.tp == "Mat" or self.tp == "vector_Mat" \
               or self.tp == "GpuMat" \
               or self.tp == "UMat" or self.tp == "vector_UMat" # or self.tp.startswith("vector")

or

       return self.tp in ["Mat", "vector_Mat", "GpuMat", "UMat", "vector_UMat"]

cudawarped · 2019-11-29T21:36:56Z

Trying to handle mix would explode generated code in 2**n times

If size is an issue wouldn't it be better to add a flag to download(), e.g. CV_MAT

CV_WRAP void download(CV_MAT OutputArray dst, Stream& stream) const;

and then all of the CUDA function wrappers except download() can be defined for GPUMat only removing the redundant UMat and Mat processing?

alalek · 2019-11-29T22:45:16Z

Please avoid adding of CV_GPU/CV_MAT modifiers.
They look like hacks to hide implementation specifics (wrote InputArray/OutputArray, but really can't handle set of *Array storage types) and they have no future (because with this approach you would use 2**n super set of these modifiers or much worse n! due ordering issues)

removing the redundant

Remove wrapping of generic InputArray, and just write "inline" specialized wrapper.
Or like in C++, use frame=cv.cuda_GpuMat() approach to specify exact wanted output type.

cudawarped · 2019-12-01T20:18:03Z

They look like hacks to hide implementation specifics (wrote InputArray/OutputArray, but really can't handle set of *Array storage types) and they have no future (because with this approach you would use 2**n super set of these modifiers or much worse n! due ordering issues)

Thank you for clarifying this, I have been completely confused because for most (probably all) functions in the cuda namespace, the arguments of type InputArray/OutputArray can only handle a single array type, either Mat or GpuMat not a set of *Array storage types.

Therefore would it be correct to say the existing nextFrame function signature is wrong and it should be

CV_WRAP virtual bool nextFrame(CV_OUT GpuMat& frame, Stream &stream = Stream::Null()) = 0;

If so should all cuda functions have Input/OuputArray types replaced with the single type that they support?

alalek

Thank you!

Fix cudacodec python * Add python bindings to cudacodec. * Allow args with CV_OUT GpuMat& or CV_OUT cuda::GpuMat& to generate python bindings that allow the argument to be an optional output in the same way as OutputArray. * Add wrapper flag to indicate that an OutputArray is a GpuMat. * python: drop CV_GPU, extra checks in test * Remove "cuda::GpuMat" check rom python parser

Add python bindings to cudacodec.

f677428

cudawarped force-pushed the fix_cudacodec_python branch from 6e4d4c3 to f677428 Compare November 20, 2019 14:05

alalek reviewed Nov 20, 2019

View reviewed changes

alalek mentioned this pull request Nov 20, 2019

Fix type error in cudacodec python interfaces #15961

Closed

cudawarped added 2 commits November 25, 2019 13:59

Allow args with CV_OUT GpuMat& or CV_OUT cuda::GpuMat& to generate py…

d5206a8

…thon bindings that allow the argument to be an optional output in the same way as OutputArray.

Add wrapper flag to indicate that an OutputArray is a GpuMat.

2b310ec

mshabunin mentioned this pull request Nov 27, 2019

Fix cudacodec::VideoReader::nextFrame python bindings opencv/opencv_contrib#2362

Merged

asmorkalov requested a review from VadimLevin November 29, 2019 09:40

python: drop CV_GPU, extra checks in test

c57bb0a

Remove "cuda::GpuMat" check rom python parser

1be3d9c

alalek approved these changes Dec 4, 2019

View reviewed changes

alalek merged commit b4a3c92 into opencv:master Dec 4, 2019

Uh oh!

Conversation

cudawarped commented Nov 20, 2019 • edited by alalek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This pullrequest adds python bindings to cudacodec

Uh oh!

alalek Nov 20, 2019

Choose a reason for hiding this comment

Uh oh!

cudawarped Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cudawarped Nov 25, 2019

Choose a reason for hiding this comment

Uh oh!

alalek Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cudawarped Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cudawarped Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

alalek Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

cudawarped Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

alalek Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

cudawarped Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

cudawarped commented Nov 29, 2019

Uh oh!

alalek commented Nov 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cudawarped commented Nov 29, 2019

Uh oh!

cudawarped commented Nov 29, 2019

Uh oh!

cudawarped commented Nov 29, 2019

Uh oh!

alalek commented Nov 29, 2019

Uh oh!

cudawarped commented Dec 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cudawarped commented Nov 20, 2019 •

edited by alalek

Loading

cudawarped Nov 20, 2019 •

edited

Loading

alalek Nov 25, 2019 •

edited

Loading

cudawarped Nov 25, 2019 •

edited

Loading

alalek commented Nov 29, 2019 •

edited

Loading

cudawarped commented Dec 1, 2019 •

edited

Loading