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

[wpiutil] Add a RawFrame JNI overload for byte[] #7179

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

spacey-sooty
Copy link
Contributor

@spacey-sooty spacey-sooty commented Oct 10, 2024

Allows avoiding two copies in #7176

cc @mcm001

@spacey-sooty spacey-sooty force-pushed the rawframe-byte-array branch 4 times, most recently from 49c9164 to 7c0b158 Compare October 10, 2024 13:28
@spacey-sooty
Copy link
Contributor Author

/format

@spacey-sooty spacey-sooty force-pushed the rawframe-byte-array branch 2 times, most recently from 992a56b to d2caa5e Compare October 10, 2024 13:38
Allows avoiding two copies in wpilibsuite#7176

Signed-off-by: Jade Turner <[email protected]>
@spacey-sooty
Copy link
Contributor Author

spacey-sooty commented Oct 10, 2024

If desired an overload which just takes the Mat.dataAddr return shouldn't be too hard here but I believe this should fix the issue with extra copies as it keeps the byte[] overload #7176 adds but removes the copies incurred.

@spacey-sooty spacey-sooty marked this pull request as ready for review October 10, 2024 13:58
@mcm001
Copy link
Contributor

mcm001 commented Oct 10, 2024

Yeah this gets rid of one copy, but for use with opencv mats we need an overload that takes (long pData, int width/height/stride/format). Not sure if there's any other implications for making a RawFrame which doesn't own the underlying array.

@spacey-sooty
Copy link
Contributor Author

Yeah this gets rid of one copy, but for use with opencv mats we need an overload that takes (long pData, int width/height/stride/format). Not sure if there's any other implications for making a RawFrame which doesn't own the underlying array.

Hmm Peter raised concerns about that, how bad would the performance implications of the copy be in PhotonVision usecase? Is there any way to use RawFrame over Mat there?

@mcm001
Copy link
Contributor

mcm001 commented Oct 11, 2024

I don't think that the Mat constructor that takes a pointer to C++ backed data is accessible. Also cscore's USBCamera gives us a Mat right? Not a RawFrame? I will need to convert one to the other at some point, and it's not workable to incur an extra copy at any step (it's on the order of ms on embedded devices)

@mcm001
Copy link
Contributor

mcm001 commented Oct 11, 2024

What's the concern with making RawFrame not own the underlying data? Opencv's mats let you do this.

@spacey-sooty
Copy link
Contributor Author

I don't think that the Mat constructor that takes a pointer to C++ backed data is accessible. Also cscore's USBCamera gives us a Mat right? Not a RawFrame? I will need to convert one to the other at some point, and it's not workable to incur an extra copy at any step (it's on the order of ms on embedded devices)

We could change USBCamera to give a raw frame

@mcm001
Copy link
Contributor

mcm001 commented Oct 11, 2024

If we do that, someone should provide a way to get that data into a opencv Mat without extra copies happening. Thats possible in photon via our new JNI capabilities, I guess, if we wanna commit to nuking opencv entirely.

@Gold856
Copy link
Contributor

Gold856 commented Oct 11, 2024

USBCamera doesn't give a Mat by itself, it's instead added as a source to a CvSink, and the Mat is pulled from that. We could instead use a RawSink, which will allow us to use a RawFrame instead of a Mat.

As for the dangers of taking the pointer stored in the Mat and using it in RawFrame, Mat uses finalize to clean up, so the data should last until the Mat gets GCed. For what we're doing, I think this is long enough?

@mcm001
Copy link
Contributor

mcm001 commented Oct 11, 2024

It looks like the Java matrix wrapping the RawMat being released isn't a concern? See the release javadoc claim: https://docs.opencv.org/3.4/d3/d63/classcv_1_1Mat.html#ae48d4913285518e2c21a3457017e716e

If the matrix header points to an external data set (see Mat::Mat ), the reference counter is NULL, and the method has no effect in this case.

Also in Mat::Mat:

The external data is not automatically deallocated, so you should take care of it.

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.

4 participants