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

Ml module imp #63

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

Conversation

abdelaziz-mahdy
Copy link
Contributor

@abdelaziz-mahdy abdelaziz-mahdy commented May 25, 2024

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
Copy link
Owner

rainyl commented May 25, 2024

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

@codecov-commenter
Copy link

codecov-commenter commented May 25, 2024

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 ⚠️
lib/src/ml/ann_mlp.dart 10.00% 126 Missing ⚠️
lib/src/ml/train_data.dart 83.09% 24 Missing ⚠️
lib/src/core/termcriteria.dart 44.44% 5 Missing ⚠️

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

Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rainyl
Copy link
Owner

rainyl commented May 25, 2024

#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
Copy link
Contributor Author

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

i was testing with the

class CascadeClassifier extends CvStruct<cvg.CascadeClassifier> {
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
Copy link
Owner

rainyl commented May 25, 2024

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)

static final finalizer =
OcvFinalizer<cvg.CascadeClassifierPtr>(CFFI.addresses.CascadeClassifier_Close);

@rainyl rainyl linked an issue May 25, 2024 that may be closed by this pull request
@abdelaziz-mahdy
Copy link
Contributor Author

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)

static final finalizer =
OcvFinalizer<cvg.CascadeClassifierPtr>(CFFI.addresses.CascadeClassifier_Close);

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
Copy link
Owner

rainyl commented May 25, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

rainyl commented May 26, 2024

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
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<String> args) {
  for (var i = 0; i < 1000; i++) {
    func();
  }
}

@abdelaziz-mahdy
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

rainyl commented May 27, 2024

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
Copy link
Owner

rainyl commented May 28, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

rainyl commented May 28, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

rainyl commented May 28, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

rainyl commented May 28, 2024

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!

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.

Add ML module
3 participants