[cscore] Sink: add ability to get most recent frame instead of waiting #7572
Conversation
|
Pre-patch: With my patch: Perf from main, and patched: (I'm not sure why they are so different)
|
|
I think we’d want this to be a new function. I feel like the determinism of the existing function in the face of running too long is worth having in most cases. |
mcm001
force-pushed
the
cscore-any-new-frame
branch
from
23279e0 to
18abf66
Compare
wpilibcExamples/build.gradle
Outdated
| targetBuildTypes 'debug' | ||
| targetBuildTypes 'release' |
wpilibcExamples/src/main/cpp/examples/AprilTagsVision/cpp/Robot.cpp
Outdated
Show resolved
Hide resolved
cameraserver/src/main/java/edu/wpi/first/vision/VisionRunner.java
Outdated
Show resolved
Hide resolved
| @@ -64,9 +73,12 @@ CS_Sink CreateRawSinkCallback(std::string_view name, bool isCv, | |||
|
|
|||
| void PutSourceFrame(CS_Source source, const WPI_RawFrame& image, | |||
| CS_Status* status); | |||
| uint64_t GrabSinkFrame(CS_Sink sink, WPI_RawFrame& image, CS_Status* status); | |||
|
|
|||
| // TODO - default parameters need to go last, is this OK? | |||
mcm001
force-pushed
the
cscore-any-new-frame
branch
from
4a98816 to
f76c92f
Compare
wpilibcExamples/src/main/cpp/examples/AprilTagsVision/cpp/Robot.cpp
Outdated
Show resolved
Hide resolved
@ThadHouse The old behavior should be the default except where I've modified examples - should I revert those changes? |
|
I've added a basic smoketest for Java. I don't have a full set of requirements so I focused on making sure that major functionality still works. I could see an argument for why a Java test will sufficiently exercise the C++ api too, since the JNI calls into |
| * use the current time | ||
| * @return Frame time, or 0 on error (call GetError() to obtain the error message) | ||
| */ | ||
| public long grabFrame(Mat image, long lastFrameTime) { |
There was a problem hiding this comment.
Because of the way this creates potentially confusing overloads (passing long vs double has quite different meaning here), I think we should name the lastFrameTime functions differently rather than overloading.
There was a problem hiding this comment.
I can do this pretty easily in Java land - C++ might be harder without introducing some code duplication. Do we want to expose a normal and a last frame time version of every function, or only the ones I actually need?
There was a problem hiding this comment.
Maybe just the ones you need for now? This probably needs a bigger refactor to avoid duplication than we want to do right now.
There was a problem hiding this comment.
Sounds good, I can make that happen. It'd also be nice to think about how we can avoid having to go from a native RawFrame -> NewDirectByteBuffer -> CvMat from the bytebuffer -> copy the mat into the user-provided one if we're already providing a parallel API, too
There was a problem hiding this comment.
We could give the native function a native pointer, size, stride, etc, to copy into, but the problem is how to handle the case when that doesn’t match the image size it has available, and it still makes a copy of course.
The other approach would be allowing zero copy by returning an object pointing to the internal RawFrame native buffer, and calling close() on that object returns the native buffer to the pool. This puts the onus on the user to do so or have a memory leak.
There was a problem hiding this comment.
Updated the the API to expose a new function for what we need, leaving everything else untouched.
mcm001
force-pushed
the
cscore-any-new-frame
branch
from
32a67e4 to
8690aa8
Compare
PeterJohnson
changed the title
Pipes the frame time we want to wait for a new frame based on up through cscore. This allows more control for Photon over when frames are dropped or awaited.
https://discord.com/channels/176186766946992128/368993897495527424/1320182740330745896 for context