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

WIP: Cuda Bindings #544

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jclaessens97
Copy link

I started the implementation of the Cuda bindings by implementing the bindings described here. thisPR is a work in progress SO DON'T MERGE YET, but just so you can follow along and give some input if needed.

Can you review this small part & tell me if there could be any improvements?

To start, I already have 2 questions:

  1. I was wondering why the generateAPITests isn't working on the functions I have to pass an argument in. The test fails because a wrong exception is thrown, although the argument is a required int. The test only succeeds if I write a manual test and set the binding in core.cc also manually instead of using FF_ARG_INT. Any idea?

  2. I didn't implement deviceSupports yet because it's a bit unclear to me how I should handle enums (FeatureSet in this case). I took a look @ TermCriteria, but I still don't get it fully because that one has a constructor but FeatureSet doesn't seem to have a ctor.

Cheers!

@justadudewhohacks
Copy link
Owner

Hi,

The way you implemented the bindings looks fine to me.

I was wondering why the generateAPITests isn't working on the functions I have to pass an argument in. The test fails because a wrong exception is thrown, although the argument is a required int. The test only succeeds if I write a manual test and set the binding in core.cc also manually instead of using FF_ARG_INT. Any idea?

Hmm, could you show me the exception that is thrown? Maybe you forgot to set the usesMacroInferno option in generateAPITests:

// provide backwards compatibility for bindings implemented with
// macro-inferno macros
usesMacroInferno = false

I know this is kind of ugly, but in order to test bindings using the FF macros from macro inferno this has to be set. Actually we wanted to move away from macro inferno and use the worker pattern and rather use converters from native-node-utils. But for now I would say, it is ok to keep your implementation of the bindings as is.

  1. I didn't implement deviceSupports yet because it's a bit unclear to me how I should handle enums (FeatureSet in this case). I took a look @ TermCriteria, but I still don't get it fully because that one has a constructor but FeatureSet doesn't seem to have a ctor.

I think you can export the featureSet constants simply like the other constants, for example the cv.statModel constants here

@jclaessens97
Copy link
Author

jclaessens97 commented Apr 24, 2019

This is the error i'm getting:

1) core - cuda
       printCudaDeviceInfo
         sync
           should throw if no args:
     AssertionError: expected '' to include 'expected arg 0 to be'
      at assertError (utils/testUtils.js:13:10)
      at Context.it (utils/testUtils.js:53:5)

  2) core - cuda
       printCudaDeviceInfo
         sync
           should throw if required arg invalid:
     AssertionError: expected '' to include 'GetDevice - expected arg 0 to be of type'
      at assertError (utils/testUtils.js:13:10)
      at expectError (utils/defaultTests.js:112:7)
      at Context.it (utils/defaultTests.js:127:9)



npm ERR! code ELIFECYCLE
npm ERR! errno 2
npm ERR! opencv4nodejs_test@1.1.0 test: `mocha --require ./globals --timeout 2000 --recursive ./tests`
npm ERR! Exit status 2
npm ERR! 
npm ERR! Failed at the opencv4nodejs_test@1.1.0 test script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/jeroen/.npm/_logs/2019-04-24T17_36_55_124Z-debug.log

For every generateAPI test (on the methods I implemented) where I need to pass required args.

This is the snippet of the test:

  describe('printCudaDeviceInfo', () => {
    it('should have function printCudaDeviceInfo', () => {
      expect(cv).to.have.property('printCudaDeviceInfo').to.be.a('function');
    });

    generateAPITests({
      getDut: () => cv,
      methodName: 'getDevice',
      hasAsync: false,
      getRequiredArgs: () => [1],
      expectOutput: res => expect(res).to.be.a('number'), 
      usesMacroInferno: true,
    });

and of the binding (like I had it before the commit):

NAN_METHOD(Core::PrintCudaDeviceInfo) {
  FF_METHOD_CONTEXT("Core::PrintCudaDeviceInfo");
  FF_ARG_INT(0, int device);

  // if (!FF_IS_INT(info[0])) {
  //   return Nan::ThrowError("Core::PrintCudaDeviceInfo expected arg0 to an int");
  // }

  // int32_t deviceNum = FF_CAST_INT(info[0]);
  cv::cuda::printCudaDeviceInfo(device);
}

It is probably because I don't check if I receive the argument like I do in the commented part, but I'm wondering why this works without checking:

NAN_METHOD(Imgproc::Canny) {
	FF_METHOD_CONTEXT("Canny");

	FF_ARG_INSTANCE(0, cv::Mat dx, Mat::constructor, FF_UNWRAP_MAT_AND_GET);
	FF_ARG_INSTANCE(1, cv::Mat dy, Mat::constructor, FF_UNWRAP_MAT_AND_GET);
	FF_ARG_NUMBER(2, double threshold1);
	FF_ARG_NUMBER(3, double threshold2);
	FF_ARG_BOOL_IFDEF(4, bool L2gradient, false);

	FF_OBJ jsMat = FF_NEW_INSTANCE(Mat::constructor);
	cv::Canny(dx, dy, FF_UNWRAP_MAT_AND_GET(jsMat), threshold1, threshold2, L2gradient);

	FF_RETURN(jsMat);
}

I'm pretty certain that the treshold parameters are required as well?

@justadudewhohacks
Copy link
Owner

I think you have a small typo there: methodName: 'getDevice'. GetDevice does not seem to have any required arguments, that's why you are receiving this error.

@jclaessens97
Copy link
Author

The error I was talking about was different before, but the 'usesMacroInferno' fixed it. Sorry.

@jclaessens97
Copy link
Author

I finished the basic cuda bindings, next up I'm gonna start implementing gpuMat and imgproc, probably this weekend, but not sure yet :)

If you have any remarks let me know.

@goulash1971
Copy link

@jclaessens97 wondering whether you're still working on this / how far you managed to get with the bindings ... would love to help if I can :)

@jclaessens97
Copy link
Author

jclaessens97 commented Nov 19, 2019

@goulash1971 hey, sorry for the late response. Sadly I didn't get much more than just some basics working. I tried to become more familiar with the code and implemented some basic CUDA stuff, but since I'm still going to college where I have some projects, I have an internship and I work at a startup I don't have any time left to work on this at this time. I would like to pick it up later, but for now feel free to start wherever you want :) I did a PR on the other project to add the CUDA flags to the install package.

The commits I added is basically everything I have achieved until now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants