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

async object detect #115

Merged
merged 23 commits into from
Jun 28, 2024
Merged

async object detect #115

merged 23 commits into from
Jun 28, 2024

Conversation

abdelaziz-mahdy
Copy link
Contributor

can you check the code and let me know if there something i am doing wrong, i am getting many errors in dart file, i was going to fix those, but i want to be sure my impl of c++ is correct before moving on to dart fully

@rainyl
Copy link
Owner

rainyl commented Jun 25, 2024

Ok, I will take a look.

@abdelaziz-mahdy
Copy link
Contributor Author

abdelaziz-mahdy commented Jun 25, 2024

thank you for the mentioned fixes, i am now stuck in dart some function params i cant understand, and ffi size and the other size are confusing me. can you check and point me to the right direction?

after fixing dart file will make the test cases to make sure all is implemented and good to go

@rainyl
Copy link
Owner

rainyl commented Jun 25, 2024

ffi.Size actually represents sizt_t, i.e., platform specific integers, I remember that ony several functions use it. so just ignore it.

cv::Size was wrapped with a struct in c header, and in dart it used to be typedef Size=(int, int), now it is wrapped with a class, but in order to keep compatibility, most functions were kept using (int, int).

Hope the above informations can help you. Also, please let me know if you have any suggestions.😄

@abdelaziz-mahdy
Copy link
Contributor Author

ffi.Size actually represents sizt_t, i.e., platform specific integers, I remember that ony several functions use it. so just ignore it.

cv::Size was wrapped with a struct in c header, and in dart it used to be typedef Size=(int, int), now it is wrapped with a class, but in order to keep compatibility, most functions were kept using (int, int).

Hope the above informations can help you. Also, please let me know if you have any suggestions.😄

well i think thats the right way to do it, so all good, if i found a better way will let you know even though i think you did the best way

will wait for you to check the current dart impl, i always get lost with pointers and ref so sometimes as you can see i fail to do it correctly

@abdelaziz-mahdy abdelaziz-mahdy changed the title initial async object detect work async object detect Jun 25, 2024
@rainyl
Copy link
Owner

rainyl commented Jun 25, 2024

I will check them later, but I am still considering how to organize async APIs, for functions they can be placed in a new file and it's fine, but for classes like HOGDescriptor, only one class name is allowed in dart, if sync and async are split to TWO files (like the objdetect_async.dart now), users have to import them separately.

So, the best solution I can think of is that, for functions, split to TWO files, but for classes, split every/several class to a single file and place sync and async in one class (one file), e.g., split HOGDescriptor to hogdescriptor.dart, and implement both sync and async inside it.

If you have better solutions, let's discuss it.

BTW, another solution is to split to different packages, like opencv_core, opencv_dnn, opencv_imgproc, etc., but it's laborious...

@abdelaziz-mahdy
Copy link
Contributor Author

abdelaziz-mahdy commented Jun 25, 2024

I will check them later, but I am still considering how to organize async APIs, for functions they can be placed in a new file and it's fine, but for classes like HOGDescriptor, only one class name is allowed in dart, if sync and async are split to TWO files (like the objdetect_async.dart now), users have to import them separately.

So, the best solution I can think of is that, for functions, split to TWO files, but for classes, split every/several class to a single file and place sync and async in one class (one file), e.g., split HOGDescriptor to hogdescriptor.dart, and implement both sync and async inside it.

If you have better solutions, let's discuss it.

BTW, another solution is to split to different packages, like opencv_core, opencv_dnn, opencv_imgproc, etc., but it's laborious...

different packages will be alot of work and i dont think its needed

lets think about which will be easier for the users
do you want them to be same class name? cant we do it like this

      cascade_classifier.dart       // Sync version
      cascade_classifier_async.dart // Async version

with classes being

class CascadeClassifier
class CascadeClassifierAsync

i think the separation will make it easier to maintain and use, but the problem that sync and async cant be used in the same class, like if an instance got created async user cant use sync methods, is that a use case we should cover

also cant we use part imports? to have sync and async separated but using one class can use them both? i dont know if that good or not but its an option

      cascade_classifier.dart        // Main class
      cascade_classifier_sync.dart   // Sync methods
      cascade_classifier_async.dart  // Async methods
lib/src/objdetect/cascade_classifier.dart:

library cv;

import 'dart:ffi' as ffi;
import 'package:ffi/ffi.dart';
import '../core/base.dart';
import '../core/mat.dart';
import '../core/rect.dart';
import '../core/size.dart';
import '../opencv.g.dart' as cvg;

part 'cascade_classifier_sync.dart';
part 'cascade_classifier_async.dart';

class CascadeClassifier extends CvStruct<cvg.CascadeClassifier> {
  CascadeClassifier._(cvg.CascadeClassifierPtr ptr, [bool attach = true])
      : super.fromPointer(ptr) {
    if (attach) {
      finalizer.attach(this, ptr.cast(), detach: this);
    }
  }
part of 'cascade_classifier.dart';

extension CascadeClassifierSync on CascadeClassifier {
  bool load(String name) {
    final cname = name.toNativeUtf8().cast<ffi.Char>();
    final p = calloc<ffi.Int>();
    cvRun(() => CFFI.CascadeClassifier_Load(ref, cname, p));
    calloc.free(cname);
    return p.value != 0;
  }

part of 'cascade_classifier.dart';

extension CascadeClassifierAsync on CascadeClassifier {
  Future<bool> loadAsync(String name) async {
    final cname = name.toNativeUtf8().cast<ffi.Char>();
    final rval = cvRunAsync<bool>(
        (callback) => CFFI.CascadeClassifier_Load_Async(ref, cname, callback),
        (c, p) {

@rainyl
Copy link
Owner

rainyl commented Jun 26, 2024

i think the separation will make it easier to maintain and use, but the problem that sync and async cant be used in the same class, like if an instance got created async user cant use sync methods, is that a use case we should cover

I personally think we should, I will be inflexible if users have to import both sync and async version of the same API. so keep sync and async available in same class name is better.

cascade_classifier.dart        // Main class
cascade_classifier_sync.dart   // Sync methods
cascade_classifier_async.dart  // Async methods

Oh, this looks better, I will test it in-depth.

Thanks for your insightful suggestions!

@abdelaziz-mahdy
Copy link
Contributor Author

thank you for the example, i was able to fix most of the errors only one remains since it returns 4 params and there is no run_async4

@abdelaziz-mahdy
Copy link
Contributor Author

i added runasync4 and fixed the error, should i implement test cases or wait for you to have a look?

@abdelaziz-mahdy
Copy link
Contributor Author

Should I free values coming from callbacks too?

@abdelaziz-mahdy
Copy link
Contributor Author

async constructors is done too

Copy link
Owner

@rainyl rainyl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Almost there, I think it's time to write test 😄

@abdelaziz-mahdy
Copy link
Contributor Author

should tests be separated or the same files test sync and async?

@rainyl
Copy link
Owner

rainyl commented Jun 27, 2024

should tests be separated or the same files test sync and async?

The separated way is better for maintenance, but I am just lazy so placed them in one file, do as what you like.

@abdelaziz-mahdy
Copy link
Contributor Author

I see you implemented the objdetect tests, so I should just implement the dnn one?

@rainyl
Copy link
Owner

rainyl commented Jun 28, 2024

Yes, I just have some free time so finished it, you can implement the tests for dnn

@abdelaziz-mahdy
Copy link
Contributor Author

Yes, I just have some free time so finished it, you can implement the tests for dnn

Ok, will do it, just a question why is the callback API not blocking and the normal calls blocking, even though both run the same c code instead of returns it calls the callback?

Can you explain it more to me, I see that flutter does it the same for java platform interface for async tasks too, but why it's not blocking

Will appreciate if there is a reference

@rainyl
Copy link
Owner

rainyl commented Jun 28, 2024

Ok, will do it, just a question why is the callback API not blocking and the normal calls blocking, even though both run the same c code instead of returns it calls the callback?

Can you explain it more to me, I see that flutter does it the same for java platform interface for async tasks too, but why it's not blocking

It's basically the credits to NativeCallable.listener, this API provided by dart::ffi also uses SendPort and ReceivePort internally, and it ensures the callback will be called in the future at the same isolate where the callback was created, and the pointers will be passed to that isolate too, so we can consume the pointers in that isolate.

The normal sync functions are called in the main isolate, as we know, dart is single-thread, the synchronized functions will be called sequentially so once some functions are time-consuming, the main isolate will be blocked.

However, because the pointers are unsendable across isolates, so if a Mat is constructed inside an isolate, it still can't be sent to another isolate, e.g., the following is ERROR:

import 'dart:isolate';

import 'package:opencv_dart/opencv_dart.dart' as cv;

void main(List<String> args) async {
  final im = await Isolate.run<cv.Mat>(() async => cv.imreadAsync("test/images/circles.jpg"));

  print(im.shape);  // ERROR!
}

However, because the callback will be called in the future, so it won't block the current/main isolate, with async/await, main isolate can process other instructions, for Flutter, this will NOT block main/UI thread so the app is still interactive.

@abdelaziz-mahdy
Copy link
Contributor Author

abdelaziz-mahdy commented Jun 28, 2024

wow, interested to see if it can use multicores or not, but in most cases async is much better due to its simple usage vs isolates, but will need to further test it

i checked the api it launches isolate and runs the function then closes the isolate, i think we will need to mention there is a max number of isolates then ui will get blocked
dart-lang/sdk#51261 i dont know if that will happen using the NativeCallable.listener or not, but a check will make it known

i added dnn_test but mat operations needs to be migrated to async
ones i need but not migrated

minMaxLoc
convertTo

i guess that should be the next step migrating core.h and core.cpp to async

@rainyl
Copy link
Owner

rainyl commented Jun 28, 2024

i added dnn_test but mat operations needs to be migrated to async ones i need but not migrated

minMaxLoc
convertTo

i guess that should be the next step migrating core.h and core.cpp to async

Yes, they are next steps, for now just kepp using sync version.

@abdelaziz-mahdy
Copy link
Contributor Author

done, i am missing the models, so i cant test the code, dont you think a script to download all the needed models and running the tests is needed? also i workflow to make sure all tests are passing, since i cant find it (correct me if it does exist )

@rainyl
Copy link
Owner

rainyl commented Jun 28, 2024

The models are located at https://github.com/rainyl/opencv_dart/releases/tag/dnn_test_files, the coverage workflow will download them, so don't worry~

BTW, I have fixed some string conversion issues, now I think it's time to merge

@rainyl rainyl merged commit a757c28 into rainyl:async Jun 28, 2024
1 check passed
@abdelaziz-mahdy
Copy link
Contributor Author

Let me know what you plan to work on, so I work on something else, if you want to give me a module I don't mind

I learned alot of ffi doing this so I am enjoying the learning experience and your explanation is awesome so thank you very much.

@rainyl
Copy link
Owner

rainyl commented Jun 28, 2024

Let's open new issues to track and assign them.

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.

2 participants