emulated_camera_mplane: Implement frame rate control by chihchiachen · Pull Request #2990 · google/android-cuttlefish

chihchiachen · 2026-08-07T22:17:05Z

Decouple frame generation from the vhost-user command thread into a dedicated background worker thread to enforce a 30 FPS frame rate while maintaining non-blocking V4L2 ioctl behavior and v4l2-compliance.

Bug: b/534454250
Test: android.hardware.cts.CameraTest#testPreviewFpsRange

jmacnak · 2026-08-10T19:38:50Z

+        let mut timer_fd = match TimerFd::new() {
+            Ok(t) => t,
+            Err(e) => {
+                log::error!("Failed to create TimerFd for worker thread: {}", e);


Less familiar with the media libs so feel free to push back on this:

Can these log messages be incorprated into the Error type? Separating the log from the error drops context and potentially makes it more difficult for readers to understand failures because they then have to try to scan logs and guess which logs are actual errors vs just informational.

Could this use something like

use thiserror::Error; #[derive(Debug, Error)] enum WorkerError { #[error("Failed to initialize timer: {0}")] FailedToInitializeTimer(#[from] io::Error), ... } #[derive(Debug, Error)] enum EmulatedCameraError { #[error("Failed to initialize worker: {0}")] FailedToInitializeWorker(#[from] WorkerError), ... }

or perhaps anyhow::Error if you never expect anyone to try to handle individual failures differently:

let mut timer_fd = match TimerFd::new() { Ok(t) => t, Err(e) => { init_tx.send(Err(anyhow!("failed to initialize timer fd: {}", e)).unwrap(); return; } ... let worker_handle = spawn_frame_worker( session_id, Arc::clone(&shared_state), Arc::clone(&wakeup_evt), Arc::clone(&exit_evt), Arc::clone(&self.evt_queue), ).context("Failed to spawn worker thread")?;

Both of these options effectively let you context and the errors become effectively

"Failed to do virtio-media: failed to start new emulate camera session: failed to start worker thread: failed to create timer fd"

I'll take a approach that's similar to your the second suggestion. PTAL.

jmacnak · 2026-08-10T19:39:48Z

+            Ok(p) => p,
+            Err(e) => {
+                log::error!("Failed to create PollContext for worker thread: {}", e);
+                let _ = init_tx.send(Err(e.errno()));


Recommend unwrap() on these. We do not expect error passing from the worker thread to ever really fail so panic'ing seems appropriate here.

How about moving the init work before the thread creation? This way we can return early from the init errors and bubble up the logs more easily. PTAL.

jmacnak · 2026-08-10T19:48:55Z

+                Ok(ev) => ev,
+                Err(e) => {
+                    log::error!("PollContext wait failed in worker thread: {}", e);
+                    break;


Again feel free to push back:

Should these non-initialization errors also be propagated back? Potentially you could do std::thread::JoinHandle<Result<(), ERROR_TYPE> potentially and then the owner of the worker thread could potentially check

if worker_handle.is_finished() { let error = worker_handle.join(); ... propagate worker exiting unexpectedly early error ... }

I used JoinHandle to propagate fatal errors and a mpsc channel to bubble up non-fatal errors/warnings. PTAL.

bridadan · 2026-08-10T17:12:50Z

+                };
+
+                for plane in &mut buffer.planes {
+                    let _ = plane.fd.as_file().seek(SeekFrom::Start(0));


If this is a result, could you log a warning if the seek fails (or send it back over the channel as @jmacnak is suggesting)?

I added a mpsc channel to bubble up non-fatal errors/warnings. PTAL.

bridadan · 2026-08-10T19:26:11Z

-        if self.active_session != Some(session.id) {
-            return;
+        if self.active_session == Some(session.id) {
+            self.active_session = None;


I don't think this check is exactly equivalent? But perhaps it doesn't matter here, I'm not sure what the invariant for self.active_session is supposed to be in this context. There's no early return path any more though, does that still need to be present?

It doesn't really harm, but let's remove the irrelevant change. Please check out my next patch.

bridadan · 2026-08-10T20:25:17Z

-            self.iteration += 1;
+            // Re-acquire lock to verify stream_count and dispatch DequeueBuffer:
+            let mut guard = shared.lock().unwrap();
+            if guard.stream_count == stream_count {


I don't fully understand why stream_count has to be checked instead of checking if streaming == false. Aren't they the same condition?

stream_count is needed to guard the edge case where the stream goes on->off->on during write_pattern().

bridadan · 2026-08-10T20:28:15Z

+        for buffer in state.buffers.iter_mut() {
            buffer.set_state(BufferState::New, self.width, self.height);
        }
+        let _ = session.wakeup_evt.write(1);


Could we log or unwrap() these writes so they don't fail silently? I see this pattern a few times in this file.

Sure - let me fix them. Thanks.

bridadan · 2026-08-10T20:29:43Z

            self.width = req_width;
            self.height = req_height;
+            let mut state = session.state.lock().unwrap();
+            state.width = req_width;
+            state.height = req_height;


It feels odd to have this duplicated between self and session.state. Can they be consolidated?

Good catch. Since width and height are moved to struct SessionSharedState, we should remove them from struct EmulatedCameraSession. Let me fix it.

(Correction: I'll remove width and height from struct EmulatedCamera, not struct EmulatedCameraSession.)

It turns out v4l2-compliance requires control/width/height to be global (not session-specific) states. Let's move those parameters to a new data struct DeviceSharedState.

bridadan · 2026-08-10T20:30:34Z

                                self.controls.gain = gain;
+                                {
+                                    let mut state = session.state.lock().unwrap();
+                                    state.controls.gain = gain;


Same here, it feels odd to have this duplicated between self and session.state. Can they be consolidated?

Thanks - I'll remove controls from struct EmulatedCamera.

It turns out v4l2-compliance requires control/width/height to be global (not session-specific) states. Let's move those parameters to a new data struct DeviceSharedState.

Decouple frame generation from the vhost-user command thread into a dedicated background worker thread to enforce a 30 FPS frame rate while maintaining non-blocking V4L2 ioctl behavior and v4l2-compliance. Bug: b/534454250 Test: atest CtsCameraTestCases:\ android.hardware.cts.CameraTest#testPreviewFpsRange

chihchiachen force-pushed the limit-frame-rate branch from 41c4d92 to e80078b Compare August 7, 2026 22:55

chihchiachen requested review from bridadan and jmacnak August 7, 2026 22:59

chihchiachen added the kokoro:run Run e2e tests. label Aug 10, 2026

GoogleCuttlefishTesterBot removed the kokoro:run Run e2e tests. label Aug 10, 2026

jmacnak reviewed Aug 10, 2026

View reviewed changes

Comment thread base/cvd/cuttlefish/host/commands/vhost_user_media/emulated_camera_mplane/src/device.rs Outdated

Comment thread base/cvd/cuttlefish/host/commands/vhost_user_media/emulated_camera_mplane/src/device.rs Outdated

chihchiachen force-pushed the limit-frame-rate branch from e80078b to 486c018 Compare August 10, 2026 18:19

chihchiachen requested a review from jmacnak August 10, 2026 18:23

jmacnak reviewed Aug 10, 2026

View reviewed changes

bridadan reviewed Aug 10, 2026

View reviewed changes

chihchiachen force-pushed the limit-frame-rate branch 4 times, most recently from 1c6c065 to edc4bff Compare August 11, 2026 21:49

chihchiachen requested review from bridadan and jmacnak August 11, 2026 22:00

chihchiachen force-pushed the limit-frame-rate branch from edc4bff to 095f1bc Compare August 11, 2026 22:33

chihchiachen force-pushed the limit-frame-rate branch from 095f1bc to 08e0393 Compare August 11, 2026 23:28

Uh oh!

Conversation

chihchiachen commented Aug 7, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chihchiachen Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chihchiachen Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chihchiachen Aug 11, 2026 •

edited

Loading

chihchiachen Aug 11, 2026 •

edited

Loading