feat(vdo): Add safe Rust bindings for VDO API - #223
Conversation
Safe wrappers over vdo-sys with builder pattern for stream creation, iterator-based frame capture, and automatic resource cleanup. Tested on device: YUV, JPEG, H.264, H.265 formats work correctly.
There was a problem hiding this comment.
Pull request overview
This PR adds safe Rust bindings for the VDO (Video Capture) API, enabling video capture from Axis cameras. The implementation provides a builder pattern for stream configuration, iterator-based frame capture, and automatic resource cleanup through RAII principles.
Key changes:
- New
vdocrate with safe abstractions overvdo-sysFFI bindings - Comprehensive error handling with detailed error code mapping
- Builder pattern for ergonomic stream configuration
- Updated
vdo_encode_clientapplication demonstrating multiple video formats
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/vdo/src/lib.rs | Core library implementation with safe wrappers for VDO API, including Stream, StreamBuffer, Frame types, and comprehensive tests |
| crates/vdo/examples/basic.rs | Simple example demonstrating video stream capture with YUV format |
| crates/vdo/Cargo.toml | Package configuration for the new vdo crate with device-tests feature flag |
| apps/vdo_encode_client/src/main.rs | Updated example application testing multiple video formats (YUV, JPEG, H.264, H.265) |
| apps/vdo_encode_client/Cargo.toml | Updated dependency from vdo-sys to the new vdo crate |
| Cargo.toml | Added vdo workspace member |
| Cargo.lock | Updated lock file with vdo crate dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I'll wait for feedback from the maintainers before addressing these suggestions, as some may be more relevant than others for this codebase. |
|
I won't have time to look at the code before new year, but I would like to say already now that I'm happy you are interested in carrying on the work that Jonathan started 🙂 |
apljungquist
left a comment
There was a problem hiding this comment.
I skimmed the PR and I think it looks pretty good. I'll try to do a more thorough read in the before next weekend focusing on safety and I'll try to respond to comments within 24h.
There was a problem hiding this comment.
Some API design related questions.
API design is difficult and typically subjective. I don't want it to stall this PR, so consider this inspiration. (that's not to say that the previous review was not subjective or disputable, only that this is even more so)
|
I created this to help with my understanding of VDO, posting it here in case you find it usefule: https://github.com/apljungquist/acap-rs/blob/f778576222874210e51f0c46ab4d6ffbdd877572/crates/vdo-sys/README.md |
apljungquist
left a comment
There was a problem hiding this comment.
I have focused this review on the Map type, which looks good. Once we have decided if/how to change it I will continue reviewing other parts.
I do this to not overwhelm you with comments and to not overwhelm myself with different discussions to keep track of and large diffs that need to be reviewed with every now commit.
Address all review comments. Fix GObject leak in Stream::drop. - Consume-by-value ownership: start(self), stop(self), unref(self) - Replace Iterator with next_buffer() -> Result - Merge Frame into StreamBuffer (VdoBuffer = VdoFrame) - Extract Map to separate module, keys as &CStr - Add Resolution enum, remove buffer_strategy, rename to custom_timestamp_us - Replace as_mut_slice with data_copy(), file_descriptor() returns Result Tested: cargo clippy clean, 33/33 tests pass on Artpec-9 device.
2326701 to
70e2962
Compare
|
@apljungquist @guoxe |
|
Will try to get around to this tomorrow |
Address all review comments. Fix GObject leak in Stream::drop. - Consume-by-value ownership: start(self), stop(self), unref(self) - Replace Iterator with next_buffer() -> Result - Merge Frame into StreamBuffer (VdoBuffer = VdoFrame) - Extract Map to separate module, keys as &CStr - Add Resolution enum, remove buffer_strategy, rename to custom_timestamp_us - Replace as_mut_slice with data_copy(), file_descriptor() returns Result Tested: cargo clippy clean, 33/33 tests pass on Artpec-9 device.
70e2962 to
2326701
Compare
|
@vsem-azamat nice progress! Please refrain from force pushing in the future as it makes it difficult to review the changes and keep track of comments. I took the liberty of cherry-picking your fix onto the branch before it was force pushed and force pushing that, so now the git history looks as it should. A few of my comments seem to have gone unaddressed. You don't have to always to things the way I think is the best, but I would appreciate if you at least acknowledge on each comment that you have read it and state what, if anything, you will do to address it 🙂 I haven't seen the results of the CI yet, but if we get more revisions in on this I will make sure to start it as soon as I can instead of waiting until I have time to do human review like this time around. |
|
It looks like something changed with github actions and it now hangs after starting the dev container: I will try to look into this in the next week or so. In the meanwhile I encourage you to run the same check locally so that we have a good PR ready to go when CI gets fixed. The check that compares checksums can be a bit unreliable and is easy to fix, so if that is the only one that is failing we should be all right. |
…ct-test Replace `use vdo_sys::*` with explicit type imports and `vdo_sys::`-qualified function/constant calls at call sites. This addresses the PR review feedback about avoiding glob imports. Also adds the missing VDO_ERROR_NO_VIDEO error code and adopts expect-test for display/formatting tests.
- Clamp size to capacity in data_copy to prevent out-of-bounds reads - Replace debug_assert with assert in unsafe constructors (from_ptr, from_raw) - Document BorrowedFd lifetime constraints on file_descriptor() - Fix double vdo_stream_stop by delegating to Drop - Add Error::InvalidFd instead of overloading NullPointer for bad fds - Return Option<usize> from header_size() for negative (no header) values - Fix from_gerror safety comment to match actual code order - Add Debug for Map and CStringPtr, Deref<Target=CStr> for CStringPtr - Use explicit ptr::null::<c_char>(), comment as_ptr GLib convention
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
apljungquist
left a comment
There was a problem hiding this comment.
Some nice improvements in this iteration. But there are still old comments that remain unaddressed. You may want to also take at least a quick look at Gustaf's comments as he he the person who will ultimately merge your PR if approved 😉
|
I took the liberty of fixing a couple of outstanding issues, I hope that's ok. |
Sure. Thanks) |
|
@guoxe |
…fety Remove async load/run APIs (overengineering for typical load-once + sync inference workflow). Fix Send soundness for Tensors and JobRequest by storing raw connection pointer instead of &Connection, avoiding the Connection: Sync requirement. Apply PR AxisCommunications#223 review feedback: rename Map::new to try_new, add SAFETY comments on unsafe blocks, fix error handling to use from_utf8_lossy. Harden Drop impls to free larodError even on success, fix panic safety in devices()/models() array handling, add #[must_use] to OwnedTensorPtrs, fix PhantomData invariance in JobRequest.
guoxe
left a comment
There was a problem hiding this comment.
I have some questions relating to some of the functions added.
Overall I think it looks good!
|
Hi! |
guoxe
left a comment
There was a problem hiding this comment.
Nice work!
Sorry it took a while for me to get around to reviewing, unless there is something else you want to address I am happy with merging this PR as is.
|
@guoxe |
Thanks you so much for for helping and reviewing this PR. It was tough for me. I'd like to merge this PR and in anything appears I'll open new FollowUp MR. Thanks) |
|
I'm happy you were up for the challenge and that it hasn't discouraged you :) |
…fety Remove async load/run APIs (overengineering for typical load-once + sync inference workflow). Fix Send soundness for Tensors and JobRequest by storing raw connection pointer instead of &Connection, avoiding the Connection: Sync requirement. Apply PR AxisCommunications#223 review feedback: rename Map::new to try_new, add SAFETY comments on unsafe blocks, fix error handling to use from_utf8_lossy. Harden Drop impls to free larodError even on success, fix panic safety in devices()/models() array handling, add #[must_use] to OwnedTensorPtrs, fix PhantomData invariance in JobRequest.
Safe Rust bindings for VDO (Video Capture) API
Safe wrappers over
vdo-syswith builder pattern for stream creation, iterator-based frame capture, and automatic resource cleanup.Tested on device: YUV, JPEG, H.264, H.265 formats work correctly.
Describe your changes
This PR adds a new
vdocrate providing safe Rust bindings for the VDO API, enabling video capture from Axis cameras.Based on the work from PR #153 by @JsGjKJzi — completed the implementation, added comprehensive tests, and updated the example application.
Key features:
StreamBuilderwith builder pattern for configuring video streamsStream,RunningStream,StreamIteratorfor lifecycle managementStreamBufferandFramefor zero-copy frame accessDropimplementationsVdoErrorincluding error code namesChanges:
crates/vdo/crate with safe bindingsapps/vdo_encode_client/to use the newvdocratedevice-testsfeature)Issue ticket number and link
Checklist before requesting a review