From 1e792f191cf24af49b609494cba72390ec2f24cd Mon Sep 17 00:00:00 2001 From: dailz Date: Sat, 20 Jun 2026 22:15:50 +0800 Subject: [PATCH] fix(state_portal): use webrtc_thread.is_some() for PTS mode gating (really fixes #24) Previous commit 079611a claimed to fix #24 but the gating condition was wrong, making the entire fix dead code: src/state_portal.rs:467 - let pts = if self.webrtc.is_some() { // ALWAYS false here + let pts = if self.webrtc_thread.is_some() { // correct lifecycle check Why self.webrtc was wrong: WebRtcState lifecycle in Portal path: 1. StatePortal::new() sets self.webrtc = Some(...) if args.port > 0 2. First frame arrives -> WaitingForFormat branch 3. state_portal.rs:274 does self.webrtc.take() and moves WebRtcState into the webrtc thread 4. Subsequent frames -> Streaming branch -> handle_pw_frame 5. By this point self.webrtc is None So my gating check 'if self.webrtc.is_some()' at handle_pw_frame ALWAYS returned false, and compute_capture_pts was NEVER called. Confirmed by debug instrumentation showing 0 invocations across a 174s WebRTC session. Net effect: #24's PTS fix was completely inert. RTP timestamps were still computed from sequential frame counter (old broken behavior). User reports of 'latency got worse' were due to other test conditions, not the dead code. The correct check is self.webrtc_thread.is_some() because: - webrtc_thread is set AFTER WebRtcState is moved into it (line 301) - webrtc_thread stays Some for the entire WebRTC session - webrtc_thread is None for MP4 mode (no thread spawned) So this check correctly distinguishes WebRTC mode from MP4 mode at the point where PTS is computed for each frame in handle_pw_frame. Lesson learned: - Oracle review (rounds 1 and 2) verified the design and code structure but did not catch the lifecycle issue because they reasoned about the code statically. - Runtime verification via debug instrumentation was needed to confirm the function was never called. - This is why the user ran the test BEFORE I committed - their feedback that latency got worse was the canary that exposed the dead code. Verification plan (next user test): - Run with --stats and confirm rtp= field in write_h264 debug logs shows VARIABLE jumps (not uniform 3000 increments) - During static periods (capture_fps < 5), rtp should jump by 30000+ - During active periods (capture_fps > 30), rtp increments may still look sequential due to 1/fps time_base quantization (acceptable) - Browser jitter buffer should stabilize at < 500ms --- src/state_portal.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/state_portal.rs b/src/state_portal.rs index 47f80c8..e895ef6 100644 --- a/src/state_portal.rs +++ b/src/state_portal.rs @@ -464,7 +464,7 @@ impl StatePortal { // damage-driven variable fps — issue #24). MP4: keep sequential counter; // file output doesn't need real-time PTS and changing it would alter // playback speed during static periods. - let pts = if self.webrtc.is_some() { + let pts = if self.webrtc_thread.is_some() { self.compute_capture_pts(frame.pts) } else { self.frames_encoded as i64 @@ -579,7 +579,8 @@ impl StatePortal { 0 }; let ticks_i128 = (relative_ns.saturating_mul(i128::from(self.args.fps))) / NS_PER_SEC; - let mut pts = i64::try_from(ticks_i128).unwrap_or(i64::MAX); + let computed_pts = i64::try_from(ticks_i128).unwrap_or(i64::MAX); + let mut pts = computed_pts; if let Some(last) = self.last_pts_emitted { if pts <= last {