Fix native memory leaks#2511
Conversation
There was a problem hiding this comment.
Should this variable be made class scope and reused instead?
This comment goes for everywhere we see "new mat*" inside of functions
There was a problem hiding this comment.
Doing so would affect the thread safety of calling the methods that use the class variable. I'm not sure if this is a real concern for any of our classes though.
There was a problem hiding this comment.
All of our pipes and pipelines should* only be called by a single thread. Or that was the design intent at least
(Asterisk for some very specific parts of camera calibration. TryFinishCalibration is called from a web server thread)
14f8c3b to
5710f34
Compare
5710f34 to
4c9ab96
Compare
|
Both native and JVM memory usage went down slightly. |
| mat.convertTo(intersectMatA, CvType.CV_32F); | ||
| secondContour.mat.convertTo(intersectMatB, CvType.CV_32F); |
There was a problem hiding this comment.
what was the intent of the old code explicitly converting to float here?
There was a problem hiding this comment.
The implementation of getMat2f is almost identical to what was done here, except the converted mat is cached. Both this code and getMat2f were added in 1149bf9,
There was a problem hiding this comment.
Pull request overview
This PR reduces native-memory leaks across PhotonVision’s Java/OpenCV stack by consistently releasing OpenCV Mat/MatOf* resources, making pipelines/pipes/modules releasable/closable, and updating tests/utilities to use try-with-resources where applicable.
Changes:
- Centralize and expand native resource cleanup (pipes/pipelines/modules), including adding/overriding
release()implementations andAutoCloseableusage. - Fix/avoid OpenCV native allocations leaking in simulation and test code by explicitly calling
release()and using try-with-resources. - Update calibration/PnP helpers and build metadata (including
mrcalVersion) in support of improved memory hygiene.
Reviewed changes
Copilot reviewed 79 out of 80 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| photon-targeting/src/main/java/org/photonvision/estimation/VisionEstimation.java | Switch pose estimation undistortion to shared helper. |
| photon-targeting/src/main/java/org/photonvision/estimation/OpenCVHelp.java | Reduce temporary Mat allocations; adjust PnP helpers and undistortion flow. |
| photon-lib/src/main/java/org/photonvision/simulation/VideoSimUtil.java | Release simulation Mats and add shutdown cleanup for static tag images. |
| photon-lib/src/main/java/org/photonvision/simulation/SimCameraProperties.java | Release temporary contour Mat in contour-area computation. |
| photon-core/src/test/java/org/photonvision/vision/target/TrackedTargetTest.java | Ensure TrackedTarget is closed in tests. |
| photon-core/src/test/java/org/photonvision/vision/target/TargetCalculationsTest.java | Release OpenCV objects created in tests. |
| photon-core/src/test/java/org/photonvision/vision/pipeline/SolvePNPTest.java | Convert pipeline/provider/result usage to try-with-resources; refactor benchmark helper usage. |
| photon-core/src/test/java/org/photonvision/vision/pipeline/ReflectivePipelineTest.java | Convert pipeline/provider/result usage to try-with-resources. |
| photon-core/src/test/java/org/photonvision/vision/pipeline/MaxDetectionsTest.java | Convert pipeline/provider/result usage to try-with-resources. |
| photon-core/src/test/java/org/photonvision/vision/pipeline/ColoredShapePipelineTest.java | Refactor helpers to use FrameProvider and close results/providers properly. |
| photon-core/src/test/java/org/photonvision/vision/pipeline/CirclePNPTest.java | Convert pipeline/provider/result usage to try-with-resources. |
| photon-core/src/test/java/org/photonvision/vision/pipeline/Calibrate3dPipeTest.java | Close pipeline results and reduce test-side native leaks during calibration runs. |
| photon-core/src/test/java/org/photonvision/vision/pipeline/ArucoPipelineTest.java | Close pipeline/provider/result/target resources and extract values for assertions. |
| photon-core/src/test/java/org/photonvision/vision/pipeline/AprilTagTest.java | Convert pipeline/provider/result usage to try-with-resources. |
| photon-core/src/test/java/org/photonvision/vision/opencv/ContourTest.java | Use try-with-resources for Contour to ensure native cleanup. |
| photon-core/src/test/java/org/photonvision/vision/frame/provider/FileFrameProviderTest.java | Close provider/frame resources in tests. |
| photon-core/src/test/java/org/photonvision/vision/frame/consumer/FileSaveFrameConsumerTest.java | Close provider/frame resources in tests. |
| photon-core/src/test/java/org/photonvision/hardware/HardwareConfigTest.java | Close hardware factory via try-with-resources. |
| photon-core/src/test/java/org/photonvision/common/ShapeBenchmarkTest.java | Reuse shared benchmark helper and ensure providers are closed. |
| photon-core/src/test/java/org/photonvision/common/configuration/SQLConfigTest.java | Disable CVMat debug printing during tests to reduce noise. |
| photon-core/src/test/java/org/photonvision/common/BenchmarkTest.java | Make benchmark helper reusable and ensure results/frames are released during warmup. |
| photon-core/src/main/java/org/photonvision/vision/target/TrackedTarget.java | Release temporary rvec/tvec Mats and additional cached polygon resources. |
| photon-core/src/main/java/org/photonvision/vision/target/TargetCalculations.java | Reuse a static Mat for undistortion and add shutdown cleanup hook. |
| photon-core/src/main/java/org/photonvision/vision/processes/VisionSourceManager.java | Implement AutoCloseable and attempt shutdown cleanup hook wiring. |
| photon-core/src/main/java/org/photonvision/vision/processes/VisionRunner.java | Implement AutoCloseable (close frame supplier). |
| photon-core/src/main/java/org/photonvision/vision/processes/VisionModuleManager.java | Implement AutoCloseable to close child modules. |
| photon-core/src/main/java/org/photonvision/vision/processes/VisionModule.java | Implement AutoCloseable to close module resources. |
| photon-core/src/main/java/org/photonvision/vision/processes/PipelineManager.java | Implement AutoCloseable to release pipelines on shutdown. |
| photon-core/src/main/java/org/photonvision/vision/pipeline/ReflectivePipeline.java | Ensure child pipes are released in release(). |
| photon-core/src/main/java/org/photonvision/vision/pipeline/OutputStreamPipeline.java | Implement Releasable and release all internal pipes. |
| photon-core/src/main/java/org/photonvision/vision/pipeline/ObjectDetectionPipeline.java | Release additional pipes to prevent leaks. |
| photon-core/src/main/java/org/photonvision/vision/pipeline/FocusPipeline.java | Avoid unnecessary Mat allocations and implement proper release(). |
| photon-core/src/main/java/org/photonvision/vision/pipeline/DriverModePipeline.java | Implement proper release() for child pipes. |
| photon-core/src/main/java/org/photonvision/vision/pipeline/CVPipeline.java | Clarify release expectations in documentation. |
| photon-core/src/main/java/org/photonvision/vision/pipeline/ColoredShapePipeline.java | Fix pipe naming and release all child pipes. |
| photon-core/src/main/java/org/photonvision/vision/pipeline/Calibrate3dPipeline.java | Release snapshot observations when removed/released. |
| photon-core/src/main/java/org/photonvision/vision/pipeline/AprilTagPipeline.java | Release additional pipes in release(). |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/SpeckleRejectPipe.java | Add no-op release() override for uniform lifecycle. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/SortContoursPipe.java | Add no-op release() override for uniform lifecycle. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/SolvePNPPipe.java | Release cached image-points Mat. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/RotateImagePipe.java | Add no-op release() override for uniform lifecycle. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/ResizeImagePipe.java | Add no-op release() override for uniform lifecycle. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/OutputMatPipe.java | Add no-op release() override for uniform lifecycle. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/ObjectDetectionPipe.java | Ensure detector is released via release(). |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/MultiTargetPNPPipe.java | Add no-op release() override for uniform lifecycle. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/HSVPipe.java | Add no-op release() override for uniform lifecycle. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/GroupContoursPipe.java | Add no-op release() override for uniform lifecycle. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/GrayscalePipe.java | Add no-op release() override for uniform lifecycle. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/FocusPipe.java | Release internal Mats used for mean/stddev computation. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/FindPolygonPipe.java | Release temporary approximation Mat to avoid leaks. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/FindContoursPipe.java | Reuse hierarchy Mat and release contours/hierarchy on pipe release. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/FindCirclesPipe.java | Release internal circles Mat. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/FindBoardCornersPipe.java | Refactor board-corner detection; cache object points and add release paths. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/FilterShapesPipe.java | Add no-op release() override for uniform lifecycle. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/FilterObjectDetectionsPipe.java | Add no-op release() override for uniform lifecycle. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/FilterContoursPipe.java | Add no-op release() override for uniform lifecycle. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/DrawCalibrationPipe.java | Add no-op release() override for uniform lifecycle. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/Draw3dTargetsPipe.java | Reuse internal Mats and release them in release(). |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/Draw2dTargetsPipe.java | Reuse internal Mats and release them in release(). |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/Draw2dCrosshairPipe.java | Add no-op release() override for uniform lifecycle. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/CornerDetectionPipe.java | Release cached polygon output Mat. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/Collect2dTargetsPipe.java | Add no-op release() override for uniform lifecycle. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/Calibrate3dPipe.java | Refactor calibration data flow and ensure internal Mats are released. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/CalculateFPSPipe.java | Add no-op release() override for uniform lifecycle. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/ArucoPoseEstimatorPipe.java | Release cached object-points Mat and clean up resources. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/ArucoDetectionPipe.java | Reuse/release corner Mats during refinement. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/AprilTagPoseEstimatorPipe.java | Reuse/release corner Mats during undistortion. |
| photon-core/src/main/java/org/photonvision/vision/pipe/impl/AprilTagDetectionPipe.java | Align with shared Releasable lifecycle patterns. |
| photon-core/src/main/java/org/photonvision/vision/pipe/CVPipe.java | Make all pipes Releasable to standardize lifecycle management. |
| photon-core/src/main/java/org/photonvision/vision/opencv/Releasable.java | Make Releasable extend AutoCloseable with default close() -> release(). |
| photon-core/src/main/java/org/photonvision/vision/opencv/Contour.java | Reduce temporary Mat allocations and adjust helper visibility. |
| photon-core/src/main/java/org/photonvision/vision/objects/RubikObjectDetector.java | Reuse/release letterboxed Mat to prevent per-call leaks. |
| photon-core/src/main/java/org/photonvision/vision/objects/RknnObjectDetector.java | Reuse/release letterboxed Mat to prevent per-call leaks. |
| photon-core/src/main/java/org/photonvision/vision/frame/StaticFrames.java | Add shutdown hook to release static Mats. |
| photon-core/src/main/java/org/photonvision/vision/frame/provider/FileFrameProvider.java | Release raw loaded image on failure to avoid leaks. |
| photon-core/src/main/java/org/photonvision/vision/camera/FileVisionSource.java | Avoid leaking a frame when reading static properties during construction. |
| photon-core/src/main/java/org/photonvision/vision/calibration/JsonMatOfDouble.java | Release wrapped Mat when present. |
| photon-core/src/main/java/org/photonvision/vision/aruco/PhotonArucoDetector.java | Clear cached corner mats after detection to reduce retained native objects. |
| photon-core/src/main/java/org/photonvision/common/util/TestUtils.java | Improve headless handling and move continuous-run helper to shared util. |
| build.gradle | Update mrcalVersion to a newer dev build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Override | ||
| public void close() { | ||
| pipelineManager.close(); | ||
| visionSource.close(); | ||
| visionRunner.close(); | ||
| lastPipelineResultBestTarget.close(); | ||
| } |
There was a problem hiding this comment.
Stop() being a prereq to close()ing things makes sense to me
| mat.convertTo(intersectMatA, CvType.CV_32F); | ||
| secondContour.mat.convertTo(intersectMatB, CvType.CV_32F); |
There was a problem hiding this comment.
The implementation of getMat2f is almost identical to what was done here, except the converted mat is cached. Both this code and getMat2f were added in 1149bf9,
| public void start() { | ||
| visionSource.cameraConfiguration.deactivated = false; | ||
| visionRunner.startProcess(); | ||
| streamRunnable.start(); | ||
| } | ||
|
|
||
| public void stop() { | ||
| visionSource.cameraConfiguration.deactivated = true; | ||
| visionRunner.stopProcess(); | ||
|
|
||
| try { | ||
| streamRunnable.interrupt(); | ||
| streamRunnable.join(); | ||
| } catch (InterruptedException e) { | ||
| logger.error("Exception killing process thread", e); | ||
| } | ||
|
|
||
| visionSource.release(); | ||
|
|
||
| inputVideoStreamer.close(); | ||
| outputVideoStreamer.close(); | ||
| inputFrameSaver.close(); | ||
| outputFrameSaver.close(); | ||
|
|
||
| changeSubscriberHandle.stop(); | ||
| setVisionLEDs(false); | ||
| } |
There was a problem hiding this comment.
stop handled releasing some resources already. It makes more sense to me to move at least some of this to AutoClosable's close. I'm debating if all of it can be moved, and start can be shifted to the constructor.
There was a problem hiding this comment.
It looks like we never actually stop/start the same VisionModule? The "Deactivate" button destroys the VisionModule, and reactivateDisabledCameraConfig recreates it using VisionModuleManager::addSource which creates a new VisionModule. Let's consolidate.
e35c600 to
d551c6b
Compare
d551c6b to
559db6d
Compare
Unfortunately, this adds complexity instead of reducing it, but there still seems to be some redundancy that can be simplified later.
559db6d to
3312ea1
Compare

Description
This fixes the many native memory leaks of OpenCV's matrixes. While I can't guarantee I got everything, it is substantially better than it was. The test results are now a lot cleaner. Blocked on PhotonVision/mrcal-java#28.
Meta
Merge checklist: