vhost_user_media: Add lens facing control, worker POLLHUP handling, and shmem_unmap fix - #3121
vhost_user_media: Add lens facing control, worker POLLHUP handling, and shmem_unmap fix#3121changyeon-jo wants to merge 4 commits into
Conversation
d99eda1 to
32236db
Compare
Add LensFacing enum and --lens-facing CLI option to v4l2_stream_proxy, aligning it with emulated_camera_mplane and emulated_camera_splane. Implement V4L2 CID_LENS_FACING control in query_ext_ctrl, g_ctrl, and g_ext_ctrls returning the configured lens facing. Add unit tests for CLI argument parsing, lens facing string conversion, and control name padding.
The previous calculation passed len: 1 to shmem_unmap. Update it to unmap the entire allocated range (end - shm_offset + 1).
When a FIFO writer disconnects and the buffer is drained, Linux poll() reports only POLLHUP (without POLLIN). Handling POLLHUP ensures read() is invoked, which consumes any remaining bytes and returns Ok(0) (EOF). This cleanly transitions the worker to WorkerState::Unopened rather than spinning in a 100% CPU poll busy-loop.
f947e43 to
0025092
Compare
bridadan
left a comment
bridadan
left a comment
There was a problem hiding this comment.
Thanks for the fixes! I think the final commit adding multi-instance support could be its own PR, the commits proceeding it are nice standalone fixes that I'd like to see merged.
| flag += ":input_height=" + | ||
| std::to_string(v4l2_stream_proxy.input_height()); | ||
| flag += ":input_fps=" + v4l2_stream_proxy.input_fps(); | ||
| if (!v4l2_stream_proxy.input_path().empty()) { |
There was a problem hiding this comment.
The validation that's done here drops invalid values silently. That's not the desired behavior as all of these options are required by the vhost user v4l2_stream_proxy binary (there are no defaults).
There's already some of this validation being done on CLI arguments here: http://github.com/google/android-cuttlefish/blob/3bd35b0f831649e30e8b22e13557047b4ddf3258/base/cvd/cuttlefish/host/libs/config/media.cpp#L87-L113
@ser-io if configuration is specified in JSON files instead of CLI arguments, are they still validated by the code I linked to above? This is the flow I have in my head, is that accurate?
flowchart TD
A[JSON Config] -->|Serialzied to CLI args| B
D[Terminal] -->|CLI args provided by user| B
B[CLI Arg String] --> C[Split and validated by host/libs/config/media.cpp]
If so, then I'd recommend all the validation should be done in host/libs/config/media.cpp.
There was a problem hiding this comment.
As suggested, I've spun off the fleet/config changes into follow-up PR #3136 and addressed this there:
Removed silent dropping in cf_media_configs.cpp: The if guards have been removed so all fields are serialized unconditionally into the --media flag.
Centralized validation in host/libs/config/media.cpp: Added explicit CF_EXPECT non-empty checks for input_path and input_fps (alongside the existing input_width > 0 and input_height > 0 checks).
| @@ -1360,7 +1360,14 @@ Result<CuttlefishConfig> InitializeCuttlefishConfiguration( | |||
| CF_EXPECT_EQ(media_configs_bindings.size(), 1, | |||
| "Expected a single binding?"); | |||
| auto media_configs = media_configs_bindings[0]->GetConfigs(); | |||
| instance.set_media_configs(media_configs); | |||
| std::vector<CuttlefishConfig::MediaConfig> instance_media_configs; | |||
| for (const auto& config : media_configs) { | |||
| if (!config.instance_index.has_value() || | |||
| config.instance_index.value() == instance_index) { | |||
| instance_media_configs.push_back(config); | |||
| } | |||
| } | |||
| instance.set_media_configs(instance_media_configs); | |||
There was a problem hiding this comment.
Is there a fundamental limitation right now only allowing us to have a single VM use a media config? It seems like it might come from how were using the injector.getMultibindings<...> flow (all the other configs that use it also seem to limit it to 1 binding).
Might be a better question for @ser-io
0025092 to
1da0933
Compare
changyeon-jo
changed the title
|
Thanks @bridadan! I've updated this PR to only contain the 4 standalone driver and vhost-user fixes. I'll open a separate follow-up PR for the multi-instance fleet configuration and validation changes. |
#3136 is created. :) |
3 participants
Summary
This PR contains standalone fixes and enhancements for
vhost_user_mediadrivers:Baseline Formatting (Commit 1):
cargo fmton the baselinev4l2_stream_proxycrate so subsequent functional changes remain strictly scoped and free of cosmetic noise.Lens Facing Control & CLI Tests (Commit 2):
LensFacingenum (FRONT,BACK,EXTERNAL) and--lens-facingCLI option tov4l2_stream_proxy, aligning it withemulated_camera_mplaneandemulated_camera_splane.CID_LENS_FACINGcontrol inquery_ext_ctrl,g_ctrl, andg_ext_ctrlsreturning the configured lens facing to allow the guest Camera HAL to query camera orientation.test_lens_facing_from_str: parsing valid orientations and rejecting invalid ones.test_cmdline_args_default_lens_facing: verifies default isEXTERNAL.test_cmdline_args_custom_lens_facing: verifies--lens-facing BACKoverride.test_ctrl_name_is_nul_padded&test_ctrl_name_truncates_and_stays_nul_terminated: verifies control name formatting.Shmem Unmap Length Calculation (Commit 3):
shmem_unmapmessage length calculation invhu_media/src/lib.rsto unmap the entire allocated region (end - shm_offset + 1) instead of 1 byte.POLLHUP Handling in Worker Loop (Commit 4):
POLLHUPalongsidePOLLINinv4l2_stream_proxy/src/worker.rswhen polling the video stream FIFO. When a FIFO writer disconnects and the buffer is drained, Linuxpoll()reports onlyPOLLHUP(withoutPOLLIN). HandlingPOLLHUPtriggersread(), consuming remaining bytes and receivingOk(0)(EOF) to cleanly transition toWorkerState::Unopenedrather than spinning in a 100% CPU poll busy-loop.(Note: Multi-instance fleet configuration support has been moved to a separate follow-up PR as suggested in review).
Testing
cargo fmt --checkinvhost_user_media/v4l2_stream_proxy(Passed)cargo testinvhost_user_media/v4l2_stream_proxy(Passed, 5/5 unit tests)cargo testinvhost_user_media/vhu_media(Passed)