rainyl / opencv_dart

OpenCV bindings for Dart language and Flutter.
https://pub.dev/packages/opencv_dart
Apache License 2.0
80 stars 10 forks source link

Ml module imp #63

Open abdelaziz-mahdy opened 1 month ago

abdelaziz-mahdy commented 1 month ago

this is my attempt to implement the wrappers c++ files

but compilations fail and i would like to fix it first, if you can help guide me to fixes for the compilation let me know

i set this as a draft since i cant move forward with dart side until the compilation completes i think

rainyl commented 1 month ago

Thanks~ I will try to compile and fix it recently.

codecov-commenter commented 1 month ago

Codecov Report

Attention: Patch coverage is 36.25498% with 320 lines in your changes are missing coverage. Please review.

Project coverage is 84.00%. Comparing base (4af53ea) to head (4633325).

Files Patch % Lines
lib/src/ml/svm.dart 8.33% 165 Missing :warning:
lib/src/ml/ann_mlp.dart 10.00% 126 Missing :warning:
lib/src/ml/train_data.dart 83.09% 24 Missing :warning:
lib/src/core/termcriteria.dart 44.44% 5 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #63 +/- ## ========================================== - Coverage 88.97% 84.00% -4.98% ========================================== Files 36 39 +3 Lines 4897 5363 +466 ========================================== + Hits 4357 4505 +148 - Misses 540 858 +318 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rainyl commented 1 month ago
#define CVD_TYPEDEF(TYPE, NAME)                                                                              \
  typedef TYPE *NAME##_CPP;                                                                                  \
  typedef struct NAME {                                                                                      \
    TYPE *ptr;                                                                                               \
  } NAME;

To avoid bare pointer be passed to native code, classes are defined by the above macro, i.e., CVD_TYPEDEF(cv::Mat, Mat) will be expanded to

typedef cv ::Mat *Mat_CPP;
typedef struct Mat {
  cv ::Mat *ptr;
} Mat;

So in a function like CvStatus Mat_CountNonZero(Mat src, int *rval);, the Mat m is actually is a struct with a filed ptr, which ptr is a cv::Mat * rather than cv::Mat, so for functions with cv::Mat as parameters (e.g., int cv::countNonZero(cv::Mat src)), we need to dereference the ptr filed, i.e., cv::countNonZero(*m.ptr)

And for API like cv::ml::ANN_MLP::create() and cv::Stitcher::create(), they will return a cv::Ptr<cv::ml::ANN_MLP>/cv::Ptr<cv::Stitcher>, and cv::Ptr is a wrapper of shared_ptr, I am not very familiar with C++ but it seems the object will be automatically freed when we get the stored pointer by shared_ptr.get() and return it to dart side, that's why I wrapped the cv::Ptr again when implementing cv::stitching

CVD_TYPEDEF(cv::Ptr<cv::Stitcher>, PtrStitcher)
CVD_TYPEDEF(cv::Stitcher, Stitcher)

So, maybe replacing APIs like CvStatus ANN_MLP_SetTrainMethod(ANN_MLP self, int method, double param1, double param2) with CvStatus ANN_MLP_SetTrainMethod(PtrANN_MLP self, int method, double param1, double param2) will improve the performance because get the instance of ANN_MLP at dart side will be unnecessary and there will be less copies.

Hoping the above information can help you~

abdelaziz-mahdy commented 1 month ago

thank you for the fix and the info, i was looking for the memory management too

i was testing with the https://github.com/rainyl/opencv_dart/blob/4af53eabbd7cd077b4f27d77de7b109186b78ec7/lib/src/objdetect/objdetect.dart#L15 an idea of an app, but looks like memory management will need to checked since the memory scales without releasing, i think the same problem will happen in this code so i guess will first implement the binding and all the logic in dart then we can see the best ways to manage memory to avoid this problem in all classes

rainyl commented 1 month ago

Actually, you don't need to worry about memory leaks, NativeFinalizer is used to attach the native resources to the dart class, so when a dart class is GCed, the native resources will also be freed using finalizer (of course, we need correct free steps in C++ code) https://github.com/rainyl/opencv_dart/blob/4af53eabbd7cd077b4f27d77de7b109186b78ec7/lib/src/objdetect/objdetect.dart#L62-L63

abdelaziz-mahdy commented 1 month ago

Actually, you don't need to worry about memory leaks, NativeFinalizer is used to attach the native resources to the dart class, so when a dart class is GCed, the native resources will also be freed using finalizer (of course, we need correct free steps in C++ code) https://github.com/rainyl/opencv_dart/blob/4af53eabbd7cd077b4f27d77de7b109186b78ec7/lib/src/objdetect/objdetect.dart#L62-L63

Well I will check what takes memory then and try to find the cause of the memory consumption, I didn't know that ffi can be auto released that's awesome to know ❤️

rainyl commented 1 month ago

I guess maybe GC is not triggered, you can inspect it in DevTools, in my test, pure dart programs triggered GC less than Flutter and may take several GB memory under heavy tasks, but Flutter doesn't, also, it can be the responsibility of free steps in C++ code, feel free to open new issues if you find any problems 😄

abdelaziz-mahdy commented 1 month ago

I gueese maybe GC is not triggered, you can inspect it in DevTools, in my test, pure dart programs triggered GC less than Flutter and may take several GB memory under heavy tasks, but Flutter doesn't, also, it can be the responsibility of free steps in C++ code, feel free to open new issues if you find any problems 😄

my code went and took 6gb, when refactored it kept going up until 40 gb which then crashed, so i guess that not normal and there is something wrong i am doing. well if i found the problem i will open an issue, for now i will try to finish this pr

rainyl commented 1 month ago

my code went and took 6gb, when refactored it kept going up until 40 gb which then crashed, so i guess that not normal and there is something wrong i am doing. well if i found the problem i will open an issue, for now i will try to finish this pr

I did a quick test and it seems GC works for cv.CascadeClassifier, but I am not expert in dart VM so don't know when GC will be triggered, maybe we can improve the performance according to GC mechanism.

image

Test Code ```dart import 'package:opencv_dart/opencv_dart.dart' as cv; void func(){ final img = cv.imread("test/images/face.jpg", flags: cv.IMREAD_COLOR); final classifier = cv.CascadeClassifier.empty(); classifier.load("test/haarcascade_frontalface_default.xml"); final rects = classifier.detectMultiScale(img); print(rects.length); } void main(List args) { for (var i = 0; i < 1000; i++) { func(); } } ```
abdelaziz-mahdy commented 1 month ago

It may be a problem with my code, so I will try to check again on what takes the amount of ram, thank you for testing it with me

abdelaziz-mahdy commented 1 month ago

I am having a lot of problems with understanding what should be a pointer and what should not, as you can see from the code.

I think a guideline to follow would be the best approach. Either use all pointers or all objects, since mixing them is causing issues. I believe the best approach will be to use all pointers, but I don't understand how to achieve that since the structs create pointers and the type is a pointer, leading to pointers to pointers, which is confusing me.

Can you check any of the files I have made and tell me the best way to move forward?

rainyl commented 1 month ago

I am having a lot of problems with understanding what should be a pointer and what should not, as you can see from the code.

Classes like Stitcher, it seems (not sure) they should be created with static methods like cv::Stitcher::create, but a cv::Ptr will be returned, as I said above, it's a smart pointer so I have to wrap it again with a struct, but you are correct, maybe we should use pointers...

Can you check any of the files I have made and tell me the best way to move forward?

Sure, I will take a look at them.

rainyl commented 1 month ago

image

Maybe this will be better? Passing the wrapped struct to functions directly, no need to get the internal object in cv::Ptr anymore.

abdelaziz-mahdy commented 1 month ago

i dont understand the changes, can you elaborate more ? i see that you used ptrs across all instead of the objects themselves is that correct?

if thats the case, i should make the rest of the dart interfaces using the same structures right? ,

btw i checked on the memory consumption problem on my mac the app takes 6gb of memory eventhough it shows in devtools as 100mb so i think memory are not released correctly

rainyl commented 1 month ago

i see that you used ptrs across all instead of the objects themselves is that correct?

yes, pass the wrapped structs to functions instead of objects, just same as all other classes wrappers.

if thats the case, i should make the rest of the dart interfaces using the same structures right?

right.

but i find that there are lots of works to do to add the ml module, and probably just few users really need it, I even didn't find many examples when writing tests, so I think maybe this work should be a low priority task.

I have no mac so can't test it, I will try to test it on linux, could you please open an issue so we can track it separately?

abdelaziz-mahdy commented 1 month ago

i will write an issue for the memory problem, but for the ml i do agree that yes it should be a lower priority task.

since native assets are experimental, and i do find that the setup command hard to use for most developers, dont you think we should make the cmake download the libs when launching or compiling the app? if you dont mind i can work on it, and leave this pr if other people request it

rainyl commented 1 month ago

make the cmake download the libs when launching or compiling the app?

For windows and linux it is reasonable, but android, ios and macos doesn't use cmake, I am wondering how to make flutter download the libs automatically? writing separate download scripts for gradlea and pod?

abdelaziz-mahdy commented 1 month ago

make the cmake download the libs when launching or compiling the app?

For windows and linux it is reasonable, but android, ios and macos doesn't use cmake, I am wondering how to make flutter download the libs automatically? writing separate download scripts for gradlea and pod?

for android yes gradle

in general we can check this https://github.com/media-kit/media-kit/tree/main/libs in media_kit libs get downloaded on app build

rainyl commented 1 month ago

make the cmake download the libs when launching or compiling the app?

For windows and linux it is reasonable, but android, ios and macos doesn't use cmake, I am wondering how to make flutter download the libs automatically? writing separate download scripts for gradlea and pod?

for android yes gradle

in general we can check this https://github.com/media-kit/media-kit/tree/main/libs in media_kit libs get downloaded on app build

Making sense, I will open a new issue to track it, but I am not familiar with macos and ios development, if you are interest in it, PRs are welcome!