Refactor emulated_camera to use a shared video_capture_device library#2653
Refactor emulated_camera to use a shared video_capture_device library#2653bridadan wants to merge 3 commits into
Conversation
ser-io
left a comment
ser-io
left a comment
There was a problem hiding this comment.
Thanks for this needed refactoring.
Can you split the PR into two commits (parts)
- Refactor code that's clearly duplicated among the two devices implemention without
CaptureDeviceFormatabstraction. - Introduce the new CaptureDeviceFormat abstraction and the refactored code around it.
I'd like to analyze the new abstraction of a commit of its own. Thanks.
bridadan
force-pushed
the
dedup_virtio_media_code
branch
from
774f514 to
3ba52ec
Compare
|
@ser-io Here's an attempt at splitting up the changes. The |
| } | ||
| } | ||
|
|
||
| pub fn enum_framesizes( |
There was a problem hiding this comment.
This helper function is not extensible. If I were to add a new frame size to my device implementation, I cannot reuse this function.
Let's keep this responsibility on the device implementation side.
There was a problem hiding this comment.
I've removed this helper function, but I did keep a shared implementation in CaptureDeviceFormat since the code only differs on the constant values of each device type at this time.
If you still would prefer to keep it entirely in the device implementation let me know and I can move it.
| }) | ||
| } | ||
|
|
||
| pub fn enum_frameintervals( |
Extract shared code from `emulated_camera_mplane` and `emulated_camera_splane` into a new `devices` module of `vhu_media`. This version extracts only obviously duplicated code (structs, enums, and helper functions). Key changes: - Created `devices` module in `vhu_media` library containing: - `Buffer` and `BufferState` implementations. - Refactored `emulated_camera_mplane` and `emulated_camera_splane` to: - Depend on the new `devices` module. - Use the shared `Buffer` and `BufferState`. - Updated Cargo.toml and BUILD.bazel files to reflect new dependencies. TAG=agy CONV=b4f9ffcf-c393-4dbc-b8fb-5c38416291c2 Bug: 519646531 Test: cd base/cvd && bazel build cuttlefish/package:cvd
…ptureDeviceFormat trait
Extract `EmulatedCamera` and `EmulatedCameraSession` structs and their
`VirtioMediaDevice` and `VirtioMediaIoctlHandler` implementations into the shared
`vhu_media` library to eliminate duplication.
Introduce the `CaptureDeviceFormat` trait in the shared library to abstract
format-specific differences between `emulated_camera_mplane` and
`emulated_camera_splane`.
Key changes:
- Created `CaptureDeviceFormat` trait in `vhu_media`.
- Moved `Buffer`, `BufferState`, `EmulatedCameraSession`, and `EmulatedCamera`
to `vhu_media`, making the camera and session generic over
`F: CaptureDeviceFormat`.
- Moved and unified `VirtioMediaDevice` and `VirtioMediaIoctlHandler`
implementations to `vhu_media`.
- Refactored `emulated_camera_mplane` and `emulated_camera_splane` to:
- Implement `CaptureDeviceFormat` for `MplaneFormat` and `SplaneFormat` respectively.
- Implement `default_fmt` and `write_pattern` directly within the trait
impl blocks.
- Instantiate the generic `EmulatedCamera` in `main.rs`.
TAG=agy
CONV=b4f9ffcf-c393-4dbc-b8fb-5c38416291c2
Bug: 519646531
Test: cd base/cvd && bazel build cuttlefish/package:cvd
# Repeat with v4l2_emulated_camera_mplane
cvd create -media=type=v4l2_emulated_camera_splane
# Ensure pixel format changes with each type
v4l2-ctl -d1 --all
# Ensure it passes with no failures
v4l2-compliance -d1 -s
bridadan
force-pushed
the
dedup_virtio_media_code
branch
from
3ba52ec to
33297c9
Compare
Bug: 519646531
Test: cd base/cvd && bazel build cuttlefish/package:cvd
# Repeat with v4l2_emulated_camera_mplane
cvd create -media=type=v4l2_emulated_camera_splane
# Ensure pixel format changes with each type
v4l2-ctl -d1 --all
# Ensure it passes with no failures
v4l2-compliance -d1 -s
bridadan
force-pushed
the
dedup_virtio_media_code
branch
from
33297c9 to
7c6c405
Compare
| use virtio_media::memfd::MemFdBuffer; | ||
|
|
||
|
|
||
| #[derive(Debug, PartialEq, Eq, Clone, Copy)] |
There was a problem hiding this comment.
This Buffer implementaion is now specific to the emulated_camera_splane device only, see #2670. Let's keep under emulated_camera_splane until it's relevant to move it to a common place, for example, if there's another single planar format device that needs it.
| use vhost_user_backend::VhostUserDaemon; | ||
| use vhu_media::VhuMediaBackend; | ||
| use vhu_media::cli::Error; | ||
| use vhu_media::cli::{CmdLineArgs, Config, Error, init_logging, Result}; |
There was a problem hiding this comment.
Let's use cli::Result, cli::Error and cli::init_logging down below and don't import these strct/cuntions directly. Result and Error are very common terms, let's use the cli prefix for a better reading.
|
|
||
| type Result<T> = std::result::Result<T, Error>; | ||
|
|
||
| #[derive(Parser, Debug)] |
There was a problem hiding this comment.
Keep CmdLineArgs and Config per binary.
2 participants
Extract shared code from
emulated_camera_mplaneandemulated_camera_splaneinto a new shared libraryvideo_capture_device. This removes a large amount of copy-pasted code between the two emulated camera implementations.Key changes:
video_capture_devicelibrary containing:CaptureDeviceFormattrait defining format-specific constants and methods.EmulatedCameraandEmulatedCameraSessionstructs implementingVirtioMediaDeviceandVirtioMediaIoctlHandler.BufferandBufferStateimplementations.configure_planesin the trait, as it was identical for both devices.emulated_camera_mplaneandemulated_camera_splaneto:video_capture_devicelibrary.CaptureDeviceFormatfor their respective format structs (MplaneFormatandSplaneFormat).EmulatedCamerato the generic one.TAG=agy
CONV=b4f9ffcf-c393-4dbc-b8fb-5c38416291c2
Bug: 519646531
Test: build and launch with both device types, ensure v4l2-compliance
passes in the guest