Skip to content

fix(dgw): fix adaptive frame skipping during EOF waits in session shadowing #1678

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
irvingouj@Devolutions (irvingoujAtDevolution) wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix(dgw): fix adaptive frame skipping during EOF waits in session shadowing #1678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
16733fc
fix(dgw): fix adaptive frame skipping during EOF waits in session sha…
irvingoujAtDevolution Feb 13, 2026
9ae0424
fix(dgw): address PR review comments on adaptive frame skip fix
irvingoujAtDevolution Feb 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions crates/video-streamer/src/streamer/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,10 @@ pub fn webm_stream(
}
}

Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MAX_RETRY_COUNT increase from 3 to 25 combined with the 3-second timeout per retry creates a maximum potential wait time of 75 seconds (25 × 3s). This seems excessive for a live streaming scenario. Consider either:

  1. Reducing MAX_RETRY_COUNT to a lower value (e.g., 10 for 30s max wait)
  2. Adding a comment explaining why 75 seconds is an acceptable maximum wait time
  3. Implementing a total elapsed time check instead of just a retry count

The original issue describes a 20-second EOF wait corrupting the ratio, but the fix allows up to 75 seconds of total wait time, which could still cause significant delays in live streaming scenarios.

Suggested change
// NOTE: MAX_RETRY_COUNT is intentionally set to 25. With the 3-second delay
// between retries, this yields a worst-case wait of 75 seconds before we
// give up on the current stream. This is acceptable in our live-streaming
// context because downstream components already enforce a stricter overall
// timeout, and tolerating longer temporary input stalls here reduces
// unnecessary reconnect churn on brief network or encoder hiccups.

Copilot uses AI. Check for mistakes.
const MAX_RETRY_COUNT: usize = 3;
// To make sure we don't retry forever
// Retry is set to 0 when we successfully read a tag
// With a 3-second timeout per EOF wait, 25 retries gives a 75-second maximum stall
// before giving up. The counter resets to 0 on every successful tag read, so this
// only triggers on continuous EOF with zero progress.
const MAX_RETRY_COUNT: usize = 25;
let mut retry_count = 0;

let result = loop {
Expand Down Expand Up @@ -190,6 +191,10 @@ pub fn webm_stream(
},
_ = stop_notifier.notified() => {
let _ = tx.send(WhenEofControlFlow::Break);
},
_ = tokio::time::sleep(std::time::Duration::from_secs(3)) => {
trace!("EOF wait timed out, retrying");
let _ = tx.send(WhenEofControlFlow::Continue);
}
}
});
Expand Down
18 changes: 13 additions & 5 deletions crates/video-streamer/src/streamer/tag_writers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::time::Instant;
use std::time::{Duration, Instant};

use anyhow::Context;
use cadeau::xmf::vpx::{VpxCodec, VpxDecoder, VpxEncoder};
Expand All @@ -14,7 +14,7 @@ use crate::debug::mastroka_spec_name;
const VPX_EFLAG_FORCE_KF: u32 = 0x00000001;

#[cfg(feature = "perf-diagnostics")]
fn duration_as_millis_u64(duration: std::time::Duration) -> u64 {
fn duration_as_millis_u64(duration: Duration) -> u64 {
u64::try_from(duration.as_millis()).unwrap_or(u64::MAX)
}

Expand Down Expand Up @@ -105,7 +105,9 @@ where
last_encoded_abs_time: Option<u64>,

// Adaptive frame skipping state
#[cfg(feature = "perf-diagnostics")]
stream_start: Instant,
processing_time: Duration,
last_ratio: f64,
frames_since_last_encode: u32,
adaptive_frame_skip: bool,
Expand Down Expand Up @@ -192,7 +194,9 @@ where
decoder,
cut_block_state: CutBlockState::HaventMet,
last_encoded_abs_time: None,
#[cfg(feature = "perf-diagnostics")]
stream_start: Instant::now(),
processing_time: Duration::ZERO,
last_ratio: 1.0,
frames_since_last_encode: 0,
adaptive_frame_skip: config.adaptive_frame_skip,
Expand Down Expand Up @@ -263,7 +267,9 @@ where
"VideoBlock created"
);

let processing_started = Instant::now();
self.process_current_block(&video_block)?;
self.processing_time += processing_started.elapsed();

Ok(WriterResult::Continue)
}
Expand Down Expand Up @@ -369,13 +375,15 @@ where
Ok(frame)
}

Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a comment to the current_realtime_ratio method to clarify that it measures the ratio of media time to processing time (excluding idle waits), not media time to wall time. This would help future maintainers understand the distinction, especially given that the fix specifically addresses the issue of idle time corrupting this ratio.

Example:

// Calculates the ratio of media time advanced to active processing time.
// This excludes idle time spent waiting for file growth during EOF retries,
// preventing temporary stalls from permanently corrupting the frame skip decision.
Suggested change
// Calculates the ratio of media time advanced to active processing time.
// This uses `processing_time`, which is intended to exclude idle waits (for example,
// time spent waiting for file growth during EOF retries), so that temporary stalls
// do not permanently corrupt the frame skip decision.

Copilot uses AI. Check for mistakes.
/// Ratio of media time to active processing time (excluding idle waits like EOF retries),
/// so that temporary stalls do not permanently corrupt the frame skip decision.
fn current_realtime_ratio(&self, media_advanced_ms: u64) -> f64 {
#[allow(clippy::cast_possible_truncation)] // u64 max is ~584 million years in ms; no real truncation risk
let wall_ms = self.stream_start.elapsed().as_millis() as u64;
if wall_ms == 0 {
let processing_ms = self.processing_time.as_millis() as u64;
if processing_ms == 0 {
1.0
} else {
media_advanced_ms as f64 / wall_ms as f64
media_advanced_ms as f64 / processing_ms as f64
}
}

Expand Down