Add support for Nvidia Jetson V4L2 M2M video encoder - #1161
Conversation
…to dc/exp/video_timing
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
ChangesetThe following package versions will be affected by this PR:
|
…dks into dc/exp/jetson_encode_0527
| nokhwa = { git = "https://github.com/l1npengtul/nokhwa", rev = "4923ecab7cf26f9dba83867a15a9d8662d021296", default-features = false, features = ["input-msmf"] } | ||
|
|
||
| [build-dependencies] | ||
| cc = "1" |
There was a problem hiding this comment.
Use the workspace dependency here (see top-level Cargo.toml). The parallel feature is enabled for better performance
| fallback | ||
| } | ||
|
|
||
| fn find_video_outbound_stats( |
There was a problem hiding this comment.
note: Outside the scope of this PR, but we might want to add some helpers in the core SDK to handle this.
|
|
||
| /// `CLOCK_MONOTONIC` value used to translate Argus sensor timestamps into wall time. | ||
| #[cfg(all(target_os = "linux", target_arch = "aarch64"))] | ||
| fn monotonic_time_ns_now() -> Option<u64> { |
There was a problem hiding this comment.
suggestion (non-blocking): Consider keeping Argus specific helpers in their own module to keep the main module lean.
There was a problem hiding this comment.
moved them to the argus mod
| use std::io; | ||
|
|
||
| /// Opaque handle to an Argus capture session. | ||
| pub struct ArgusCaptureSession { |
There was a problem hiding this comment.
note: While this is probably fine for an example, C/C++ bindings typically live in their own -sys crates (e.g., webrtc-sys, argus-sys) and then provide a higher level Rust wrapper that hides any FFI types. Also outside the scope of this PR, but I do see value in eventually shipping a dedicated crate for this and updating the example to use it.
There was a problem hiding this comment.
good call out, I've created a new project to create new livekit-capture crate and migrate all capture related code there. Will refactor examples once we have that.
| /// Captures a Jetson DMA-buffer backed video frame. | ||
| /// | ||
| /// `pixel_format` is `0` for NV12 and `1` for YUV420M. | ||
| pub fn capture_dmabuf_frame( |
There was a problem hiding this comment.
suggestion: This and the following method should only be compiled for Linux.
| unsafe impl Send for ArgusCaptureSession {} | ||
|
|
||
| extern "C" { | ||
| fn lk_argus_create_session( |
There was a problem hiding this comment.
This example is cool--if I'm tracking the pieces right, couldn't this code be used by consumers in library form (vs example runnable form) to do Argus frame capture? Or is the expectation that user pipelines will do their own Argus interfacing, this is just an example of how it connects with LiveKit?
Or am I not following right?
There was a problem hiding this comment.
yep, we plan on creating a dedicated capture crate that contains all the different platform capture paths. For now this is just inline in the example app so it can run on a jetson. I would expect customers to do their own integration, and they will probably use C++ SDK if they were using libargus anyways.
| int scaled_height) override; | ||
|
|
||
| // DMA buffer accessors | ||
| int dmabuf_fd() const { return dmabuf_fd_; } |
There was a problem hiding this comment.
Nit: in the C++ SDK we aligned all methods to camelBack() case, since this isn't in the SDK not a biggie just for your awareness. If the webrtc stuff does snake_case for base accessors and PascalCase for operations (which it appears to from the override lines above), nothing to change such that it's consistent.
| const ::webrtc::VideoFrame& input_frame, | ||
| bool is_keyframe); | ||
|
|
||
| const webrtc::Environment& env_; |
There was a problem hiding this comment.
Minor: I am generally avoidant of inheritance (esp double inheritance, given this already extends VideoEncoder, but there seems to be a lot of shared state between these _impl classes.
Consider a JetsonVideoEncoder class that extends VideoEncoder which these then extend, or for composition instead, a helper object that defines all the common variables/methods and then is instantiated in each of these. One layer of indirection, but less copy/pasta
| public: | ||
| struct LayerConfig { | ||
| int simulcast_idx = 0; | ||
| int width = -1; |
There was a problem hiding this comment.
Nit: std::optional is more modern/clear than default negative values, but this certainly works given the context of the data, and is more lightweight/C-like
| (!output_is_nv12_ && buffer->n_planes > 2) && | ||
| GetPitchAndHeightFromNvBufSurfaceFd(buffer->planes[2].fd, 2, &v_pitch, &v_h, &v_np); | ||
|
|
||
| auto stride_from_plane = [&](int plane_index, |
There was a problem hiding this comment.
I wonder if this lambda (and similar sections of this method) could be written in a way that don't depend on HW state, such that they could be unit tested
There was a problem hiding this comment.
refactored into a new file, added unit tests
| let jetson_mmapi_include = PathBuf::from("/usr/src/jetson_multimedia_api/include"); | ||
| if jetson_mmapi_include.exists() { | ||
| let jetson_classes_dir = | ||
| PathBuf::from("/usr/src/jetson_multimedia_api/samples/common/classes"); |
There was a problem hiding this comment.
Should we check if this path is valid as well?
There was a problem hiding this comment.
hmm, I wonder if we should just copy the files over and include them in our repo like we do w/ the nvenc related files
There was a problem hiding this comment.
@cloudwebrtc any opinions? I see for nvenc we added NvEncoder.x NvDecoder.x into the repo.
…dks into dc/exp/jetson_encode_0527
uvcorargus