From 3fb722a31c9b1a1849fcb78623366af612616321 Mon Sep 17 00:00:00 2001 From: "Eduardo M. Rezende" Date: Sun, 2 Aug 2026 14:16:55 -0300 Subject: [PATCH 1/3] fix(compositor): live preview holds the current frame instead of consuming one per tick Free-running preview playback (and the poc-d3d harness) decoded exactly one real frame per 1/60s tick, assuming a constant ~60fps source. ScreenCaptureKit (and equivalent screen captures) only emits a frame when the screen changes, so a recording with long static stretches could contain only a few hundred real frames over its whole duration. Consuming one frame per tick regardless exhausted the stream long before elapsed wall time reached the recording's duration, so the decoder hit EOF, looped back to the start, and the preview appeared to accelerate then jump back to the beginning. Adds a peek/commit lookahead (peek_next_time_sec / commit_peek) to each platform decoder (linux, macos, windows) so a frame is only adopted once its pts is actually due; otherwise the current frame is held. live::Player::step and timeline_walk::advance_decoder_to (already correct on the export path) now share this hold semantics, and render_thread's accumulator tracks source time actually consumed instead of a fixed 1/60s step per tick. Co-Authored-By: Claude Sonnet 5 --- crates/compositor/src/linux_decode.rs | 67 +++++++++-- crates/compositor/src/live.rs | 138 ++++++++++++++-------- crates/compositor/src/pipeline_linux.rs | 18 +++ crates/compositor/src/pipeline_macos.rs | 85 +++++++++++-- crates/compositor/src/pipeline_windows.rs | 72 +++++++++-- crates/compositor/src/timeline_walk.rs | 38 ++++-- crates/poc-d3d/src/app.rs | 36 ++++-- 7 files changed, 362 insertions(+), 92 deletions(-) diff --git a/crates/compositor/src/linux_decode.rs b/crates/compositor/src/linux_decode.rs index e8bb81bc5..8d6bbe83a 100644 --- a/crates/compositor/src/linux_decode.rs +++ b/crates/compositor/src/linux_decode.rs @@ -58,6 +58,11 @@ pub struct SwDecoder { frame: *mut AVFrame, sent_eof: bool, cur_pts: Option, + /// Buffer de lookahead pour `peek_next_time_sec` : symétrique de + /// `pipeline_macos::Decoder::peek_frame` — cf. là-bas pour la justification. + peek_frame: *mut AVFrame, + /// `true` si `peek_frame` porte une frame décodée en attente de `commit_peek`. + has_peek: bool, } /// Libère toutes les ressources ffmpeg. `Drop` ne peut pas faillir ; on @@ -78,6 +83,9 @@ impl Drop for SwDecoder { if !self.pkt.is_null() { av_packet_free(&mut self.pkt); } + if !self.peek_frame.is_null() { + av_frame_free(&mut self.peek_frame); + } } } } @@ -177,7 +185,8 @@ impl SwDecoder { }; let pkt = av_packet_alloc(); let frame = av_frame_alloc(); - if pkt.is_null() || frame.is_null() { + let peek_frame = av_frame_alloc(); + if pkt.is_null() || frame.is_null() || peek_frame.is_null() { avcodec_free_context(&mut dec); avformat_close_input(&mut fmt); bail!("av_packet_alloc/av_frame_alloc (pompage sequentiel)"); @@ -192,6 +201,8 @@ impl SwDecoder { frame, sent_eof: false, cur_pts: None, + peek_frame, + has_peek: false, }) } @@ -201,21 +212,33 @@ impl SwDecoder { /// seek PAS : le decodeur garde son etat, donc une lecture sequentielle coute /// UN packet par frame au lieu d'un re-parcours de demi-GOP. pub unsafe fn next_frame(&mut self) -> Result<*mut AVFrame> { + if self.has_peek { + return self.commit_peek(); + } + if !self.receive_into(self.frame)? { + return Ok(ptr::null_mut()); + } + let pts = (*self.frame).best_effort_timestamp; + self.cur_pts = if pts == i64::MIN { None } else { Some(pts) }; + Ok(self.frame) + } + + /// Décode dans `into` (buffer courant ou de lookahead) jusqu'à obtenir une frame ou + /// l'EOF — cf. `pipeline_macos::Decoder::receive_into` pour la justification. + unsafe fn receive_into(&mut self, into: *mut AVFrame) -> Result { loop { - let r = avcodec_receive_frame(self.dec, self.frame); + let r = avcodec_receive_frame(self.dec, into); if r == 0 { - let pts = (*self.frame).best_effort_timestamp; - self.cur_pts = if pts == i64::MIN { None } else { Some(pts) }; - return Ok(self.frame); + return Ok(true); } if r == AVERROR_EOF { - return Ok(ptr::null_mut()); + return Ok(false); } if r != AVERROR_EAGAIN { bail!("avcodec_receive_frame: {r}"); } if self.sent_eof { - return Ok(ptr::null_mut()); + return Ok(false); } let rr = av_read_frame(self.fmt, self.pkt); if rr < 0 { @@ -240,6 +263,34 @@ impl SwDecoder { } } + /// Décode la prochaine frame dans le buffer de lookahead et renvoie son temps (s) — + /// `None` à EOF. Cf. `pipeline_macos::Decoder::peek_next_time_sec`. + pub unsafe fn peek_next_time_sec(&mut self) -> Result> { + if !self.has_peek { + if !self.receive_into(self.peek_frame)? { + return Ok(None); + } + self.has_peek = true; + } + let pts = (*self.peek_frame).best_effort_timestamp; + Ok(Some(if pts == i64::MIN { + 0.0 + } else { + pts as f64 * self.stream_timebase + })) + } + + /// Promeut la frame de lookahead au rang de frame courante. Cf. + /// `pipeline_macos::Decoder::commit_peek`. + pub unsafe fn commit_peek(&mut self) -> Result<*mut AVFrame> { + debug_assert!(self.has_peek, "commit_peek sans peek_next_time_sec préalable"); + std::mem::swap(&mut self.frame, &mut self.peek_frame); + self.has_peek = false; + let pts = (*self.frame).best_effort_timestamp; + self.cur_pts = if pts == i64::MIN { None } else { Some(pts) }; + Ok(self.frame) + } + /// Temps source (secondes) de la derniere frame rendue par `next_frame` / /// `decode_at`, tire du pts REEL et non d'un compteur d'index. pub fn cur_time_sec(&self) -> Option { @@ -263,6 +314,8 @@ impl SwDecoder { /// `AVERROR_INVALIDDATA` plutôt que de paniquer : la prochaine itération /// lira le packet complet suivant. pub unsafe fn decode_at(&mut self, frame_idx: u32) -> Result<*mut AVFrame> { + // Tout seek invalide un éventuel peek en attente — cf. pipeline_macos::Decoder::seek_to. + self.has_peek = false; let fps = self.fps; let target_ts = (frame_idx as f64 / fps) * 1_000_000.0; // AV_TIME_BASE = µs // `AVSEEK_FLAG_BACKWARD` vaut 1, pas 4 — 4 est `AVSEEK_FLAG_ANY`. La constante diff --git a/crates/compositor/src/live.rs b/crates/compositor/src/live.rs index 49199a699..8a163d838 100644 --- a/crates/compositor/src/live.rs +++ b/crates/compositor/src/live.rs @@ -338,42 +338,64 @@ impl Player { } /// Temps source courant du décodeur écran — utilisé par `render_thread` pour détecter le - /// franchissement de la fin de fenêtre du clip actif pendant la lecture libre. - pub(crate) unsafe fn screen_time_sec(&self) -> f64 { + /// franchissement de la fin de fenêtre du clip actif pendant la lecture libre, et pour + /// calculer la cible de `step` en lecture libre. `pub` (pas `pub(crate)`) : le harnais + /// `poc-d3d` (crate externe) en a besoin pour piloter sa propre boucle de lecture libre. + pub unsafe fn screen_time_sec(&self) -> f64 { self.sdec.cur_time_sec() } - /// Compose la frame suivante (→ `comp.rt`). Boucle sur EOF. `false` si fixture vide. + /// Compose la PROCHAINE frame due (→ `comp.rt`), au plus une, si `target_source_time` + /// (temps écran) est atteint. Sémantique de "hold" : `false` sans rien composer quand la + /// frame suivante n'est pas encore due — l'appelant garde alors l'image déjà affichée, + /// au lieu d'avancer aveuglément. Boucle sur EOF réel. `false` aussi si fixture vide. /// - /// L'écran pilote la cadence (1 frame/tick) ; la webcam suit son PROPRE temps source - /// (`screen_time - webcam_offset_sec`), pas un pas 1:1 avec l'écran — BUG corrigé : les - /// deux décodeurs avançaient d'exactement une frame par tick chacun, quelle que soit leur - /// cadence réelle. Écran et webcam sont capturés par des pipelines indépendants (souvent - /// à des fps différents), donc la webcam jouait 2× trop vite dès que sa cadence était - /// inférieure à celle de l'écran. Même logique que `advance_decoder_to` (pipeline.rs), - /// déjà correcte côté export — la preview live ne l'avait jamais reprise. La webcam boucle - /// aussi de façon INDÉPENDANTE à son propre EOF (un clip webcam plus court que l'écran ne - /// doit pas réinitialiser le décodeur écran). - pub unsafe fn step(&mut self, comp: &Compositor, cfg: &Cfg) -> Result { + /// BUG corrigé : cette fonction consommait auparavant EXACTEMENT une frame réelle par + /// appel (un `next()` inconditionnel), et `render_thread` l'appelait une fois par tranche + /// de 1/60s de temps réel écoulé — une hypothèse de vidéo à ~60 fps constant. Or + /// ScreenCaptureKit (et les captures équivalentes) ne livre une frame que quand l'écran + /// change : un enregistrement de 26s avec de longs plans fixes peut ne contenir que + /// quelques centaines de frames RÉELLES. Consommer 1 frame/tick épuisait alors le flux + /// bien avant que le temps réel écoulé n'atteigne la durée de l'enregistrement — le + /// décodeur retombait sur l'EOF, rebouclait sur `seek_to(0.0)`, et la preview semblait + /// « accélérer puis sauter au début » en boucle. Le curseur/zoom, eux, suivent le pts réel + /// (`sync_time`) et se retrouvaient donc en avance sur ce que l'œil voyait défiler. + /// + /// Le correctif : ne décoder/adopter (`commit_peek`) la frame suivante QUE si son pts a + /// réellement été atteint par `target_source_time` (le temps réel écoulé, mis à l'échelle + /// par la vitesse active — cf. `render_thread`) ; sinon on continue de tenir la frame + /// courante, aussi longtemps qu'il le faut. Même principe que `advance_decoder_to` + /// (`timeline_walk.rs`), déjà correct côté export. + /// + /// La webcam suit le MÊME principe indépendamment (son propre temps source = + /// `screen_time - webcam_offset_sec`, pas un pas 1:1 avec l'écran) : deux pipelines de + /// capture indépendants n'ont pas la même cadence ni les mêmes trous. Elle boucle aussi + /// de façon indépendante à son propre EOF (un clip webcam plus court que l'écran ne doit + /// pas réinitialiser le décodeur écran). + pub unsafe fn step(&mut self, comp: &Compositor, cfg: &Cfg, target_source_time: f64) -> Result { let use_current = self.use_current_on_next_step; self.use_current_on_next_step = false; - let mut sf = if use_current { + let sf = if use_current { self.sdec.cur_frame() } else { - self.sdec.next()? + match self.sdec.peek_next_time_sec()? { + Some(t) if t <= target_source_time => self.sdec.commit_peek()?, + Some(_) => return Ok(false), // pas encore due : on tient la frame courante. + None => { + // EOF réel (plus aucune frame à décoder) : reboucle sur le début. + self.idx = 0; + self.sdec.seek_to(0.0)? + } + } }; - if sf.is_null() { - sf = self.sdec.seek_to(0.0)?; - self.idx = 0; - } if sf.is_null() { self.has_current_frame = false; return Ok(false); } let target_webcam_t = (self.sdec.cur_time_sec() - self.webcam_offset_sec).max(0.0); - let mut wf = if use_current { + let wf = if use_current { self.wdec.cur_frame() } else { let cur = self.wdec.cur_frame(); @@ -381,19 +403,22 @@ impl Player { // Jamais décodée (nouvelle ouverture) : on saute directement au temps synchronisé. self.wdec.seek_to(target_webcam_t)? } else { - // Rattrape la webcam vers `target_webcam_t`, au pire une poignée de frames par - // tick (fps proches) — le garde-fou n'existe que contre un cas pathologique. + // Rattrape la webcam vers `target_webcam_t` par pts réel, jamais au-delà — + // même sémantique de hold que l'écran ci-dessus (et que `advance_decoder_to`) : + // adopter une frame webcam dont le pts dépasse `target_webcam_t` l'afficherait + // en avance sur son heure. Le garde-fou ne joue que contre un cas pathologique. let mut wf = cur; let mut guard = 0u32; - while self.wdec.cur_time_sec() < target_webcam_t { - match self.wdec.next()? { - f if f.is_null() => { + loop { + match self.wdec.peek_next_time_sec()? { + Some(t) if t <= target_webcam_t => wf = self.wdec.commit_peek()?, + Some(_) => break, // pas encore due : hold sur la frame webcam courante. + None => { // Fin de la webcam avant l'écran : elle boucle SEULE — l'écran // garde sa propre position, inchangée. wf = self.wdec.seek_to(0.0)?; break; } - f => wf = f, } guard += 1; if guard > 1000 { @@ -1450,26 +1475,34 @@ unsafe fn render_thread( } acc = 0.0; // resynchronise l'accumulateur de lecture libre après un seek } else if shared.playing.load(Ordering::Relaxed) { - // BUG corrigé : la lecture libre décodait toujours exactement 1 frame par tick de - // 1/60s réel, quelle que soit la speed region active au temps source courant — ni - // l'écran ni la webcam n'accéléraient/ralentissaient jamais en preview live (seul - // l'export, via `speed_segments_for_window`/`advance_decoder_to` dans pipeline.rs, - // retimait correctement). Mod 3 corrige déjà le fps-mismatch webcam/écran (la webcam - // suit le temps source RÉEL de l'écran, pas un pas 1:1) — reprend ici la même idée : - // l'accumulateur de temps réel est mis à l'échelle par le multiplicateur de vitesse - // actif, donc `step()` (qui resynchronise la webcam sur le temps écran courant, - // cf. plus haut) décode plus/moins de frames par seconde réelle selon la région. + // BUG corrigé : la lecture libre décodait auparavant exactement 1 frame RÉELLE par + // tranche de 1/60s de temps réel écoulé, quelle que soit la densité effective de + // frames de la source. ScreenCaptureKit (et les captures équivalentes) ne livre une + // frame que quand l'écran change : un enregistrement avec de longs plans fixes peut + // ne contenir que quelques centaines de frames RÉELLES sur toute sa durée. Consommer + // 1 frame/tick épuisait alors le flux bien avant que le temps réel écoulé n'atteigne + // la durée de l'enregistrement — `step()` retombait sur l'EOF et rebouclait sur + // `seek_to(0.0)`, d'où la preview qui semblait « accélérer puis sauter au début » en + // boucle (cf. doc de `step()` pour le détail). + // + // Le correctif : `acc` (mis à l'échelle par la speed region active, cf. mod 3 plus + // bas pour le fps-mismatch webcam/écran) n'est plus consommé par tranches fixes de + // 1/60s — c'est une CIBLE de temps source (`target = temps courant + acc`) que + // `step()` n'atteint qu'en adoptant une frame dont le pts est réellement dû (hold + // sinon). `acc` n'est décrémenté que du temps source RÉELLEMENT consommé par chaque + // frame adoptée, jamais d'un pas fixe — donc les plans fixes ne consomment aucune + // frame et n'avancent le décodeur que quand une frame due existe vraiment. let speed = full_scene .as_ref() .map(|scene| speed_at(&scene.speed_regions, active_clip_index, player.screen_time_sec())) .unwrap_or(1.0); acc += dt * speed; - let step = 1.0 / 60.0; let mut n = 0; - // Cap proportionnel à la vitesse (borné) : à vitesse élevée, plus de frames doivent - // être décodées par tick réel pour ne pas prendre du retard sur l'accumulateur. + // Cap sur le nombre de frames RÉELLEMENT adoptées par tick (pas sur le nombre de + // ticks) : à vitesse élevée sur du contenu dense, plus de frames doivent être + // décodées par tick réel pour ne pas prendre du retard sur l'accumulateur. let max_steps = ((3.0 * speed.max(1.0)).ceil() as i32).min(64); - while acc >= step && n < max_steps { + loop { // Timeline = niveau d'abstraction AU-DESSUS des clips : dès que le décodeur // écran atteint la fin de fenêtre du clip actif, on enchaîne nous-mêmes sur // le clip suivant (ou on reboucle sur le premier après le dernier) — sans @@ -1507,9 +1540,9 @@ unsafe fn render_thread( } } let screen_time_before_step = full_scene.as_ref().map(|_| player.screen_time_sec()); - if player.step(&comp, &cfg)? { - stepped = true; - } + let before = player.screen_time_sec(); + let target = before + acc; + let committed = player.step(&comp, &cfg, target)?; // Filet de sécurité : un clip NON trimmé (source_end_sec == durée totale du // fichier) peut ne jamais franchir le seuil ci-dessus si la dernière frame // réelle a un PTS strictement inférieur à `source_end_sec` déclaré — `step()` @@ -1532,11 +1565,24 @@ unsafe fn render_thread( ); } } - acc -= step; + if !committed { + // Rien n'est dû pour l'instant (hold) : `acc` reste tel quel — il continue + // de s'accumuler aux ticks suivants jusqu'à ce qu'une vraie frame arrive. + break; + } + stepped = true; + let after = player.screen_time_sec(); + // `after < before` : `step()` a rebouclé sur l'EOF (temps qui recule) — le calcul + // de delta n'a alors aucun sens, on repart d'un accumulateur propre. + acc = if after >= before { (acc - (after - before)).max(0.0) } else { 0.0 }; n += 1; - } - if acc > step { - acc = 0.0; + if n >= max_steps { + // Rattrapage plafonné : le contenu dû est plus dense que ce qu'on peut décoder + // en un tick réel. On laisse tomber le reliquat plutôt que de creuser une + // dette qui s'accumulerait indéfiniment d'un tick à l'autre. + acc = 0.0; + break; + } } } else if first || ip_changed || scene_changed || clip_changed || resized { // pause : recompose la frame courante (param / scène / clip / résolution changés). diff --git a/crates/compositor/src/pipeline_linux.rs b/crates/compositor/src/pipeline_linux.rs index 2e8af625f..4e8ffc7d5 100644 --- a/crates/compositor/src/pipeline_linux.rs +++ b/crates/compositor/src/pipeline_linux.rs @@ -139,6 +139,24 @@ impl Decoder { Ok(carrier) } + /// Décode la prochaine frame dans le buffer de lookahead du décodeur sous-jacent et + /// renvoie son temps (s), sans la présenter (donc sans toucher `self.cur`) — `None` à + /// EOF. Cf. `pipeline_macos::Decoder::peek_next_time_sec` pour la sémantique "hold". + pub unsafe fn peek_next_time_sec(&mut self) -> Result> { + self.sw.peek_next_time_sec() + } + + /// Promeut la frame de lookahead au rang de frame courante ET la présente (upload NV12 + /// vers la texture carrier), contrairement au chemin macOS/Windows où la promotion est + /// un pur échange de pointeurs — ici la présentation est le pas qui manque. + pub unsafe fn commit_peek(&mut self) -> Result<*mut AVFrame> { + let raw = self.sw.commit_peek()?; + let carrier = self.frames.present(raw)?; + self.cur = carrier; + self.next_idx = self.next_idx.saturating_add(1); + Ok(carrier) + } + pub unsafe fn cur_frame(&self) -> *mut AVFrame { self.cur } diff --git a/crates/compositor/src/pipeline_macos.rs b/crates/compositor/src/pipeline_macos.rs index 8fec0fb23..1e44ba9c5 100644 --- a/crates/compositor/src/pipeline_macos.rs +++ b/crates/compositor/src/pipeline_macos.rs @@ -85,6 +85,14 @@ pub struct Decoder { /// qu'on pose dans `data[0]`). `None` quand VideoToolbox couvre le codec — le décodeur /// rend alors directement la frame VideoToolbox. cpu: Option, + /// Buffer de lookahead pour `peek_next_time_sec` : une frame décodée à l'avance, pas + /// encore promue en frame courante. Sépare "voir le pts de la frame suivante" de + /// "l'adopter" — condition de la sémantique "hold" (cf. `timeline_walk::advance_decoder_to` + /// et `live::Player::step`) : sans ce second buffer, `avcodec_receive_frame` écraserait + /// `frame` avant qu'on ait pu décider si son pts est déjà dû. + peek_frame: *mut crate::ffi::AVFrame, + /// `true` si `peek_frame` porte une frame décodée en attente de `commit_peek`. + has_peek: bool, } impl Decoder { @@ -201,6 +209,8 @@ impl Decoder { sent_eof: false, cur_pts: None, cpu, + peek_frame: crate::ffi::av_frame_alloc(), + has_peek: false, }) } } @@ -235,6 +245,9 @@ impl Decoder { /// rapide compris : mêmes seuils, même critère d'arrêt (`decode_forward_to`), pour /// que les deux moteurs rendent la même frame au même coût relatif. pub unsafe fn seek_to(&mut self, seconds: f64) -> Result<*mut crate::ffi::AVFrame> { + // Tout seek invalide un éventuel peek en attente : il portait sur "la frame après + // l'ancienne position courante", qui n'a plus de sens une fois qu'on a sauté ailleurs. + self.has_peek = false; let tb_sec = self.tb_sec(); if tb_sec > 0.0 { @@ -309,24 +322,42 @@ impl Decoder { /// Windows, juste sans le dispatch D3D11VA (le GPU hand-off est déjà fait par /// `av_hwdevice_ctx_create`). pub unsafe fn next(&mut self) -> Result<*mut crate::ffi::AVFrame> { + // Un peek déjà décodé en attente : l'appelant n'est pas passé par `commit_peek` + // (chemins qui ne raisonnent pas en hold, ex. `seek_to`/`decode_forward_to` après + // qu'aucun peek n'ait été posé) — le promouvoir reste correct dans tous les cas : + // c'est bien la prochaine frame du flux. + if self.has_peek { + return self.commit_peek(); + } + if !self.receive_into(self.frame)? { + return Ok(ptr::null_mut()); + } + let pts = (*self.frame).best_effort_timestamp; + self.cur_pts = if pts == i64::MIN { None } else { Some(pts) }; + match &mut self.cpu { + Some(cpu) => cpu.present(self.frame), + None => Ok(self.frame), + } + } + + /// Décode dans `into` (buffer courant ou de lookahead) jusqu'à obtenir une frame ou + /// l'EOF — pompage `avcodec_receive_frame`/`av_read_frame` brut, indépendant du buffer + /// cible. Factorisé pour que `next()` et `peek_next_time_sec()` partagent exactement la + /// même mécanique de décodage, seul le buffer destinataire changeant. + unsafe fn receive_into(&mut self, into: *mut crate::ffi::AVFrame) -> Result { loop { - let r = crate::ffi::avcodec_receive_frame(self.dctx, self.frame); + let r = crate::ffi::avcodec_receive_frame(self.dctx, into); if r == 0 { - let pts = (*self.frame).best_effort_timestamp; - self.cur_pts = if pts == i64::MIN { None } else { Some(pts) }; - return match &mut self.cpu { - Some(cpu) => cpu.present(self.frame), - None => Ok(self.frame), - }; + return Ok(true); } if r == crate::ffi::AVERROR_EOF { - return Ok(ptr::null_mut()); + return Ok(false); } if r != crate::ffi::AVERROR_EAGAIN { crate::ffi::averr(r, "receive_frame")?; } if self.sent_eof { - return Ok(ptr::null_mut()); + return Ok(false); } let rr = crate::ffi::av_read_frame(self.fmt, self.pkt); if rr == crate::ffi::AVERROR_EOF { @@ -345,6 +376,41 @@ impl Decoder { } } + /// Décode la PROCHAINE frame dans le buffer de lookahead (si aucun peek n'est déjà en + /// attente) et renvoie son temps (s) — `None` à EOF. Ne touche pas au buffer courant : + /// l'appelant peut ainsi comparer ce pts à une cible avant de décider d'adopter la + /// frame (`commit_peek`) ou de continuer à tenir la frame courante (hold). + pub unsafe fn peek_next_time_sec(&mut self) -> Result> { + if !self.has_peek { + if !self.receive_into(self.peek_frame)? { + return Ok(None); + } + self.has_peek = true; + } + let pts = (*self.peek_frame).best_effort_timestamp; + let tb_sec = self.tb_sec(); + Ok(Some(if pts == i64::MIN || tb_sec <= 0.0 { + 0.0 + } else { + pts as f64 * tb_sec + })) + } + + /// Promeut la frame de lookahead (décodée par un `peek_next_time_sec` précédent) au + /// rang de frame courante — échange de pointeurs, aucune E/S. Ne doit être appelé + /// qu'après un `peek_next_time_sec` ayant renvoyé `Some`. + pub unsafe fn commit_peek(&mut self) -> Result<*mut crate::ffi::AVFrame> { + debug_assert!(self.has_peek, "commit_peek sans peek_next_time_sec préalable"); + std::mem::swap(&mut self.frame, &mut self.peek_frame); + self.has_peek = false; + let pts = (*self.frame).best_effort_timestamp; + self.cur_pts = if pts == i64::MIN { None } else { Some(pts) }; + match &mut self.cpu { + Some(cpu) => cpu.present(self.frame), + None => Ok(self.frame), + } + } + pub unsafe fn cur_frame(&self) -> *mut crate::ffi::AVFrame { match &self.cpu { Some(cpu) => cpu.current(), @@ -405,6 +471,7 @@ impl Drop for Decoder { fn drop(&mut self) { unsafe { crate::ffi::av_frame_free(&mut self.frame); + crate::ffi::av_frame_free(&mut self.peek_frame); crate::ffi::av_packet_free(&mut self.pkt); crate::ffi::avcodec_free_context(&mut self.dctx); if !self.hwdev.is_null() { diff --git a/crates/compositor/src/pipeline_windows.rs b/crates/compositor/src/pipeline_windows.rs index bbdab2642..7d8bd2881 100644 --- a/crates/compositor/src/pipeline_windows.rs +++ b/crates/compositor/src/pipeline_windows.rs @@ -486,6 +486,11 @@ pub(crate) struct Decoder { /// sous le même contrat que D3D11VA (voir `cpu_frames`). `None` en matériel — le /// décodeur rend alors directement la texture du pool D3D11VA, sans copie. cpu: Option, + /// Buffer de lookahead pour `peek_next_time_sec` : symétrique de + /// `pipeline_macos::Decoder::peek_frame`. Cf. là-bas pour la justification. + peek_frame: *mut AVFrame, + /// `true` si `peek_frame` porte une frame décodée en attente de `commit_peek`. + has_peek: bool, } // SAFETY: `Decoder` only owns FFI pointers into FFmpeg's own heap-allocated state, which @@ -555,6 +560,8 @@ impl Decoder { sent_eof: false, cur_pts: None, cpu, + peek_frame: av_frame_alloc(), + has_peek: false, }) } @@ -592,6 +599,8 @@ impl Decoder { /// perf multiclip — un seul seek par frontière de clip, décodage séquentiel ensuite, /// donc le débit par frame ne change pas. Renvoie la frame (ou null à EOF). pub(crate) unsafe fn seek_to(&mut self, seconds: f64) -> Result<*mut AVFrame> { + // Tout seek invalide un éventuel peek en attente — cf. pipeline_macos::Decoder::seek_to. + self.has_peek = false; let tb_sec = self.tb_sec(); // Chemin rapide. Le seek complet ci-dessous jette TOUT l'état du décodeur et repart @@ -711,24 +720,36 @@ impl Decoder { /// Rend la prochaine frame (valide jusqu'au prochain appel), ou null à EOF. pub(crate) unsafe fn next(&mut self) -> Result<*mut AVFrame> { + if self.has_peek { + return self.commit_peek(); + } + if !self.receive_into(self.frame)? { + return Ok(ptr::null_mut()); + } + let pts = (*self.frame).best_effort_timestamp; + self.cur_pts = if pts == i64::MIN { None } else { Some(pts) }; + match &mut self.cpu { + Some(cpu) => cpu.present(self.frame), + None => Ok(self.frame), + } + } + + /// Décode dans `into` (buffer courant ou de lookahead) jusqu'à obtenir une frame ou + /// l'EOF — cf. `pipeline_macos::Decoder::receive_into` pour la justification. + unsafe fn receive_into(&mut self, into: *mut AVFrame) -> Result { loop { - let r = avcodec_receive_frame(self.dctx, self.frame); + let r = avcodec_receive_frame(self.dctx, into); if r == 0 { - let pts = (*self.frame).best_effort_timestamp; - self.cur_pts = if pts == i64::MIN { None } else { Some(pts) }; - return match &mut self.cpu { - Some(cpu) => cpu.present(self.frame), - None => Ok(self.frame), - }; + return Ok(true); } if r == AVERROR_EOF { - return Ok(ptr::null_mut()); + return Ok(false); } if r != AVERROR_EAGAIN { averr(r, "receive_frame")?; } if self.sent_eof { - return Ok(ptr::null_mut()); + return Ok(false); } let rr = av_read_frame(self.fmt, self.pkt); if rr == AVERROR_EOF { @@ -743,12 +764,45 @@ impl Decoder { } } } + + /// Décode la prochaine frame dans le buffer de lookahead et renvoie son temps (s) — + /// `None` à EOF. Cf. `pipeline_macos::Decoder::peek_next_time_sec`. + pub(crate) unsafe fn peek_next_time_sec(&mut self) -> Result> { + if !self.has_peek { + if !self.receive_into(self.peek_frame)? { + return Ok(None); + } + self.has_peek = true; + } + let pts = (*self.peek_frame).best_effort_timestamp; + let tb_sec = self.tb_sec(); + Ok(Some(if pts == i64::MIN || tb_sec <= 0.0 { + 0.0 + } else { + pts as f64 * tb_sec + })) + } + + /// Promeut la frame de lookahead au rang de frame courante. Cf. + /// `pipeline_macos::Decoder::commit_peek`. + pub(crate) unsafe fn commit_peek(&mut self) -> Result<*mut AVFrame> { + debug_assert!(self.has_peek, "commit_peek sans peek_next_time_sec préalable"); + std::mem::swap(&mut self.frame, &mut self.peek_frame); + self.has_peek = false; + let pts = (*self.frame).best_effort_timestamp; + self.cur_pts = if pts == i64::MIN { None } else { Some(pts) }; + match &mut self.cpu { + Some(cpu) => cpu.present(self.frame), + None => Ok(self.frame), + } + } } impl Drop for Decoder { fn drop(&mut self) { unsafe { av_frame_free(&mut self.frame); + av_frame_free(&mut self.peek_frame); av_packet_free(&mut self.pkt); avcodec_free_context(&mut self.dctx); av_buffer_unref(&mut self.hwdev); diff --git a/crates/compositor/src/timeline_walk.rs b/crates/compositor/src/timeline_walk.rs index e5146e6cd..1367d3797 100644 --- a/crates/compositor/src/timeline_walk.rs +++ b/crates/compositor/src/timeline_walk.rs @@ -23,24 +23,40 @@ use crate::scene::Scene; use anyhow::Result; use std::collections::HashMap; -/// Avance un décodeur jusqu'au premier pts dans le référentiel écran qui atteint la cible. -/// `timeline_offset_sec` remet les pts webcam dans ce référentiel (`webcam + offset = screen`) : -/// chaque source garde ainsi sa cadence propre au lieu d'être consommée 1:1 avec l'autre. +/// Avance un décodeur vers `target_source_time`, sémantique de "hold" : à l'instant t on +/// affiche la DERNIÈRE frame dont le pts est ≤ t, jamais une frame dont le pts est encore à +/// venir. `timeline_offset_sec` remet les pts webcam dans le référentiel écran +/// (`webcam + offset = screen`) : chaque source garde ainsi sa cadence propre au lieu +/// d'être consommée 1:1 avec l'autre. +/// +/// BUG corrigé : l'ancienne version avançait tant que `cur_time_sec() < target`, un pas de +/// `next()` à la fois, et s'arrêtait dès que la frame COURANTE dépassait la cible — mais +/// `next()` saute à la prochaine frame RÉELLEMENT capturée, qui peut être très en avance +/// sur `target` quand la source a un trou (ex. ScreenCaptureKit qui ne livre rien tant que +/// l'écran ne change pas). Un seul `next()` pouvait alors faire passer le décodeur d'un pts +/// proche de la cible à un pts bien après elle, et la condition d'arrêt considérait ça comme +/// "atteint" — la frame FUTURE se retrouvait affichée bien avant son heure. Ici, `next()` +/// n'est plus appelé à l'aveugle : on regarde d'abord le pts de la frame suivante +/// (`peek_next_time_sec`, décodée dans un buffer séparé) et on ne l'adopte +/// (`commit_peek`) que si elle est réellement due ; sinon on continue de tenir la frame +/// courante, aussi longtemps qu'il le faut. pub(crate) unsafe fn advance_decoder_to( decoder: &mut Decoder, target_source_time: f64, timeline_offset_sec: f64, ) -> Result { + if decoder.cur_frame().is_null() { + return Ok(false); + } loop { - if decoder.cur_frame().is_null() { - return Ok(false); - } - if decoder.cur_time_sec() + timeline_offset_sec >= target_source_time { - return Ok(true); - } - if decoder.next()?.is_null() { - return Ok(false); + let next_time = match decoder.peek_next_time_sec()? { + Some(t) => t, + None => return Ok(true), // EOF : plus rien à décoder, on tient la dernière frame connue. + }; + if next_time + timeline_offset_sec > target_source_time { + return Ok(true); // la frame suivante n'est pas encore due : hold sur la courante. } + decoder.commit_peek()?; } } diff --git a/crates/poc-d3d/src/app.rs b/crates/poc-d3d/src/app.rs index 6c49b7d2b..5822a6157 100644 --- a/crates/poc-d3d/src/app.rs +++ b/crates/poc-d3d/src/app.rs @@ -100,16 +100,24 @@ struct App { } impl App { - /// Compose + affiche la 1re frame, avant l'ouverture de la fenêtre. + /// Compose + affiche la 1re frame, avant l'ouverture de la fenêtre. Cible `INFINITY` : + /// on veut cette toute première frame quel que soit son pts, la notion de "due" n'a pas + /// encore de sens avant le premier tick réel (cf. `Player::step`, sémantique de hold). unsafe fn init_first_frame(&mut self) { let cfg = self.cfgs[self.cur].clone(); - let _ = self.player.step(&self.comp, &cfg); + let _ = self.player.step(&self.comp, &cfg, f64::INFINITY); let _ = self.render(); self.update_ready_status(); self.last = Instant::now(); } /// Cadence 60 fps par horloge murale (accumulateur), avec garde anti-spirale. + /// + /// `self.acc` est une CIBLE de temps source (mis à l'échelle par le temps réel écoulé), + /// pas un compte de frames à décoder — `Player::step` n'adopte une frame que si son pts + /// est réellement dû, sinon il tient la frame courante (hold). Sans ça, ce harnais + /// consommerait une frame réelle par 1/60s de temps réel même quand la source n'en livre + /// pas autant (cf. la doc de `Player::step` côté lib, même bug que la preview Electron). unsafe fn on_tick(&mut self) -> Result<()> { if self.exporting || !self.playing { return Ok(()); @@ -118,19 +126,27 @@ impl App { let dt = (now - self.last).as_secs_f64().min(0.1); self.last = now; self.acc += dt; - let step = 1.0 / 60.0; let cfg = self.cfgs[self.cur].clone(); let mut stepped = false; let mut n = 0; - while self.acc >= step && n < 3 { - if self.player.step(&self.comp, &cfg)? { - stepped = true; + loop { + let before = self.player.screen_time_sec(); + let target = before + self.acc; + if !self.player.step(&self.comp, &cfg, target)? { + break; // rien de dû pour l'instant : `self.acc` reste tel quel. } - self.acc -= step; + stepped = true; + let after = self.player.screen_time_sec(); + self.acc = if after >= before { + (self.acc - (after - before)).max(0.0) + } else { + 0.0 // reboucle sur l'EOF (temps qui recule) : accumulateur remis à zéro. + }; n += 1; - } - if self.acc > step { - self.acc = 0.0; // largue le retard accumulé (fenêtre masquée, etc.) + if n >= 3 { + self.acc = 0.0; // largue le retard accumulé (fenêtre masquée, etc.) + break; + } } if stepped { self.render()?; From fbb382618cb6dddd4de6cb16bc46535bebc4db5d Mon Sep 17 00:00:00 2001 From: Etienne Lescot Date: Mon, 3 Aug 2026 15:42:23 +0200 Subject: [PATCH 2/3] fix(compositor): make the hold semantics safe on broken streams, and test it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the frame-hold fix — three from CodeRabbit, three from a second read. An unusable pts no longer reads as "due" (CodeRabbit). `peek_next_time_sec` returned `Some(0.0)` when `best_effort_timestamp` is `i64::MIN` or the time_base is zero, and `0.0` satisfies the commit condition against every target — so a stream with no usable pts was drained frame after frame to EOF, which is the exact failure the hold semantics exist to prevent, only worse. The three decoders now return a `NextFrameTime` that says `At`, `Unknown` or `Eof`, and `Unknown` advances exactly ONE frame before yielding — the pre-hold behaviour, restricted to the broken stream that warrants it instead of being the general rule. `commit_peek` enforces its precondition with `bail!` instead of `debug_assert!` (CodeRabbit). Compiled out in release, the assertion let a caller promote an `AVFrame` that was never filled, with an undefined `best_effort_timestamp`, all the way into the presentation path. `rewind()` clears the pending peek (CodeRabbit). `seek_to` already did; `rewind` ran the same `av_seek_frame` + `avcodec_flush_buffers` without it, so the next `next()` promoted a frame decoded at the pre-rewind position, carrying its old `cur_pts`. Windows and macOS both. The export's EOF behaviour is now stated rather than implied. `advance_decoder_to` returning `true` at EOF (it returned `false`) stops the clip loop from breaking early, so a clip whose declared window outruns its last real pts fills that window by holding its last frame. That is what the audio already assumes — `on_clip_end` reports the clip's frame count and the audio is stretched over the DECLARED duration — so a video that stopped short used to shift the next clip's junction. The trade is real and now documented: a genuinely truncated source freezes to the end of its window instead of stopping. `advance_decoder_to` also re-checks the frame it just adopted, restoring the per-iteration invariant the entry guard used to provide. And it has tests now, which the original change did not: the decision is extracted into a pure `frame_step`, and the accumulator arithmetic into `consume_acc`, so both are exercisable without ffmpeg or a file. They cover what the bug was actually about — a 24fps source adopting 24 frames per real second, not 60 — plus the sparse-source gap, the exact-pts boundary, the webcam offset, EOF hold, and the unusable-pts case. 122/122 pass. --- crates/compositor/src/linux_decode.rs | 29 ++-- crates/compositor/src/live.rs | 75 +++++++-- crates/compositor/src/pipeline_linux.rs | 9 +- crates/compositor/src/pipeline_macos.rs | 38 +++-- crates/compositor/src/pipeline_windows.rs | 32 ++-- crates/compositor/src/timeline_walk.rs | 177 +++++++++++++++++++++- 6 files changed, 308 insertions(+), 52 deletions(-) diff --git a/crates/compositor/src/linux_decode.rs b/crates/compositor/src/linux_decode.rs index 8d6bbe83a..1255898ee 100644 --- a/crates/compositor/src/linux_decode.rs +++ b/crates/compositor/src/linux_decode.rs @@ -16,6 +16,8 @@ use anyhow::{bail, Context, Result}; use std::ffi::CString; use std::ptr; +use crate::timeline_walk::NextFrameTime; + use crate::ffi::{ av_frame_alloc, av_frame_free, av_frame_move_ref, av_frame_unref, av_packet_alloc, av_packet_free, av_packet_unref, av_read_frame, av_seek_frame, avcodec_alloc_context3, @@ -263,27 +265,34 @@ impl SwDecoder { } } - /// Décode la prochaine frame dans le buffer de lookahead et renvoie son temps (s) — - /// `None` à EOF. Cf. `pipeline_macos::Decoder::peek_next_time_sec`. - pub unsafe fn peek_next_time_sec(&mut self) -> Result> { + /// Décode la prochaine frame dans le buffer de lookahead et renvoie son temps. + /// Cf. `pipeline_macos::Decoder::peek_next_time_sec`. + pub(crate) unsafe fn peek_next_time_sec(&mut self) -> Result { if !self.has_peek { if !self.receive_into(self.peek_frame)? { - return Ok(None); + return Ok(NextFrameTime::Eof); } self.has_peek = true; } let pts = (*self.peek_frame).best_effort_timestamp; - Ok(Some(if pts == i64::MIN { - 0.0 + // Sans pts ni time_base exploitables on ne PEUT pas dire si la frame est due : + // `Unknown`, et non `0.0` — qui passait pour « due » à tous les coups. + Ok(if pts == i64::MIN || self.stream_timebase <= 0.0 { + NextFrameTime::Unknown } else { - pts as f64 * self.stream_timebase - })) + NextFrameTime::At(pts as f64 * self.stream_timebase) + }) } /// Promeut la frame de lookahead au rang de frame courante. Cf. /// `pipeline_macos::Decoder::commit_peek`. - pub unsafe fn commit_peek(&mut self) -> Result<*mut AVFrame> { - debug_assert!(self.has_peek, "commit_peek sans peek_next_time_sec préalable"); + pub(crate) unsafe fn commit_peek(&mut self) -> Result<*mut AVFrame> { + // `bail!` et non `debug_assert!` : compilée en release, l'assertion disparaissait + // et l'échange promouvait un `AVFrame` jamais rempli, avec un + // `best_effort_timestamp` indéterminé, jusque dans le chemin de présentation. + if !self.has_peek { + bail!("commit_peek sans peek_next_time_sec préalable"); + } std::mem::swap(&mut self.frame, &mut self.peek_frame); self.has_peek = false; let pts = (*self.frame).best_effort_timestamp; diff --git a/crates/compositor/src/live.rs b/crates/compositor/src/live.rs index 8a163d838..e7a876564 100644 --- a/crates/compositor/src/live.rs +++ b/crates/compositor/src/live.rs @@ -29,6 +29,7 @@ use crate::config::{self, Cfg}; use crate::cursor::CursorTrack; use crate::d3d::Gpu; use crate::pipeline::Decoder; +use crate::timeline_walk::NextFrameTime; use anyhow::Result; use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; use std::sync::{Arc, Mutex}; @@ -379,10 +380,18 @@ impl Player { let sf = if use_current { self.sdec.cur_frame() } else { + // Match direct sur `NextFrameTime` plutôt que via `frame_step` : la lecture + // live a une politique d'EOF PROPRE (reboucler au début), là où l'export tient + // la dernière frame. Le reste — « due » vs « pas encore due » — est la même + // règle, `>`/`<=` compris. match self.sdec.peek_next_time_sec()? { - Some(t) if t <= target_source_time => self.sdec.commit_peek()?, - Some(_) => return Ok(false), // pas encore due : on tient la frame courante. - None => { + NextFrameTime::At(t) if t <= target_source_time => self.sdec.commit_peek()?, + NextFrameTime::At(_) => return Ok(false), // pas encore due : on tient la courante. + // pts inexploitable : impossible de dire si elle est due. `step()` n'adopte + // qu'une frame écran par appel, donc l'adopter revient exactement à + // l'ancien « une frame par tick » — le repli correct pour un flux cassé. + NextFrameTime::Unknown => self.sdec.commit_peek()?, + NextFrameTime::Eof => { // EOF réel (plus aucune frame à décoder) : reboucle sur le début. self.idx = 0; self.sdec.seek_to(0.0)? @@ -411,9 +420,18 @@ impl Player { let mut guard = 0u32; loop { match self.wdec.peek_next_time_sec()? { - Some(t) if t <= target_webcam_t => wf = self.wdec.commit_peek()?, - Some(_) => break, // pas encore due : hold sur la frame webcam courante. - None => { + NextFrameTime::At(t) if t <= target_webcam_t => { + wf = self.wdec.commit_peek()? + } + NextFrameTime::At(_) => break, // pas encore due : hold sur la courante. + NextFrameTime::Unknown => { + // pts webcam inexploitable : on avance d'UNE frame et on sort, + // au lieu de vider la piste jusqu'à son EOF (ce que `0.0` + // faisait, puisqu'il est toujours ≤ à la cible). + wf = self.wdec.commit_peek()?; + break; + } + NextFrameTime::Eof => { // Fin de la webcam avant l'écran : elle boucle SEULE — l'écran // garde sa propre position, inchangée. wf = self.wdec.seek_to(0.0)?; @@ -494,6 +512,24 @@ impl Player { } } +/// Retranche de l'accumulateur le temps source RÉELLEMENT consommé par la frame qui vient +/// d'être adoptée. C'est ce qui fait jouer une source à sa propre cadence : une frame de +/// 1/24 s consomme 1/24 s d'accumulateur, donc 24 frames par seconde réelle — là où +/// l'ancien pas fixe de 1/60 s en décodait 60, soit 2,5× trop vite sur du 24 fps. +/// +/// `after < before` : `step()` a rebouclé sur l'EOF (le temps recule) — le delta n'a plus +/// de sens, on repart d'un accumulateur propre. +/// +/// Fonction à part pour être testable : c'est l'arithmétique dont dépend la vitesse de +/// lecture, et elle vivait au milieu de la boucle de rendu. +pub(crate) fn consume_acc(acc: f64, before: f64, after: f64) -> f64 { + if after >= before { + (acc - (after - before)).max(0.0) + } else { + 0.0 + } +} + /// Paramètres inspector pilotés depuis l'UI (setParam). Le thread de rendu les applique : /// booléens/taps → reconstruits dans le `Cfg` ; valeurs continues → `set_live_params`. #[derive(Clone, Copy, PartialEq)] @@ -1572,9 +1608,7 @@ unsafe fn render_thread( } stepped = true; let after = player.screen_time_sec(); - // `after < before` : `step()` a rebouclé sur l'EOF (temps qui recule) — le calcul - // de delta n'a alors aucun sens, on repart d'un accumulateur propre. - acc = if after >= before { (acc - (after - before)).max(0.0) } else { 0.0 }; + acc = consume_acc(acc, before, after); n += 1; if n >= max_steps { // Rattrapage plafonné : le contenu dû est plus dense que ce qu'on peut décoder @@ -1823,6 +1857,29 @@ mod tests { }"##).expect("multiclip scene") } + #[test] + fn acc_is_reduced_by_the_source_time_the_frame_actually_consumed() { + // 1/24 s d'accumulateur par frame de 24 fps : c'est ce qui fait jouer une source à + // SA cadence. L'ancien pas fixe retranchait 1/60 s quelle que soit la source, d'où + // 60 frames décodées par seconde réelle — 2,5× trop vite sur du 24 fps. + let acc = consume_acc(0.05, 10.0, 10.0 + 1.0 / 24.0); + assert!((acc - (0.05 - 1.0 / 24.0)).abs() < 1e-12); + } + + #[test] + fn acc_never_goes_negative() { + // Une frame plus longue que le retard accumulé ne doit pas creuser une dette + // négative qui ferait sauter la frame suivante. + assert_eq!(consume_acc(0.01, 10.0, 10.5), 0.0); + } + + #[test] + fn acc_resets_when_playback_loops_back_to_the_start() { + // `after < before` : `step()` a rebouclé sur l'EOF. Le delta serait négatif et + // gonflerait l'accumulateur — lecture emballée juste après la boucle. + assert_eq!(consume_acc(0.02, 1950.0, 0.0), 0.0); + } + #[test] fn explicit_index_disambiguates_clips_sharing_sources() { let scene = multiclip_scene(); diff --git a/crates/compositor/src/pipeline_linux.rs b/crates/compositor/src/pipeline_linux.rs index 4e8ffc7d5..0d0d9804b 100644 --- a/crates/compositor/src/pipeline_linux.rs +++ b/crates/compositor/src/pipeline_linux.rs @@ -28,6 +28,7 @@ use crate::config::Cfg; use crate::d3d::Gpu; use crate::ffi::AVFrame; use crate::linux_decode::SwDecoder; +use crate::timeline_walk::NextFrameTime; use crate::linux_frames::CpuFrames; /// `SWS_POINT` (plus proche voisin). Bindgen ne genere pas les `SWS_*` (macros), @@ -140,16 +141,16 @@ impl Decoder { } /// Décode la prochaine frame dans le buffer de lookahead du décodeur sous-jacent et - /// renvoie son temps (s), sans la présenter (donc sans toucher `self.cur`) — `None` à - /// EOF. Cf. `pipeline_macos::Decoder::peek_next_time_sec` pour la sémantique "hold". - pub unsafe fn peek_next_time_sec(&mut self) -> Result> { + /// renvoie son temps, sans la présenter (donc sans toucher `self.cur`). + /// Cf. `pipeline_macos::Decoder::peek_next_time_sec` pour la sémantique "hold". + pub(crate) unsafe fn peek_next_time_sec(&mut self) -> Result { self.sw.peek_next_time_sec() } /// Promeut la frame de lookahead au rang de frame courante ET la présente (upload NV12 /// vers la texture carrier), contrairement au chemin macOS/Windows où la promotion est /// un pur échange de pointeurs — ici la présentation est le pas qui manque. - pub unsafe fn commit_peek(&mut self) -> Result<*mut AVFrame> { + pub(crate) unsafe fn commit_peek(&mut self) -> Result<*mut AVFrame> { let raw = self.sw.commit_peek()?; let carrier = self.frames.present(raw)?; self.cur = carrier; diff --git a/crates/compositor/src/pipeline_macos.rs b/crates/compositor/src/pipeline_macos.rs index 1e44ba9c5..2ad12b662 100644 --- a/crates/compositor/src/pipeline_macos.rs +++ b/crates/compositor/src/pipeline_macos.rs @@ -35,6 +35,7 @@ use crate::audio::{ }; use crate::compositor::Compositor; use crate::d3d::Gpu; +use crate::timeline_walk::NextFrameTime; use anyhow::{anyhow, bail, Result}; use std::ffi::{c_void, CString}; use std::ptr; @@ -216,6 +217,11 @@ impl Decoder { } pub unsafe fn rewind(&mut self) -> Result<()> { + // Même règle que `seek_to` : tout repositionnement invalide le peek en attente. + // Il portait sur « la frame d'après l'ancienne position », qui n'a plus de sens + // ici — sans ça le `next()` suivant promouvait une frame décodée avant le rewind, + // avec son ancien `cur_pts`. + self.has_peek = false; crate::ffi::averr( crate::ffi::av_seek_frame( self.fmt, @@ -377,30 +383,38 @@ impl Decoder { } /// Décode la PROCHAINE frame dans le buffer de lookahead (si aucun peek n'est déjà en - /// attente) et renvoie son temps (s) — `None` à EOF. Ne touche pas au buffer courant : - /// l'appelant peut ainsi comparer ce pts à une cible avant de décider d'adopter la - /// frame (`commit_peek`) ou de continuer à tenir la frame courante (hold). - pub unsafe fn peek_next_time_sec(&mut self) -> Result> { + /// attente) et renvoie son temps. Ne touche pas au buffer courant : l'appelant peut + /// ainsi comparer ce pts à une cible avant de décider d'adopter la frame + /// (`commit_peek`) ou de continuer à tenir la frame courante (hold). + /// + /// `NextFrameTime::Unknown` — et non `0.0` — quand le pts est inexploitable : `0.0` + /// satisfait toujours la condition d'adoption, ce qui vidait le flux jusqu'à l'EOF. + pub(crate) unsafe fn peek_next_time_sec(&mut self) -> Result { if !self.has_peek { if !self.receive_into(self.peek_frame)? { - return Ok(None); + return Ok(NextFrameTime::Eof); } self.has_peek = true; } let pts = (*self.peek_frame).best_effort_timestamp; let tb_sec = self.tb_sec(); - Ok(Some(if pts == i64::MIN || tb_sec <= 0.0 { - 0.0 + Ok(if pts == i64::MIN || tb_sec <= 0.0 { + NextFrameTime::Unknown } else { - pts as f64 * tb_sec - })) + NextFrameTime::At(pts as f64 * tb_sec) + }) } /// Promeut la frame de lookahead (décodée par un `peek_next_time_sec` précédent) au /// rang de frame courante — échange de pointeurs, aucune E/S. Ne doit être appelé - /// qu'après un `peek_next_time_sec` ayant renvoyé `Some`. - pub unsafe fn commit_peek(&mut self) -> Result<*mut crate::ffi::AVFrame> { - debug_assert!(self.has_peek, "commit_peek sans peek_next_time_sec préalable"); + /// qu'après un `peek_next_time_sec` ayant renvoyé une frame. + pub(crate) unsafe fn commit_peek(&mut self) -> Result<*mut crate::ffi::AVFrame> { + // `bail!` et non `debug_assert!` : compilée en release, l'assertion disparaissait + // et l'échange promouvait un `AVFrame` jamais rempli, avec un + // `best_effort_timestamp` indéterminé, jusque dans le chemin de présentation. + if !self.has_peek { + anyhow::bail!("commit_peek sans peek_next_time_sec préalable"); + } std::mem::swap(&mut self.frame, &mut self.peek_frame); self.has_peek = false; let pts = (*self.frame).best_effort_timestamp; diff --git a/crates/compositor/src/pipeline_windows.rs b/crates/compositor/src/pipeline_windows.rs index 7d8bd2881..f5b02ac27 100644 --- a/crates/compositor/src/pipeline_windows.rs +++ b/crates/compositor/src/pipeline_windows.rs @@ -17,7 +17,7 @@ use crate::scene::Scene; // `walk_composited_timeline` / `advance_decoder_to` vivaient ici ; ils sont // portables et servent aussi au pipeline macOS et à `gif_export` — voir // `timeline_walk.rs` pour le pourquoi du déplacement. -use crate::timeline_walk::walk_composited_timeline; +use crate::timeline_walk::{walk_composited_timeline, NextFrameTime}; use anyhow::{anyhow, bail, Result}; use std::collections::HashMap; use std::ffi::{c_void, CString}; @@ -582,6 +582,11 @@ impl Decoder { /// Repositionne le flux à la première keyframe (t=0) et vide le codec — pour boucler /// la playback sans réallouer les décodeurs. La fixture démarre sur un IDR (§11). pub(crate) unsafe fn rewind(&mut self) -> Result<()> { + // Même règle que `seek_to` : tout repositionnement invalide le peek en attente. + // Il portait sur « la frame d'après l'ancienne position », qui n'a plus de sens + // ici — sans ça le `next()` suivant promouvait une frame décodée avant le rewind, + // avec son ancien `cur_pts`. + self.has_peek = false; averr(av_seek_frame(self.fmt, self.vidx, 0, AVSEEK_FLAG_BACKWARD), "seek")?; avcodec_flush_buffers(self.dctx); self.sent_eof = false; @@ -765,28 +770,35 @@ impl Decoder { } } - /// Décode la prochaine frame dans le buffer de lookahead et renvoie son temps (s) — - /// `None` à EOF. Cf. `pipeline_macos::Decoder::peek_next_time_sec`. - pub(crate) unsafe fn peek_next_time_sec(&mut self) -> Result> { + /// Décode la prochaine frame dans le buffer de lookahead et renvoie son temps. + /// Cf. `pipeline_macos::Decoder::peek_next_time_sec`. + pub(crate) unsafe fn peek_next_time_sec(&mut self) -> Result { if !self.has_peek { if !self.receive_into(self.peek_frame)? { - return Ok(None); + return Ok(NextFrameTime::Eof); } self.has_peek = true; } let pts = (*self.peek_frame).best_effort_timestamp; let tb_sec = self.tb_sec(); - Ok(Some(if pts == i64::MIN || tb_sec <= 0.0 { - 0.0 + // Sans pts ni time_base exploitables on ne PEUT pas dire si la frame est due : + // `Unknown`, et non `0.0` — qui passait pour « due » à tous les coups. + Ok(if pts == i64::MIN || tb_sec <= 0.0 { + NextFrameTime::Unknown } else { - pts as f64 * tb_sec - })) + NextFrameTime::At(pts as f64 * tb_sec) + }) } /// Promeut la frame de lookahead au rang de frame courante. Cf. /// `pipeline_macos::Decoder::commit_peek`. pub(crate) unsafe fn commit_peek(&mut self) -> Result<*mut AVFrame> { - debug_assert!(self.has_peek, "commit_peek sans peek_next_time_sec préalable"); + // `bail!` et non `debug_assert!` : compilée en release, l'assertion disparaissait + // et l'échange promouvait un `AVFrame` jamais rempli, avec un + // `best_effort_timestamp` indéterminé, jusque dans le chemin de présentation. + if !self.has_peek { + anyhow::bail!("commit_peek sans peek_next_time_sec préalable"); + } std::mem::swap(&mut self.frame, &mut self.peek_frame); self.has_peek = false; let pts = (*self.frame).best_effort_timestamp; diff --git a/crates/compositor/src/timeline_walk.rs b/crates/compositor/src/timeline_walk.rs index 1367d3797..0e0efdad6 100644 --- a/crates/compositor/src/timeline_walk.rs +++ b/crates/compositor/src/timeline_walk.rs @@ -23,6 +23,56 @@ use crate::scene::Scene; use anyhow::Result; use std::collections::HashMap; +/// Ce que le décodeur sait de la PROCHAINE frame, sans l'adopter. +/// +/// Un simple `Option` ne suffisait pas : il confondait « pts inexploitable » et +/// « pts = 0 ». `peek_next_time_sec` renvoyait `0.0` quand `best_effort_timestamp` vaut +/// `i64::MIN` (ou que la time_base est nulle), et `0.0` satisfait TOUJOURS la condition +/// d'adoption — un flux sans pts fiable se faisait donc vider frame après frame jusqu'à +/// l'EOF, exactement le défaut que la sémantique de hold est censée corriger, en pire. +#[derive(Debug, Clone, Copy, PartialEq)] +pub(crate) enum NextFrameTime { + /// pts exploitable de la frame en attente, en secondes. + At(f64), + /// Une frame attend, mais son pts est inexploitable : impossible de dire si elle est + /// due. Le seul repli honnête est d'avancer d'UNE frame puis de rendre la main — + /// c'est le comportement d'avant la sémantique de hold, restreint au flux cassé qui + /// le mérite au lieu d'être la règle générale. + Unknown, + /// Plus aucune frame à décoder. + Eof, +} + +/// Ce que la boucle d'avance doit faire de la frame en attente. +#[derive(Debug, Clone, Copy, PartialEq)] +pub(crate) enum FrameStep { + /// Adopter la frame, puis continuer à chercher. + Commit, + /// Adopter la frame puis s'arrêter (cas `Unknown`, cf. ci-dessus). + CommitAndStop, + /// Ne rien adopter : on tient la frame courante. + Hold, +} + +/// Décision pure de la sémantique de hold — extraite pour être testable sans ffmpeg ni +/// fichier, parce que c'est ici que vivent les cas limites (EOF, pts inconnu, et la +/// frontière exacte « due » vs « pas encore due »). +/// +/// `offset_sec` remet le pts dans le référentiel de la cible (`webcam + offset = écran`). +pub(crate) fn frame_step(next: NextFrameTime, offset_sec: f64, target_sec: f64) -> FrameStep { + match next { + // Plus rien à décoder : on tient la dernière frame connue. C'est ce qui permet à + // un clip dont la dernière frame RÉELLE précède la fin déclarée d'occuper quand + // même toute sa fenêtre — cf. `walk_composited_timeline`. + NextFrameTime::Eof => FrameStep::Hold, + NextFrameTime::Unknown => FrameStep::CommitAndStop, + // `>` et non `>=` : une frame dont le pts vaut EXACTEMENT la cible est due + // (« la dernière frame dont le pts est ≤ t »). + NextFrameTime::At(t) if t + offset_sec > target_sec => FrameStep::Hold, + NextFrameTime::At(_) => FrameStep::Commit, + } +} + /// Avance un décodeur vers `target_source_time`, sémantique de "hold" : à l'instant t on /// affiche la DERNIÈRE frame dont le pts est ≤ t, jamais une frame dont le pts est encore à /// venir. `timeline_offset_sec` remet les pts webcam dans le référentiel écran @@ -40,6 +90,18 @@ use std::collections::HashMap; /// (`peek_next_time_sec`, décodée dans un buffer séparé) et on ne l'adopte /// (`commit_peek`) que si elle est réellement due ; sinon on continue de tenir la frame /// courante, aussi longtemps qu'il le faut. +/// +/// CHANGEMENT DE COMPORTEMENT À L'EXPORT, délibéré : à l'EOF cette fonction renvoie +/// désormais `true` (on tient la dernière frame) là où elle renvoyait `false`, ce qui +/// coupait la boucle du clip (`break 'clip_frames`). Un clip dont la fenêtre déclarée +/// dépasse le dernier pts réel — dernière frame légèrement avant `source_end_sec`, ou +/// piste webcam plus courte que l'écran — ne se termine donc plus en avance : il occupe +/// toute sa fenêtre en tenant sa dernière image. C'est ce que l'audio suppose déjà : +/// `on_clip_end` reçoit le nombre de frames du clip et l'audio est étiré sur la durée +/// DÉCLARÉE (`stretch_clip_pcm_by_speed`), donc une vidéo qui s'arrêtait tôt décalait la +/// jonction audio/vidéo du clip suivant. La contrepartie assumée : une source réellement +/// tronquée produit maintenant une image figée jusqu'au bout de sa fenêtre au lieu de +/// s'arrêter net. pub(crate) unsafe fn advance_decoder_to( decoder: &mut Decoder, target_source_time: f64, @@ -49,14 +111,21 @@ pub(crate) unsafe fn advance_decoder_to( return Ok(false); } loop { - let next_time = match decoder.peek_next_time_sec()? { - Some(t) => t, - None => return Ok(true), // EOF : plus rien à décoder, on tient la dernière frame connue. - }; - if next_time + timeline_offset_sec > target_source_time { - return Ok(true); // la frame suivante n'est pas encore due : hold sur la courante. + let next = decoder.peek_next_time_sec()?; + match frame_step(next, timeline_offset_sec, target_source_time) { + FrameStep::Hold => return Ok(true), + // La frame adoptée devient la frame courante : si la présentation n'a rien + // produit, la boucle n'a plus d'invariant (elle tournerait sur une frame + // nulle jusqu'à l'EOF) — on rend la main comme le faisait le garde d'entrée. + FrameStep::Commit => { + if decoder.commit_peek()?.is_null() { + return Ok(false); + } + } + FrameStep::CommitAndStop => { + return Ok(!decoder.commit_peek()?.is_null()); + } } - decoder.commit_peek()?; } } @@ -233,3 +302,97 @@ pub(crate) unsafe fn walk_composited_timeline( comp.set_timeline_time(None); Ok(frames) } + +#[cfg(test)] +mod tests { + use super::{frame_step, FrameStep, NextFrameTime}; + + /// La cadence de lecture, en une phrase : à 24 fps, une seconde réelle doit adopter 24 + /// frames et pas une de plus. Le bug d'origine (un pas fixe de 1/60 s, une frame par + /// pas) en consommait 60, soit 2,5× trop vite — c'est ce que ce test verrouille. + fn frames_committed_over(fps: f64, window_sec: f64) -> usize { + let mut committed = 0usize; + // La frame 0 est déjà la frame courante : on compte ce qui est ADOPTÉ ensuite. + // Le pts est recalculé depuis un index entier plutôt qu'accumulé, sinon la dérive + // flottante fausse le compte au bout de quelques dizaines de frames. + let mut index = 1u64; + // Une cible qui avance au temps réel, échantillonnée à 60 Hz comme le thread de rendu. + let ticks = (window_sec * 60.0).round() as usize; + for tick in 1..=ticks { + let target = tick as f64 / 60.0; + loop { + let pts = index as f64 / fps; + match frame_step(NextFrameTime::At(pts), 0.0, target) { + FrameStep::Commit => { + committed += 1; + index += 1; + } + _ => break, + } + } + } + committed + } + + #[test] + fn plays_a_24fps_source_at_24_frames_per_second() { + assert_eq!(frames_committed_over(24.0, 1.0), 24); + assert_eq!(frames_committed_over(24.0, 2.0), 48); + } + + #[test] + fn plays_a_60fps_source_at_60_frames_per_second() { + // Le cas qui tombait juste par hasard avant le correctif. + assert_eq!(frames_committed_over(60.0, 1.0), 60); + } + + #[test] + fn plays_a_30fps_source_at_30_frames_per_second() { + assert_eq!(frames_committed_over(30.0, 1.0), 30); + } + + #[test] + fn holds_a_frame_that_is_not_due_yet() { + assert_eq!(frame_step(NextFrameTime::At(0.5), 0.0, 0.4), FrameStep::Hold); + } + + #[test] + fn adopts_a_frame_whose_pts_is_exactly_the_target() { + // « la DERNIÈRE frame dont le pts est ≤ t » : l'égalité est due. + assert_eq!(frame_step(NextFrameTime::At(0.4), 0.0, 0.4), FrameStep::Commit); + } + + #[test] + fn a_sparse_source_holds_across_its_gap() { + // ScreenCaptureKit ne livre rien tant que l'écran ne bouge pas : la frame suivante + // peut être 10 s plus loin. Elle ne doit surtout pas être adoptée à la seconde 1. + assert_eq!(frame_step(NextFrameTime::At(10.0), 0.0, 1.0), FrameStep::Hold); + assert_eq!(frame_step(NextFrameTime::At(10.0), 0.0, 10.0), FrameStep::Commit); + } + + #[test] + fn offset_moves_the_webcam_into_the_screen_clock() { + // webcam + offset = écran : à offset 2 s, une frame webcam à 0.5 s vaut 2.5 s écran. + assert_eq!(frame_step(NextFrameTime::At(0.5), 2.0, 2.4), FrameStep::Hold); + assert_eq!(frame_step(NextFrameTime::At(0.5), 2.0, 2.5), FrameStep::Commit); + } + + #[test] + fn eof_holds_the_last_frame_instead_of_ending_the_clip() { + // Le changement de comportement à l'export, verrouillé : un clip dont la fenêtre + // déclarée dépasse le dernier pts réel occupe toute sa fenêtre en tenant sa + // dernière image, au lieu de s'arrêter net et de décaler l'audio du clip suivant. + assert_eq!(frame_step(NextFrameTime::Eof, 0.0, 1_000.0), FrameStep::Hold); + } + + #[test] + fn an_unusable_pts_advances_exactly_one_frame() { + // Régression : `peek_next_time_sec` renvoyait `0.0` pour un pts inexploitable, et + // `0.0` est toujours ≤ à la cible — le flux se vidait jusqu'à l'EOF d'un seul coup. + assert_eq!(frame_step(NextFrameTime::Unknown, 0.0, 0.0), FrameStep::CommitAndStop); + assert_eq!( + frame_step(NextFrameTime::Unknown, 0.0, 1_000.0), + FrameStep::CommitAndStop + ); + } +} From a8cb4f6ead1c152114efa818921927bc526c776f Mon Sep 17 00:00:00 2001 From: Etienne Lescot Date: Mon, 3 Aug 2026 16:18:48 +0200 Subject: [PATCH 3/3] fix(compositor): hold the webcam at its EOF instead of restarting it every tick MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up: a webcam track shorter than the screen — the camera stopping before the capture does, which the doc already called a normal case — put the player into permanent full-speed decoding. `target_webcam_t` is derived from the screen clock and keeps growing. At the webcam's EOF the catch-up loop seeked back to 0 and left the target untouched, so the NEXT tick restarted the catch-up from t=0 against a target still tens of minutes away and committed frames until the 1000-frame guard cut it off. Every tick. The webcam file was re-decoded end to end, forever, to display a track that had nothing left to show. Holding its last frame is both the fix and the semantics this PR is built on. Once EOF is a hold, the webcam decision is exactly the screen's at export time, so the loop now calls `frame_step` directly instead of carrying its own copy of the four cases — and `frame_step`'s tests cover this path too. 122/122 pass. --- crates/compositor/src/live.rs | 38 +++++++++++++++++------------------ 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/crates/compositor/src/live.rs b/crates/compositor/src/live.rs index e7a876564..9c1c8f382 100644 --- a/crates/compositor/src/live.rs +++ b/crates/compositor/src/live.rs @@ -29,7 +29,7 @@ use crate::config::{self, Cfg}; use crate::cursor::CursorTrack; use crate::d3d::Gpu; use crate::pipeline::Decoder; -use crate::timeline_walk::NextFrameTime; +use crate::timeline_walk::{frame_step, FrameStep, NextFrameTime}; use anyhow::Result; use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; use std::sync::{Arc, Mutex}; @@ -370,9 +370,10 @@ impl Player { /// /// La webcam suit le MÊME principe indépendamment (son propre temps source = /// `screen_time - webcam_offset_sec`, pas un pas 1:1 avec l'écran) : deux pipelines de - /// capture indépendants n'ont pas la même cadence ni les mêmes trous. Elle boucle aussi - /// de façon indépendante à son propre EOF (un clip webcam plus court que l'écran ne doit - /// pas réinitialiser le décodeur écran). + /// capture indépendants n'ont pas la même cadence ni les mêmes trous. Arrivée à son + /// propre EOF — un clip webcam plus court que l'écran, cas normal quand la caméra + /// s'arrête avant la capture — elle TIENT sa dernière image et laisse l'écran + /// continuer seul, plutôt que de reboucler au début. pub unsafe fn step(&mut self, comp: &Compositor, cfg: &Cfg, target_source_time: f64) -> Result { let use_current = self.use_current_on_next_step; self.use_current_on_next_step = false; @@ -416,27 +417,26 @@ impl Player { // même sémantique de hold que l'écran ci-dessus (et que `advance_decoder_to`) : // adopter une frame webcam dont le pts dépasse `target_webcam_t` l'afficherait // en avance sur son heure. Le garde-fou ne joue que contre un cas pathologique. + // + // BUG corrigé : à son EOF la webcam était reseekée à 0 alors que + // `target_webcam_t` continue de croître avec le temps écran. Au tick + // suivant, le rattrapage repartait donc de 0 et réavalait le fichier + // entier vers une cible toujours aussi lointaine — 1000 frames par tick + // (le plafond du garde-fou), en boucle, pour l'éternité. Un décodage + // permanent à fond, pour afficher une webcam qui n'a plus rien à montrer. + // Une fois l'EOF traité comme un hold, la décision webcam est EXACTEMENT + // celle de l'écran à l'export : `frame_step` couvre les quatre cas sans + // rien de spécifique, et ses tests couvrent donc aussi ce chemin. let mut wf = cur; let mut guard = 0u32; loop { - match self.wdec.peek_next_time_sec()? { - NextFrameTime::At(t) if t <= target_webcam_t => { - wf = self.wdec.commit_peek()? - } - NextFrameTime::At(_) => break, // pas encore due : hold sur la courante. - NextFrameTime::Unknown => { - // pts webcam inexploitable : on avance d'UNE frame et on sort, - // au lieu de vider la piste jusqu'à son EOF (ce que `0.0` - // faisait, puisqu'il est toujours ≤ à la cible). + match frame_step(self.wdec.peek_next_time_sec()?, 0.0, target_webcam_t) { + FrameStep::Commit => wf = self.wdec.commit_peek()?, + FrameStep::CommitAndStop => { wf = self.wdec.commit_peek()?; break; } - NextFrameTime::Eof => { - // Fin de la webcam avant l'écran : elle boucle SEULE — l'écran - // garde sa propre position, inchangée. - wf = self.wdec.seek_to(0.0)?; - break; - } + FrameStep::Hold => break, } guard += 1; if guard > 1000 {