Add new virtio-media V4l2 stream virtual device - #2935
Conversation
ser-io
left a comment
There was a problem hiding this comment.
Let's avoid forking emulated_camera_mplane as the first commit, it makes the second commit very hard to review. Feel free to introduce the new device in the first commit.
0d284fd to
752ec67
Compare
The previous implementation of the send_events function used all but one descriptor because the iterator was advanced through the entire collection of descriptors. Then only one descriptor is released back. This limits the throughput for devices and makes them more susceptible to jitter due to system load. This implementation only advances the descriptor iterator by one, leaving the rest unused. Bug: 472497998 Assisted-by: Jetski:Gemini 3.5 Flash
This adds the implementation of the v4l2_stream_proxy device, which reads from a FIFO and behaves as a virtio-media device. worker_thread_loop in v4l2_stream_proxy uses nix::poll and nix::sys::eventfd to wait for both FIFO data and control signals (Stop/BufferQueued) without busy looping or sleeping. Bug: 472497998 Assisted-by: Jetski:Gemini 3.5 Flash
fc32f68 to
4bdd8f5
Compare
This adds all of the necessary flags and configuration to launch the v4l2_stream_proxy virtual device from the cvd cli. Bug: 472497998 Assisted-by: Jetski:Gemini 3.5 Flash
Adds TestV4l2StreamProxyCompliance to e2e tests, which: 1. Creates a FIFO on the host. 2. Starts a goroutine to write dummy YUV420M frames to the FIFO. 3. Launches CVD with v4l2_stream_proxy pointing to the FIFO. 4. Finds the video node in the guest. 5. Runs v4l2-compliance on the guest. Bug: 472497998 Assisted-by: Jetski:Gemini 3.5 Flash
|
|
||
| fn default_fmt(&self, queue: QueueType) -> v4l2_format { | ||
| let plane_sizes = self.config.format.plane_sizes(self.config.input_width, self.config.input_height); | ||
| if self.config.format.is_multiplanar() { |
There was a problem hiding this comment.
should we support only multiplanar for now. then make the case for single plane in a different PR?
There was a problem hiding this comment.
As per your previous request, the only format supported is YUV420 multiplanar. I can go ahead remove all of the logic that detects if it is single or multiplanar and just assume everything is multiplanar for now.
| " 'input_path': required for v4l2_stream_proxy, path to the host named pipe\n" | ||
| " 'input_width': required for v4l2_stream_proxy, width of the video stream in pixels\n" | ||
| " 'input_height': required for v4l2_stream_proxy, height of the video stream in pixels\n" | ||
| " 'input_fps': required for v4l2_stream_proxy, frames per second (e.g., 30 or 30000/1001)\n" |
There was a problem hiding this comment.
as these new properties are only relevant to the new v4l2_stream_proxy type. let's group this new fileds into its own block. we can append a new block like the following:
<EMPTY LINE>
"v4l2_stream_proxy properties:\n"
" 'input_path': [required] path to the host named pipe"
" 'input_width': [required] width of the video stream in pixels"
...
You keep the new "v4l2_stream_proxy" entry under the "Supported types:\n" block.
There was a problem hiding this comment.
The current format is:
Supported types:
<all the types>
Supported keys for v4l2_emulated_camera_splane, v4l2_emulated_camera_mplane, and v4l2_proxy:
lens_facing: ...
Supported keys for v4l2_stream_proxy:
<all the other ones>
To clarify, are you requesting the following format instead?
Supported types:
<all the existing types>
'v4l2_stream_proxy': stream video from a host named pipe into the guest
v4l2_stream_proxy properties:
<input_*>
Supported keys:
lens_facing: ...
One thing to note: lens_facing support in v4l2_stream_proxy was removed based on your previous comment. If the second version is what you're requesting, how would you like to note that lens_facing isn't supported?
There was a problem hiding this comment.
Yes, let's do:
Supported types:
<all the existing types>
'v4l2_stream_proxy': stream video from a host named pipe into the guest
v4l2_stream_proxy properties:
<input_*>
Skip "lens_facing" property in this commit/PR as it was not introduced here and it's confusing. I'll add it later in a follow up PR.
| } | ||
| } | ||
|
|
||
| func TestV4l2StreamProxyCompliance(t *testing.T) { |
There was a problem hiding this comment.
Let's create a new directory for v4l2 stream proxy device e2e tests. Let's move this new test to file: /media_tests/v4l2_stream_proxy/v4l2_compliance/main_test.go
There was a problem hiding this comment.
Do you in general want to keep the emulated tests separated from the stream proxy tests? Would you want to do something like:
- All existing tests, move to
/media_tests/v4l2_emulated_camera- e.g.
/media_tests/v4l2_emulated_camera/cts,/media_tests/v4l2_emulated_camera/v4l2_compliance
- e.g.
- This new test, move to
/media_tests/v4l2_stream_proxy- e.g.
/media_tests/v4l2_stream_proxy/v4l2_compliance
- e.g.
There was a problem hiding this comment.
Tests that are aimed to act on an specific virtio-media device should have it's own directory. In this case we should have /media_tests/v4l2_emulated_camera_mplane and /media_tests/v4l2_stream_proxy for the v4l2 compliance tests. However the cts tests should stay under /media_tests/cts (for now) as they don't target an specific virtio-media device but the phone as a whole. W
In this PR, let's focus only on the new /media_tests/v4l2_stream_proxy/ directory. I'll take care of /media_tests/v4l2_emulated_camera_mplane later.
| } | ||
| Err(e) => { | ||
| log::error!("Failed to open FIFO: {:?}", e); | ||
| std::thread::sleep(std::time::Duration::from_millis(500)); |
There was a problem hiding this comment.
why is this needed? sleep calls lead to flakiness, let's explain why it's needed and whether the timtout value should be part of the "config: &Config"
There was a problem hiding this comment.
The sleep was added here to prevent a fast loop in case of continuous errors, which should keep system load down. But agreed it can lead to inconsistency. I just didn't have a better idea at the time of implementation.
All of the sleeps in this file are in the error path (except for the one I noted below). Perhaps we should just remove them and see how things play out during testing? I agree they cause some concern while reading the code. We can always revisit once we see some real error behavior in the wild.
| last_frame_time: Instant, | ||
| } | ||
|
|
||
| enum WorkerCmd { |
There was a problem hiding this comment.
let's move WorkerCmd, WorkerHandle, WorkerState and other worker related functionalties into its own file and package. I'd like to have a clear view of the public interface of the worker package and how the VirtioMediaDevice implementation interacts with it.
There was a problem hiding this comment.
"its own file and package".
Do you mean just its own file? Or do you mean I need to create a whole other crate? I would push back on the separate crate as its only used by the v4l2_stream_proxy crate right now.
There was a problem hiding this comment.
Not another create. In Rust the term is module, looking forward to encapsulate the worker logic into it's own module separated from the v4l2 device implementation. We can have this module implementation into its own file.
mod worker {
}
There was a problem hiding this comment.
Clarified offline that a new file alone is sufficient
| match fifo_file.read(&mut local_buf[..read_chunk]) { | ||
| Ok(0) => { | ||
| log::info!("FIFO EOF, writer disconnected. Re-opening..."); | ||
| std::thread::sleep(std::time::Duration::from_millis(100)); |
There was a problem hiding this comment.
Note to self, this sleep should be removed
This PR introduces the
v4l2_streamdevice, avhost-user-mediabackend that enables streaming video from a host-side source (via a FIFO/named pipe) into a guest VM using thevirtio-mediaprotocol.It allows video data generated on the host (e.g., from a video file, synthetic test source, or any other producer of raw video frames) to be streamed directly into the guest VM's V4L2 framework.
The first commit copies over the
emulated_camera_mplanecrate into the newv4l2_streamcrate, and includes minimal changes to get it building and launchable. The second commit modifies the new crate with the new functionality.b/472497998