Skip to content

Feature: Unsafe version of imencode() that does not copy #304

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

Closed
Levi-Lesches opened this issue Dec 5, 2024 · 6 comments · Fixed by #305
Closed

Feature: Unsafe version of imencode() that does not copy #304

Levi-Lesches opened this issue Dec 5, 2024 · 6 comments · Fixed by #305
Labels
enhancement New feature or request

Comments

@Levi-Lesches
Copy link

Is your feature request related to a problem? Please describe.
The imencode() function has a line like this:

final vec = VecUChar.fromPointer(buffer);
final u8List = vec.toU8List(); // will copy data
vec.dispose();
return (success, u8List);

I'm writing a program that tries to read from 8+ cameras, on a Raspberry Pi 5, at ~30 FPS. My old implementation passed a (Pointer<Uint8> data, int length) directly between isolates to avoid copying, but now I'm afraid this will introduce latency from copying all those frames.

Quick math: (8 cameras) * (30 frames / second / camera) * (60,000 bytes / frame) = 14 Mb / sec.

Describe the solution you'd like
A function like the following could return the pointer itself, allowing me to pass just a single integer across isolates and send the data from native memory, avoiding all copies entirely, so long as I dispose of everything properly.

(bool, VecUChar) imencodeUnsafe(...);

Describe alternatives you've considered
On a Windows laptop with just one integrated webcam, the current code works fine. I have yet to test this on on my 8+ cameras on a Raspberry Pi 5, so I imagine that copying will introduce unacceptable delays. I am happy to open a PR if you'd like.

It might be worth considering a breaking change in the future to make this the default behavior. Similar to VideoCapture.read() which returns a Mat, I think it's okay to let the caller dispose of their data manually, especially if it means avoiding some major copying.

Additional context

This might help my use case even more: I recently discovered @pragma('vm:deeply-immutable'), which allows some objects to not be copied between isolates, but shared. As far as I know, a Uint8List does not qualify, but if I use vec.ptr.address, that will work, and I can eliminate all copying across isolates (9+ isolates) entirely!

@Levi-Lesches Levi-Lesches added the enhancement New feature or request label Dec 5, 2024
@rainyl
Copy link
Owner

rainyl commented Dec 5, 2024

@Levi-Lesches Yes, it makes sense, so does imdecode.

And, actually the data will be copied 2 times, since cv::imencode only accepts std::vector<uchar> but VecUChar is a struct consisted of a pointer and a size_t length. Apparently this can also be improved:

https://github.com/rainyl/dartcv/blob/7bd81ecc6273087926761a0e6e64791e3fecb1d7/dartcv/core/types.h#L82-L87

This might help my use case even more: I recently discovered @pragma('vm:deeply-immutable'), which allows some objects to not be copied between isolates, but shared. As far as I know, a Uint8List does not qualify, but if I use vec.ptr.address, that will work, and I can eliminate all copying across isolates (9+ isolates) entirely!

Will look into this in the future.
Related:

@Levi-Lesches
Copy link
Author

Levi-Lesches commented Dec 5, 2024

Can't you pass a vector to imencode and send its .data() pointer over FFI? That should also result in no copying

Like, change the function vecuchar_cpp2c to return a struct of vector.data() and vector.size()

@rainyl
Copy link
Owner

rainyl commented Dec 5, 2024

Yes it's applicable, I just didn't take it into consideration when designing, that's what I said can be improved. 😢

However I have no much time to reconstruct the vector related wrappers recently, it would be nice if you can work on this. 😄

@Levi-Lesches
Copy link
Author

Sounds good, I'll submit a PR soon

@rainyl rainyl reopened this Dec 6, 2024
@Levi-Lesches
Copy link
Author

Oh, thanks for doing #305 for us! So all that's left for me to do is change all the vecXXX_cpp2c to be non-copying, right?

@rainyl
Copy link
Owner

rainyl commented Dec 6, 2024

Oh, thanks for doing #305 for us! So all that's left for me to do is change all the vecXXX_cpp2c to be non-copying, right?

Exactly. But it should be done in dartcv, so please submit PR there, we can keep tracking at rainyl/dartcv#14

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

Successfully merging a pull request may close this issue.

2 participants