diff --git a/crates/compositor/src/compositor_linux.rs b/crates/compositor/src/compositor_linux.rs index 8b950f753..03a0aa6b9 100644 --- a/crates/compositor/src/compositor_linux.rs +++ b/crates/compositor/src/compositor_linux.rs @@ -1497,6 +1497,9 @@ impl Compositor { italic: text.font_style == "italic", underline: text.text_decoration == "underline", align: text.text_align.clone(), + // Absent = "center", le comportement historique : les + // annotations ne changent pas d'un pixel. + valign: text.vertical_align.clone().unwrap_or_default(), box_px: [ quad_px[0].round().max(1.0) as u32, quad_px[1].round().max(1.0) as u32, diff --git a/crates/compositor/src/compositor_macos.rs b/crates/compositor/src/compositor_macos.rs index 2cee70e30..cffc9395d 100644 --- a/crates/compositor/src/compositor_macos.rs +++ b/crates/compositor/src/compositor_macos.rs @@ -1146,6 +1146,9 @@ impl Compositor { italic: text.font_style == "italic", underline: text.text_decoration == "underline", align: text.text_align.clone(), + // Absent = "center", le comportement historique : les + // annotations ne changent pas d'un pixel. + valign: text.vertical_align.clone().unwrap_or_default(), box_px: [quad_px[0].round() as u32, quad_px[1].round() as u32], }; let key = spec.cache_key(); diff --git a/crates/compositor/src/compositor_windows.rs b/crates/compositor/src/compositor_windows.rs index dbaf6f855..51c3a3a28 100644 --- a/crates/compositor/src/compositor_windows.rs +++ b/crates/compositor/src/compositor_windows.rs @@ -1777,6 +1777,9 @@ impl Compositor { italic: text.font_style == "italic", underline: text.text_decoration == "underline", align: text.text_align.clone(), + // Absent = "center", le comportement historique : les + // annotations ne changent pas d'un pixel. + valign: text.vertical_align.clone().unwrap_or_default(), box_px: [quad_px[0].round() as u32, quad_px[1].round() as u32], }; let key = spec.cache_key(); diff --git a/crates/compositor/src/scene.rs b/crates/compositor/src/scene.rs index bf9260bef..2d20e233b 100644 --- a/crates/compositor/src/scene.rs +++ b/crates/compositor/src/scene.rs @@ -261,6 +261,18 @@ pub struct SceneAnnotationText { pub font_style: String, pub text_decoration: String, pub text_align: String, + /// Quelle arête du bloc de texte est épinglée à sa boîte : `"top"` / `"center"` + /// / `"bottom"`. Absent = `"center"`, le comportement historique — les + /// annotations n'émettent jamais la clé et ne bougent donc pas d'un pixel. + /// Les sous-titres l'émettent pour que l'arête ancrée tienne quand le texte + /// gagne une ligne (un bloc centré voit ses deux arêtes se déplacer). + /// + /// `Option` et pas une enum, pour la même raison que `space` : serde + /// rejette une variante d'unité inconnue, donc une valeur future ferait + /// échouer `Scene::from_json` *en entier* sur un binaire plus ancien, au lieu + /// de coûter un seul sous-titre mal placé. + #[serde(default)] + pub vertical_align: Option, #[serde(default)] pub animation: Option, } diff --git a/crates/compositor/src/text_linux.rs b/crates/compositor/src/text_linux.rs index 3e7b45b2b..34bac45e1 100644 --- a/crates/compositor/src/text_linux.rs +++ b/crates/compositor/src/text_linux.rs @@ -37,6 +37,12 @@ pub struct TextSpec { pub underline: bool, /// "left" | "center" | "right". pub align: String, + /// "top" | "center" | "bottom" -- quelle arete du bloc de texte est epinglee + /// a la boite. "center" est le comportement historique (et celui des + /// annotations, qui reproduisent `alignItems: center` de l'overlay web) ; les + /// sous-titres passent "bottom" ou "top" pour que l'arete ancree ne bouge pas + /// quand le texte gagne une ligne. + pub valign: String, /// Taille de la boite en px de sortie. pub box_px: [u32; 2], } @@ -61,6 +67,10 @@ impl TextSpec { } mix(&[self.bold as u8, self.italic as u8, self.underline as u8]); mix(self.align.as_bytes()); + // Juste apres `align`, memes octets et meme position que sur les deux + // autres backends : deux specs ne differant que par l'alignement vertical + // rendraient sinon les pixels l'une de l'autre depuis le cache. + mix(self.valign.as_bytes()); mix(&self.box_px[0].to_le_bytes()); mix(&self.box_px[1].to_le_bytes()); h @@ -194,7 +204,28 @@ impl TextRasterizer { .fold(0.0f32, f32::max); // `max(0)` : un texte plus haut que sa boite reste ancre en haut plutot // que de sortir par le dessus, ou il serait entierement rogne. - let y_offset = (((h as f32) - text_h) * 0.5).max(0.0).round() as i32; + // + // ANCRAGE. `center` est le comportement historique, et reste celui des + // annotations. Les sous-titres epinglent une arete : c'est la seule facon + // que l'arete ancree ne bouge pas quand le texte gagne une ligne, parce + // qu'un bloc centre voit ses DEUX aretes se deplacer. + // + // `anchor_pad` reserve la marge de la plaque DU COTE ANCRE. Sans elle, coller + // le bloc de texte au bord laisse la plaque poser toute sa marge du cote + // oppose et zero du cote ancre : le fond epouse alors le bas des lettres au + // pixel pres tout en respirant deux fois trop au-dessus. Ce qui doit toucher + // le bord de la boite est la PLAQUE, pas les glyphes — c'est elle que le + // viewer voit. Sans plaque, il n'y a rien a reserver. + let has_plate = spec.background[3] > 0.0; + let anchor_pad = if has_plate { pad_y } else { 0.0 }; + let slack_y = ((h as f32) - text_h).max(0.0); + let y_offset = match spec.valign.as_str() { + "top" | "start" => anchor_pad, + "bottom" | "end" => slack_y - anchor_pad, + _ => slack_y * 0.5, + } + .clamp(0.0, slack_y) + .round() as i32; // LA PLAQUE EPOUSE LE BLOC, PAS LA BOITE. Miroir de // `text_macos::block_layout` (en coordonnees descendantes ici, CoreText @@ -385,6 +416,7 @@ mod tests { italic: false, underline: false, align: align.to_owned(), + valign: "center".to_owned(), box_px: [400, 200], } } @@ -566,6 +598,97 @@ mod tests { ); } + #[test] + fn the_anchored_edge_holds_still_when_the_text_gains_a_line() { + // L'INVARIANT de la refonte du placement des sous-titres, en une + // assertion — et celle que l'ancienne architecture ne pouvait pas ecrire. + // + // Un bloc centre voit ses DEUX aretes bouger quand il grandit : c'est + // exactement pourquoi elargir la bande deplacait verticalement le + // sous-titre. Ancre en bas, l'arete basse ne doit pas bouger d'un pixel, + // que le texte tienne sur une ligne ou en reclame trois. + let raster = TextRasterizer::new().expect("rasterizer"); + let (w, h) = (400usize, 200usize); + let long = "un texte assez long pour devoir se replier sur plusieurs lignes"; + + // On mesure la PLAQUE, pas le dernier pixel d'encre. La plaque epouse la boite + // de lignes posee par cosmic-text : c'est exactement ce que ce code epingle et + // ce que le compositeur dessine. Le bas de l'ENCRE, lui, depend des jambages du + // contenu — "Hx" n'en a aucun, "replier" en a — donc il descend plus bas a + // ancrage identique. Ancrer la boite de lignes plutot que l'encre est le + // comportement typographique attendu partout, et c'est la premiere version de + // ce test qui avait tort : elle comparait 184 a 199 et appelait ca une derive. + let plate_of = |valign: &str, content: &str| { + let mut s = spec(content, "center"); + s.valign = valign.to_owned(); + let atlas = raster.build_atlas(&s).expect("atlas"); + let [_, py, _, ph] = atlas.plate; + let rows = ink_rows(&atlas.pixels, w, 0, w); + assert!(!rows.is_empty(), "aucune encre pour {valign:?}"); + (py, py + ph, rows[0], *rows.last().unwrap()) + }; + + let (short_top_edge, short_bottom, _, _) = plate_of("bottom", "Hx"); + let (long_top_edge, long_bottom, _, _) = plate_of("bottom", long); + assert!( + (short_bottom - long_bottom).abs() < 1.0, + "ancrage bas : l'arete basse a bouge de {short_bottom} a {long_bottom} \ + en passant d'une ligne a plusieurs" + ); + + // Et le miroir, pour que « haut » ne soit pas juste « pas bas ». + let (short_top, _, _, _) = plate_of("top", "Hx"); + let (long_top, _, _, _) = plate_of("top", long); + assert!( + (short_top - long_top).abs() < 1.0, + "ancrage haut : l'arete haute a bouge de {short_top} a {long_top}" + ); + + // Le texte long doit vraiment se replier, sinon les deux assertions ci-dessus + // passeraient sur deux rendus identiques et ne prouveraient rien. + assert!( + (long_bottom - long_top_edge) > (short_bottom - short_top_edge) + 1.0, + "le texte « long » ne s'est pas replie : le test ne prouve rien" + ); + + // Les trois ancrages doivent poser la plaque a trois endroits differents, + // sinon `valign` n'est pas applique du tout. + let (top_y, _, _, _) = plate_of("top", "Hx"); + let (ctr_y, _, _, _) = plate_of("center", "Hx"); + let (bot_y, _, _, _) = plate_of("bottom", "Hx"); + assert!( + top_y < ctr_y && ctr_y < bot_y, + "les trois ancrages ne se distinguent pas : haut={top_y} centre={ctr_y} bas={bot_y}" + ); + + // Enfin, l'encre reste dans la plaque qui la porte, et la plaque dans la boite : + // c'est ce qui relie la boite mesuree ci-dessus a ce que le viewer voit. + // + // Tolerance `pad_y`, la meme que `the_plate_hugs_the_text_instead_of_filling_the_box` + // plus haut : l'encre est bornee par la boite de LIGNES, et un glyphe peut deborder + // legerement la sienne (jambages, accents) sans que rien ne soit casse. C'est + // exactement l'hypothese que la premiere version de ce test avait fausse. + for valign in ["top", "bottom"] { + let (py, pb, ink_top, ink_bottom) = plate_of(valign, long); + let (_, pad_y) = crate::text_plate::padding(40.0); + assert!( + (ink_top as f32) >= py - pad_y && (ink_bottom as f32) <= pb + pad_y, + "{valign} : l'encre ({ink_top}..{ink_bottom}) sort de la plaque ({py}..{pb})" + ); + assert!(pb <= (h as f32) + 0.01, "{valign} : la plaque sort de la boite"); + } + } + + #[test] + fn the_vertical_anchor_changes_the_cache_key() { + // Le piege du cache : la cle est partagee entre plateformes, et deux specs + // ne differant que par `valign` rendraient les pixels l'une de l'autre si + // le champ n'y entrait pas. + let mut bottom = spec("Hx", "center"); + bottom.valign = "bottom".to_owned(); + assert_ne!(bottom.cache_key(), spec("Hx", "center").cache_key()); + } + #[test] fn centering_moves_the_ink_off_the_left_edge() { // `spec.align` n'etait jamais applique : tout sortait ferre a gauche diff --git a/crates/compositor/src/text_macos.rs b/crates/compositor/src/text_macos.rs index 57385efe0..5979ab14c 100644 --- a/crates/compositor/src/text_macos.rs +++ b/crates/compositor/src/text_macos.rs @@ -52,6 +52,11 @@ pub struct TextSpec { pub underline: bool, /// "left" | "center" | "right". pub align: String, + /// "top" | "center" | "bottom" — quelle arête du bloc est épinglée à la boîte. + /// "center" est le comportement historique (et celui des annotations) ; les + /// sous-titres passent "bottom" ou "top" pour que l'arête ancrée ne bouge pas + /// quand le texte gagne une ligne. + pub valign: String, /// Taille de la boîte en px de sortie — la mise en page en dépend (retours à la ligne). pub box_px: [u32; 2], } @@ -79,6 +84,10 @@ impl TextSpec { } mix(&[self.bold as u8, self.italic as u8, self.underline as u8]); mix(self.align.as_bytes()); + // Juste après `align`, mêmes octets et même position que sur les deux + // autres backends : deux specs ne différant que par l'alignement vertical + // rendraient sinon les pixels l'une de l'autre depuis le cache. + mix(self.valign.as_bytes()); mix(&self.box_px[0].to_le_bytes()); mix(&self.box_px[1].to_le_bytes()); h @@ -308,6 +317,11 @@ fn block_layout( text_w: CGFloat, text_h: CGFloat, align: u8, + valign: &str, + // Une plaque de fond est-elle dessinée ? Elle décide de la marge à réserver du côté + // ancré — voir `anchor_pad` plus bas. (Commentaire ordinaire et pas `///` : rustc + // refuse un doc-comment sur un paramètre.) + has_plate: bool, font_px: CGFloat, ) -> (CGRect, CGRect) { let (pad_x, pad_y) = plate_padding(font_px); @@ -317,7 +331,24 @@ fn block_layout( // la mesure. On l'étend d'un pixel vers le BAS — donc en abaissant l'origine `y`, pas // en montant le sommet — pour que le haut du texte ne bouge pas d'un poil. const GUARD: CGFloat = 1.0; - let top = ((box_h - text_h) * 0.5).max(0.0); + // ANCRAGE. `center` reste le comportement historique (et celui des annotations, + // qui reproduisent `alignItems: center` de l'overlay web). Les sous-titres + // épinglent une arête : un bloc centré voit ses DEUX arêtes bouger quand il + // gagne une ligne, ce qui déplaçait le sous-titre. `top` est ici une distance + // depuis le HAUT de la boîte, en coordonnées descendantes. + // `anchor_pad` réserve la marge de la plaque DU CÔTÉ ANCRÉ. Sans elle, coller le + // bloc de texte au bord laisse la plaque poser toute sa marge du côté opposé et + // zéro du côté ancré : le fond épouse le bas des lettres au pixel près tout en + // respirant deux fois trop au-dessus. Ce qui doit toucher le bord de la boîte est + // la PLAQUE, pas les glyphes. Sans plaque, il n'y a rien à réserver. + let anchor_pad = if has_plate { pad_y } else { 0.0 }; + let slack_y = (box_h - text_h).max(0.0); + let top = match valign { + "top" | "start" => anchor_pad, + "bottom" | "end" => slack_y - anchor_pad, + _ => slack_y * 0.5, + } + .clamp(0.0, slack_y); let frame_x = (box_w - avail_w) * 0.5; let frame = CGRect { origin: CGPoint { @@ -593,7 +624,16 @@ impl TextRasterizer { let text_h = measured.height.ceil().max(0.0); let (frame_rect, plate_rect) = - block_layout(box_w, box_h, text_w, text_h, alignment, font_px); + block_layout( + box_w, + box_h, + text_w, + text_h, + alignment, + &spec.valign, + spec.background[3] > 0.0, + font_px, + ); // --- plaque de fond, sous le texte --- if spec.background[3] > 0.0 && plate_rect.size.width > 0.0 && plate_rect.size.height > 0.0 @@ -651,6 +691,7 @@ mod tests { italic: false, underline: false, align: "center".into(), + valign: "center".into(), box_px: [256, 256], } } @@ -828,7 +869,7 @@ mod tests { /// Géométrie pure — pas de GPU, pas de CoreText. #[test] fn block_layout_centres_the_frame_and_sizes_the_plate() { - let (frame, plate) = block_layout(1536.0, 238.0, 500.0, 56.0, 2, 48.0); + let (frame, plate) = block_layout(1536.0, 238.0, 500.0, 56.0, 2, "center", true, 48.0); // Cadre centré : autant de vide au-dessus qu'en dessous (repère CG, y vers le haut). let above = 238.0 - (frame.origin.y + frame.size.height); let below = frame.origin.y; @@ -843,11 +884,49 @@ mod tests { fn block_layout_never_lets_the_plate_leave_the_box() { for align in [0u8, 1, 2] { // Bloc plus large et plus haut que la boîte : la plaque doit se contenter d'elle. - let (_, plate) = block_layout(200.0, 60.0, 400.0, 200.0, align, 48.0); + let (_, plate) = block_layout(200.0, 60.0, 400.0, 200.0, align, "center", true, 48.0); assert!(plate.origin.x >= 0.0, "align={align} : x={}", plate.origin.x); assert!(plate.origin.y >= 0.0, "align={align} : y={}", plate.origin.y); assert!(plate.origin.x + plate.size.width <= 200.0 + 0.01, "align={align}"); assert!(plate.origin.y + plate.size.height <= 60.0 + 0.01, "align={align}"); } } + + /// L'invariant de la refonte du placement des sous-titres, en géométrie pure. + /// Un bloc centré voit ses DEUX arêtes bouger quand il grandit ; ancré, l'arête + /// ancrée ne bouge pas. Repère CoreGraphics : `y` monte. + #[test] + fn block_layout_pins_the_anchored_edge_whatever_the_block_height() { + let (box_w, box_h) = (1536.0, 238.0); + let edges = |valign: &str, text_h: f64| { + let (frame, _) = block_layout(box_w, box_h, 500.0, text_h, 2, valign, true, 48.0); + // (bas, haut) en distance depuis le bas de la boîte. + (frame.origin.y, frame.origin.y + frame.size.height) + }; + + // Ancrage bas : l'arête basse est la même à une et à trois lignes. + let (one_bottom, _) = edges("bottom", 56.0); + let (three_bottom, _) = edges("bottom", 168.0); + assert!( + (one_bottom - three_bottom).abs() < 0.01, + "ancrage bas : l'arête basse a bougé de {one_bottom} à {three_bottom}" + ); + + // Ancrage haut : l'arête haute est la même. + let (_, one_top) = edges("top", 56.0); + let (_, three_top) = edges("top", 168.0); + assert!( + (one_top - three_top).abs() < 0.01, + "ancrage haut : l'arête haute a bougé de {one_top} à {three_top}" + ); + + // Et le centrage, lui, fait bien bouger les deux — c'est le comportement + // historique qu'on préserve pour les annotations. + let (c1_bottom, c1_top) = edges("center", 56.0); + let (c3_bottom, c3_top) = edges("center", 168.0); + assert!( + (c1_bottom - c3_bottom).abs() > 1.0 && (c1_top - c3_top).abs() > 1.0, + "le centrage devrait déplacer les deux arêtes" + ); + } } diff --git a/crates/compositor/src/text_windows.rs b/crates/compositor/src/text_windows.rs index 93a5f6df8..e09f4cdc7 100644 --- a/crates/compositor/src/text_windows.rs +++ b/crates/compositor/src/text_windows.rs @@ -33,8 +33,9 @@ use windows::Win32::Graphics::DirectWrite::{ DWriteCreateFactory, IDWriteFactory, DWRITE_FACTORY_TYPE_SHARED, DWRITE_FONT_STRETCH_NORMAL, DWRITE_FONT_STYLE_ITALIC, DWRITE_FONT_STYLE_NORMAL, DWRITE_FONT_WEIGHT_BOLD, DWRITE_FONT_WEIGHT_NORMAL, DWRITE_PARAGRAPH_ALIGNMENT_CENTER, - DWRITE_TEXT_ALIGNMENT_CENTER, DWRITE_TEXT_ALIGNMENT_LEADING, DWRITE_TEXT_ALIGNMENT_TRAILING, - DWRITE_TEXT_METRICS, DWRITE_TEXT_RANGE, + DWRITE_PARAGRAPH_ALIGNMENT_FAR, DWRITE_PARAGRAPH_ALIGNMENT_NEAR, DWRITE_TEXT_ALIGNMENT_CENTER, + DWRITE_TEXT_ALIGNMENT_LEADING, DWRITE_TEXT_ALIGNMENT_TRAILING, DWRITE_TEXT_METRICS, + DWRITE_TEXT_RANGE, }; use windows::Win32::Graphics::Dxgi::Common::DXGI_FORMAT_B8G8R8A8_UNORM; use windows::Win32::Graphics::Dxgi::Common::DXGI_SAMPLE_DESC; @@ -56,6 +57,11 @@ pub struct TextSpec { pub underline: bool, /// "left" | "center" | "right". pub align: String, + /// "top" | "center" | "bottom" — quelle arête du bloc est épinglée à la boîte. + /// "center" est le comportement historique (et celui des annotations) ; les + /// sous-titres passent "bottom" ou "top" pour que l'arête ancrée ne bouge pas + /// quand le texte gagne une ligne. + pub valign: String, /// Taille de la boîte en px de sortie — la mise en page en dépend (retours à la ligne). pub box_px: [u32; 2], } @@ -79,6 +85,10 @@ impl TextSpec { } mix(&[self.bold as u8, self.italic as u8, self.underline as u8]); mix(self.align.as_bytes()); + // Juste après `align`, mêmes octets et même position que sur les deux + // autres backends : deux specs ne différant que par l'alignement vertical + // rendraient sinon les pixels l'une de l'autre depuis le cache. + mix(self.valign.as_bytes()); mix(&self.box_px[0].to_le_bytes()); mix(&self.box_px[1].to_le_bytes()); h @@ -90,6 +100,39 @@ fn wide(s: &str) -> Vec { s.encode_utf16().chain(std::iter::once(0)).collect() } +/// La plaque de fond, en `[left, top, right, bottom]` px dans la boîte, à partir des +/// métriques DirectWrite du bloc mis en page. +/// +/// Fonction pure — et volontairement extraite du chemin de dessin : le rasteriseur +/// Windows exige un device D3D, donc tout ce qui reste inline dans `rasterize` n'est +/// couvert par aucun test. macOS a `block_layout` pour la même raison ; ceci met les +/// deux backends au même niveau, sur le calcul qui décide si la plaque se fait rogner. +/// +/// Les deux annulations qui portent tout : +/// * horizontalement, le texte est dessiné à `pad_x` et commence donc à `pad_x + m.left` : +/// la plaque part de `m.left`, l'inset de la boîte de mise en page et la marge de +/// plaque se compensent exactement, quel que soit l'alignement ; +/// * verticalement, la plaque n'est dessinée QUE si le fond est opaque, et dans ce cas le +/// texte est dessiné à `pad_y` dans une boîte de mise en page rentrée de `2*pad_y` +/// (`anchor_pad` vaut alors `pad_y`). Son haut réel vaut donc `pad_y + m.top` et la +/// plaque va de `m.top` à `m.top + m.height + 2*pad_y`. Ancré en bas +/// (`DWRITE_PARAGRAPH_ALIGNMENT_FAR`), ce second terme tombe pile sur `box_h` : la +/// marge basse tient tout juste au lieu d'être rognée par le `.min()`, et la plaque +/// respire autant en dessous qu'au-dessus du texte. +/// +/// Le bornage à la boîte est ce qui empêche la plaque d'être coupée net par le bord de +/// la texture, où elle perdrait ses coins arrondis. +fn plate_rect(metrics: [f32; 4], box_px: [f32; 2], pad_x: f32, pad_y: f32) -> [f32; 4] { + let [m_left, m_top, m_width, m_height] = metrics; + let [box_w, box_h] = box_px; + [ + m_left.max(0.0), + m_top.max(0.0), + (m_left + m_width + pad_x * 2.0).min(box_w), + (m_top + m_height + pad_y * 2.0).min(box_h), + ] +} + pub struct TextRasterizer { d2d: ID2D1Factory, dwrite: IDWriteFactory, @@ -173,8 +216,14 @@ impl TextRasterizer { "right" => DWRITE_TEXT_ALIGNMENT_TRAILING, _ => DWRITE_TEXT_ALIGNMENT_CENTER, })?; - // Centrage vertical : l'overlay web met `alignItems: center` sur le conteneur. - format.SetParagraphAlignment(DWRITE_PARAGRAPH_ALIGNMENT_CENTER)?; + // ANCRAGE vertical. `center` reproduit `alignItems: center` de l'overlay web et + // reste le comportement des annotations ; les sous-titres épinglent une arête, + // parce qu'un bloc centré voit ses DEUX arêtes bouger quand il gagne une ligne. + format.SetParagraphAlignment(match spec.valign.as_str() { + "top" | "start" => DWRITE_PARAGRAPH_ALIGNMENT_NEAR, + "bottom" | "end" => DWRITE_PARAGRAPH_ALIGNMENT_FAR, + _ => DWRITE_PARAGRAPH_ALIGNMENT_CENTER, + })?; let text: Vec = spec.content.encode_utf16().collect(); // La boîte de mise en page est rentrée de la marge de plaque (cf. `text_plate`), et @@ -184,9 +233,22 @@ impl TextRasterizer { let font_px = spec.font_size_px.max(1.0); let (pad_x, pad_y) = crate::text_plate::padding(font_px); let layout_w = crate::text_plate::layout_width(w as f32, font_px); + // La boîte de mise en page est aussi rentrée VERTICALEMENT de la marge de plaque, + // et le texte se dessine à `anchor_pad`. Sans ça, un ancrage bas colle les glyphes + // au bord de la boîte : la plaque pose alors toute sa marge du côté opposé et zéro + // du côté ancré, et le `.min(h)` plus bas rogne net sa marge basse. Ce qui doit + // toucher le bord de la boîte est la PLAQUE, pas les glyphes. + // + // Conditionné à la présence d'une plaque, comme sur les deux autres backends : + // sans fond il n'y a pas de marge à réserver, et réserver quand même décalerait + // le texte de `pad_y` par rapport à Linux et macOS. Le centrage est rigoureusement + // inchangé dans les deux cas (l'inset et le décalage s'annulent), donc les + // annotations ne bougent pas d'un pixel. + let anchor_pad = if spec.background[3] > 0.0 { pad_y } else { 0.0 }; + let layout_h = ((h as f32) - anchor_pad * 2.0).max(1.0); let layout = self .dwrite - .CreateTextLayout(&text, &format, layout_w, h as f32)?; + .CreateTextLayout(&text, &format, layout_w, layout_h)?; if spec.underline { layout.SetUnderline( true, @@ -214,16 +276,9 @@ impl TextRasterizer { a: spec.background[3], }; let bg_brush = rt.CreateSolidColorBrush(&bg, None)?; - // Le texte commence à `pad_x + m.left`, donc la plaque à `m.left` — l'inset de la - // boîte de mise en page et la marge de plaque s'annulent exactement, quel que soit - // l'alignement. Elle est ensuite bornée à la boîte : au-delà, elle serait coupée - // net par le bord de la texture et perdrait ses coins arrondis. - let rect = D2D_RECT_F { - left: m.left.max(0.0), - top: (m.top - pad_y).max(0.0), - right: (m.left + m.width + pad_x * 2.0).min(w as f32), - bottom: (m.top + m.height + pad_y).min(h as f32), - }; + let [pl, pt, pr, pb] = + plate_rect([m.left, m.top, m.width, m.height], [w as f32, h as f32], pad_x, pad_y); + let rect = D2D_RECT_F { left: pl, top: pt, right: pr, bottom: pb }; let radius = crate::text_plate::radius( font_px, (rect.right - rect.left).max(0.0), @@ -239,7 +294,7 @@ impl TextRasterizer { ); } rt.DrawTextLayout( - D2D_POINT_2F { x: pad_x, y: 0.0 }, + D2D_POINT_2F { x: pad_x, y: anchor_pad }, &layout, &brush, D2D1_DRAW_TEXT_OPTIONS_NONE, @@ -269,10 +324,81 @@ mod tests { italic: false, underline: false, align: "center".into(), + valign: "center".into(), box_px: [400, 120], } } + /// Métriques DirectWrite telles que `SetParagraphAlignment` les produit, pour un + /// bloc de `text_h` px dans une boîte de `box_h` : la mise en page se fait dans + /// `box_h - 2*pad_y` (cf. `rasterize`), et l'alignement décide de `m.top` dedans. + fn metrics_for(valign: &str, box_h: f32, text_h: f32, pad_y: f32) -> [f32; 4] { + let layout_h = (box_h - pad_y * 2.0).max(1.0); + let slack = (layout_h - text_h).max(0.0); + let top = match valign { + "top" => 0.0, + "bottom" => slack, + _ => slack * 0.5, + }; + [0.0, top, 200.0, text_h] + } + + #[test] + fn the_plate_survives_the_bottom_anchor_instead_of_being_clipped() { + // LE risque de la bascule d'ancrage sous Windows. Avec l'ancienne mise en page + // (boîte pleine hauteur, dessin à y=0), `FAR` collait les glyphes au bord et le + // `.min(box_h)` rognait net la marge basse de la plaque. Ici elle doit tomber + // pile sur le bord, marge comprise. + let (box_w, box_h, pad_y) = (400.0f32, 120.0f32, 4.8f32); + let m = metrics_for("bottom", box_h, 56.0, pad_y); + let [_, top, _, bottom] = plate_rect(m, [box_w, box_h], 9.6, pad_y); + + assert!( + (bottom - box_h).abs() < 0.01, + "la plaque ancrée en bas devrait finir sur le bord de la boîte, pas à {bottom}" + ); + assert!(top >= 0.0, "plaque hors boîte par le haut : {top}"); + // Et elle fait bien la hauteur du bloc plus ses deux marges — donc rien n'a été rogné. + assert!( + ((bottom - top) - (56.0 + pad_y * 2.0)).abs() < 0.01, + "la marge de la plaque a été rognée : {}px pour un bloc de 56 + 2*{pad_y}", + bottom - top + ); + } + + #[test] + fn the_centred_plate_is_exactly_where_it_was_before_the_anchor_landed() { + // La bascule d'ancrage a rentré la boîte de mise en page de 2*pad_y ET décalé le + // dessin de pad_y. Les deux DOIVENT s'annuler pour le centrage, sinon toutes les + // annotations existantes bougent. Référence : l'ancien calcul, boîte pleine + // hauteur, `top = m.top - pad_y`, `bottom = m.top + m.height + pad_y`. + let (box_w, box_h, pad_y, text_h) = (400.0f32, 120.0f32, 4.8f32, 56.0f32); + + let legacy_top = (box_h - text_h) * 0.5 - pad_y; + let legacy_bottom = (box_h - text_h) * 0.5 + text_h + pad_y; + + let m = metrics_for("center", box_h, text_h, pad_y); + let [_, top, _, bottom] = plate_rect(m, [box_w, box_h], 9.6, pad_y); + + assert!( + (top - legacy_top).abs() < 0.01 && (bottom - legacy_bottom).abs() < 0.01, + "le centrage a bougé : ({top}, {bottom}) au lieu de ({legacy_top}, {legacy_bottom})" + ); + } + + #[test] + fn the_plate_never_leaves_the_box() { + // Un bloc plus grand que sa boîte : la plaque se contente de la boîte plutôt que + // d'être coupée net par le bord de la texture (elle y perdrait ses coins arrondis). + let (box_w, box_h) = (200.0f32, 60.0f32); + for valign in ["top", "center", "bottom"] { + let m = metrics_for(valign, box_h, 400.0, 4.8); + let [l, t, r, b] = plate_rect(m, [box_w, box_h], 9.6, 4.8); + assert!(l >= 0.0 && t >= 0.0, "{valign} : coin haut-gauche hors boîte ({l}, {t})"); + assert!(r <= box_w + 0.01 && b <= box_h + 0.01, "{valign} : plaque hors boîte"); + } + } + #[test] fn identical_specs_share_a_cache_key() { assert_eq!(spec("Bonjour").cache_key(), spec("Bonjour").cache_key()); @@ -296,6 +422,11 @@ mod tests { other.align = "left".into(); assert_ne!(other.cache_key(), base, "alignement"); other = spec("Bonjour"); + // Sans ça, deux sous-titres ne différant que par l'ancrage se partageraient + // une texture et rendraient les pixels l'un de l'autre. + other.valign = "bottom".into(); + assert_ne!(other.cache_key(), base, "ancrage vertical"); + other = spec("Bonjour"); // La taille de boîte compte : elle décide des retours à la ligne, donc des pixels. other.box_px = [401, 120]; assert_ne!(other.cache_key(), base, "boîte"); diff --git a/electron/ipc/handlers.ts b/electron/ipc/handlers.ts index 8596dfafd..958ef6d96 100644 --- a/electron/ipc/handlers.ts +++ b/electron/ipc/handlers.ts @@ -1353,6 +1353,12 @@ function readNativeWindowsEncoderSelection(output: string) { // which is what `salvageNativeWindowsFragmentedCapture` asks. container?: string; preferSoftwareEncoder?: boolean; + // Whether BeginWriting() actually landed on a hardware H.264 MFT, as + // opposed to `video` above, which only says which configuration path + // was tried. "default" plus a software runtime means the machine never + // got hardware acceleration in the first place -- see + // kVideoEncoderRuntime* in mf_encoder.h. + videoEncoderRuntime?: string; }; } catch { return null; @@ -1658,6 +1664,66 @@ async function resolveMediaLinksForVideo(videoPath: string): Promise<{ return { resolvedVia: "none" }; } +/** + * Writes the diagnostic bundle a bug report needs: app/OS facts, the native + * helpers' raw stdout/stderr (which is where `[stop-timing]` and + * `encoder-selection` land — see nativeWindowsCaptureStop.ts), and the main + * process's own recent console output. Shared by the renderer's IPC call and + * the menu/tray "Save Diagnostics" entry point in main.ts, which has no + * renderer-side `projectState`/`logs` to offer and does not need to. + */ +export async function exportDiagnosticFile(payload: { + error: string; + stack?: string; + projectState: unknown; + logs: string[]; +}) { + const { filePath, canceled } = await dialog.showSaveDialog({ + title: "Save Diagnostic File", + defaultPath: `openscreen-diagnostic-${Date.now()}.json`, + filters: [{ name: "JSON", extensions: ["json"] }], + }); + + if (canceled || !filePath) return { success: false, canceled: true }; + + const HELPER_OUTPUT_MAX_BYTES = 64 * 1024; + const tail = (s: string, max: number) => (s.length <= max ? s : s.slice(s.length - max)); + + const diagnostic = { + timestamp: new Date().toISOString(), + appVersion: app.getVersion(), + platform: process.platform, + arch: process.arch, + // The same fact the About box leads with, and for the same reason: it is what + // explains why a copy does or does not offer an update check. This file is the + // artifact users actually attach, so it must not be the one that omits it. + channel: getInstallChannel(), + osRelease: os.release(), + osVersion: os.version(), + totalMemoryMB: Math.round(os.totalmem() / 1024 / 1024), + nodeVersion: process.versions.node, + electronVersion: process.versions.electron, + chromeVersion: process.versions.chrome, + error: payload.error, + stack: payload.stack, + projectState: payload.projectState, + recentLogs: payload.logs, + helperOutput: { + windows: tail(nativeWindowsCaptureOutput, HELPER_OUTPUT_MAX_BYTES), + mac: tail(nativeMacCaptureOutput, HELPER_OUTPUT_MAX_BYTES), + }, + mainProcessLogs: mainLogBuffer.snapshot(), + }; + + try { + await fs.writeFile(filePath, JSON.stringify(diagnostic, null, 2), "utf-8"); + return { success: true, path: filePath }; + } catch (error) { + console.error("Failed to write diagnostic file:", error); + return { success: false, error: String(error) }; + } +} + export function registerIpcHandlers( createEditorWindow: () => void, createSourceSelectorWindow: () => BrowserWindow, @@ -2531,6 +2597,7 @@ export function registerIpcHandlers( path: outputPath, helperPath, videoEncoderSelection: encoderSelection?.video ?? null, + videoEncoderRuntime: encoderSelection?.videoEncoderRuntime ?? null, webcamUnavailable, microphoneDefaulted, }; @@ -4085,55 +4152,8 @@ export function registerIpcHandlers( ipcMain.handle( "save-diagnostic", - async ( - _, - payload: { error: string; stack?: string; projectState: unknown; logs: string[] }, - ) => { - const { filePath, canceled } = await dialog.showSaveDialog({ - title: "Save Diagnostic File", - defaultPath: `openscreen-diagnostic-${Date.now()}.json`, - filters: [{ name: "JSON", extensions: ["json"] }], - }); - - if (canceled || !filePath) return { success: false, canceled: true }; - - const HELPER_OUTPUT_MAX_BYTES = 64 * 1024; - const tail = (s: string, max: number) => (s.length <= max ? s : s.slice(s.length - max)); - - const diagnostic = { - timestamp: new Date().toISOString(), - appVersion: app.getVersion(), - platform: process.platform, - arch: process.arch, - // The same fact the About box leads with, and for the same reason: it is what - // explains why a copy does or does not offer an update check. This file is the - // artifact users actually attach, so it must not be the one that omits it. - channel: getInstallChannel(), - osRelease: os.release(), - osVersion: os.version(), - totalMemoryMB: Math.round(os.totalmem() / 1024 / 1024), - nodeVersion: process.versions.node, - electronVersion: process.versions.electron, - chromeVersion: process.versions.chrome, - error: payload.error, - stack: payload.stack, - projectState: payload.projectState, - recentLogs: payload.logs, - helperOutput: { - windows: tail(nativeWindowsCaptureOutput, HELPER_OUTPUT_MAX_BYTES), - mac: tail(nativeMacCaptureOutput, HELPER_OUTPUT_MAX_BYTES), - }, - mainProcessLogs: mainLogBuffer.snapshot(), - }; - - try { - await fs.writeFile(filePath, JSON.stringify(diagnostic, null, 2), "utf-8"); - return { success: true, path: filePath }; - } catch (error) { - console.error("Failed to write diagnostic file:", error); - return { success: false, error: String(error) }; - } - }, + async (_, payload: { error: string; stack?: string; projectState: unknown; logs: string[] }) => + exportDiagnosticFile(payload), ); // One instance each, not one per call. DocumentService serialises saves of a diff --git a/electron/main.ts b/electron/main.ts index 85eb063b8..a85629bf3 100644 --- a/electron/main.ts +++ b/electron/main.ts @@ -41,7 +41,11 @@ import { } from "./globalShortcut"; import { mainT, setMainLocale } from "./i18n"; import { getInstallChannel, offersUpdateCheck, platformOwnsUpdates } from "./install-channel"; -import { getSelectedDesktopSource, registerIpcHandlers } from "./ipc/handlers"; +import { + exportDiagnosticFile, + getSelectedDesktopSource, + registerIpcHandlers, +} from "./ipc/handlers"; import { installMainProcessErrorGuards } from "./main-process-errors"; import { registerSttIpc, shutdownStt } from "./stt"; import { checkLatestRelease } from "./update-checker"; @@ -211,6 +215,11 @@ function setupApplicationMenu() { role: "about", label: mainT("common", "actions.about") || "About OpenScreen", }, + { type: "separator" as const }, + { + label: mainT("common", "actions.saveDiagnostics") || "Save Diagnostics", + click: runSaveDiagnostics, + }, // Omitted entirely — here, in the Help menu and in the tray — where a package // manager owns the update. See `canOfferUpdateCheck`. ...(canOfferUpdateCheck() @@ -369,6 +378,11 @@ function setupApplicationMenu() { label: mainT("common", "actions.about") || "About OpenScreen", click: runAboutDialog, }, + { type: "separator" as const }, + { + label: mainT("common", "actions.saveDiagnostics") || "Save Diagnostics", + click: runSaveDiagnostics, + }, ], }); } @@ -519,6 +533,47 @@ function runUpdateCheck() { }); } +/** + * Menu and tray entry point for exporting a diagnostic bundle. The backend + * (`exportDiagnosticFile`) and its "Save Diagnostics" label already existed — + * nothing in the app ever called it (getopenscreen/openscreen#460). Reveals + * the written file on success, the same confirmation the export flow's "Show + * in folder" gives, so there is no need for a second dialog on top of the + * native Save dialog the user already went through. + * + * No renderer `projectState`/`logs` to attach from here, unlike the in-app + * crash path this shares a payload shape with — the diagnostic value for a + * capture bug is almost entirely `helperOutput`/`mainProcessLogs`, which + * `exportDiagnosticFile` reads straight from the main process regardless. + */ +function runSaveDiagnostics() { + exportDiagnosticFile({ error: "Manual diagnostic export", projectState: null, logs: [] }) + .then((result) => { + if (result.canceled) return; + if (!result.success) { + // exportDiagnosticFile resolves rather than rejects on a write + // failure, so this is the branch that turns "user picked a save + // location and got silence" into a visible error instead of a + // menu action that looks like it did nothing. + showMessageBox({ + type: "error", + title: PRODUCT_NAME, + message: mainT("dialogs", "export.failed") || "Export Failed", + detail: result.error, + }).catch((error) => { + console.error("[diagnostics] failure dialog failed", error); + }); + return; + } + if (result.path) { + shell.showItemInFolder(result.path); + } + }) + .catch((error) => { + console.error("[diagnostics] save failed", error); + }); +} + /** Mirrors the flag that already drives the tray icon. An update must never interrupt a take — * and on Windows it physically cannot, because the capture helpers spawn from inside the * install directory and NSIS cannot overwrite a running .exe. */ @@ -730,6 +785,14 @@ function updateTrayMenu(recording: boolean = false) { label: mainT("common", "actions.about") || "About OpenScreen", click: runAboutDialog, }, + // Right next to About, and reachable without opening any window: this is the + // one place in the app most likely to still be usable right after a recording + // failed to stop, which is exactly when the [stop-timing]/encoder-selection + // lines this exports are worth the most (getopenscreen/openscreen#460). + { + label: mainT("common", "actions.saveDiagnostics") || "Save Diagnostics", + click: runSaveDiagnostics, + }, { type: "separator" as const }, { label: mainT("common", "actions.quit") || "Quit", diff --git a/electron/native/screencapturekit/Sources/OpenScreenScreenCaptureKitHelper/ScreenCaptureRecorder.swift b/electron/native/screencapturekit/Sources/OpenScreenScreenCaptureKitHelper/ScreenCaptureRecorder.swift index 75163dee7..2e88e5c41 100644 --- a/electron/native/screencapturekit/Sources/OpenScreenScreenCaptureKitHelper/ScreenCaptureRecorder.swift +++ b/electron/native/screencapturekit/Sources/OpenScreenScreenCaptureKitHelper/ScreenCaptureRecorder.swift @@ -802,6 +802,15 @@ final class ScreenCaptureRecorder: NSObject, SCStreamOutput, SCStreamDelegate { struct OpenScreenScreenCaptureKitHelper { static func main() async { do { + // This helper is a plain command-line executable, so nothing has connected it to + // the window server yet. `SCContentFilter(desktopIndependentWindow:)` reaches into + // SkyLight (`SLSGetDisplaysWithRect`) to find the display a window sits on, and + // SkyLight aborts with `CGS_REQUIRE_INIT` when CoreGraphics was never initialised + // in the process — so every window capture crashed before it produced a frame, + // while display capture (which never resolves a rect) worked fine. Touching any + // CoreGraphics display API first performs that initialisation. + _ = CGMainDisplayID() + guard CommandLine.arguments.count == 2 else { throw HelperError.invalidArguments } diff --git a/electron/native/wgc-capture/src/main.cpp b/electron/native/wgc-capture/src/main.cpp index 1479bc842..a9e21d45c 100644 --- a/electron/native/wgc-capture/src/main.cpp +++ b/electron/native/wgc-capture/src/main.cpp @@ -682,6 +682,16 @@ int main(int argc, char* argv[]) { // ordinary hardware, so the stop path can be regression-tested at all. const int testStallReadbackMs = std::max(0, readEnvInt("OPENSCREEN_WGC_TEST_STALL_READBACK_MS", 0)); + // Test-only: stall the WGC frame *callback* itself while it holds the + // same frame lock, rather than the writer's readback -- the shape + // getopenscreen/openscreen#460 actually reproduced on Intel HD 520 + // ("A WGC frame callback did not finish"). Distinct from + // testStallReadbackMs above because quiesceCapture()'s drain only ever + // sees the callback side: a stall placed in the writer instead leaves + // callbacksInFlight_ at zero and wgcDrained true, which cannot exercise + // the video-writer-join skip this stall exists to test. + const int testStallFrameCallbackMs = + std::max(0, readEnvInt("OPENSCREEN_WGC_TEST_STALL_FRAME_CALLBACK_MS", 0)); std::cout << "{\"event\":\"ready\",\"schemaVersion\":2}" << std::endl; @@ -865,7 +875,14 @@ int main(int argc, char* argv[]) { << "\",\"container\":\"" << encoder.containerFormat() << "\",\"preferSoftwareEncoder\":" << (config.preferSoftwareEncoder ? "true" : "false") - << "}" << std::endl; + // What BeginWriting() actually landed on, not what the "video" + // field above asked for -- see kVideoEncoderRuntime* in + // mf_encoder.h. "default" plus "software" here means the machine + // never got a hardware encoder in the first place, which is a + // different bug report than "default" plus "hardware" stalling + // on stop. + << ",\"videoEncoderRuntime\":\"" << encoder.videoEncoderRuntime() + << "\"}" << std::endl; MFEncoder webcamEncoder; if (writeSeparateWebcam) { MFEncoderOptions webcamEncoderOptions = encoderOptions; @@ -924,6 +941,18 @@ int main(int argc, char* argv[]) { } } + // Gated on an already-arrived first frame: main() blocks up to 10s + // waiting for firstFrameWritten before it will even print + // recording-started, a startup budget this stall is meant to outlast + // (it needs to still be asleep when `stop` arrives, seconds later). + // Stalling the first frame trips that unrelated timeout instead of + // reaching the steady-state shutdown path this exists to test, and + // does not match the real report either -- getopenscreen/openscreen + // #460's diagnostic shows recording-started succeeding before the + // hang. + if (testStallFrameCallbackMs > 0 && firstFrameWritten.load()) { + std::this_thread::sleep_for(std::chrono::milliseconds(testStallFrameCallbackMs)); + } session.context()->CopyResource(latestFrameTexture.Get(), texture); latestFrameTimestampHns = timestampHns; if (!firstFrameWritten.exchange(true)) { @@ -1413,8 +1442,47 @@ int main(int argc, char* argv[]) { } logStopStep("audio-mixer"); beginStopStep("video-writer-join", stepBudgetMs); - stopVideoWriter(); - logStopStep("video-writer-join"); + if (wgcDrained) { + stopVideoWriter(); + logStopStep("video-writer-join"); + } else { + // wgc-quiesce already reported the frame callback stuck inside the + // driver (getopenscreen/openscreen#460 on Intel HD 520: a + // CopyResource that never returns), still holding the same + // frame-state `mutex` writeVideoFrames takes for its own + // per-iteration wait -- the one it also needs to notice + // stopRequested. Joining is not a step that can time out here, it is + // one that cannot ever succeed, and this is not the only step that + // assumed it would: encoder.finalize() below resets the very D3D + // device/context a still-blocked writer thread might resume touching + // the moment that lock frees, and quiesceCapture()/stop() already + // treat "leave everything alone and let process exit reclaim it" as + // the only safe response to exactly this state. So this ends the + // process here, on this thread, rather than pretending the rest of a + // clean shutdown is reachable -- which cost nothing extra before + // today: the same TerminateProcess happened anyway, just + // stepBudgetMs later, once this step's own watchdog gave up waiting + // on a join that could never return. detach() first, not because + // TerminateProcess needs it (it does not touch the C++ runtime, no + // std::thread destructor runs), but so nothing between here and the + // kill can trip over a still-joinable thread. + // + // The fragmented sink writes moof+mdat incrementally, roughly once a + // second, so this is not a new source of loss: whatever was already + // on disk before the callback wedged is on disk regardless of + // whether Finalize() ever runs, on this path or the slower one it + // replaces. + videoWriterThread.detach(); + std::cerr << "[stop-timing] step=video-writer-join elapsed_ms=" << stopElapsedMs() + << " phase=abandoned encode_stage=" << encoder.encodeStage() + << " audio_stage=" << encoder.audioStage() << " reason=frame-callback-stuck" + << std::endl; + std::cout << "{\"event\":\"stop-timeout\",\"schemaVersion\":2,\"step\":\"video-writer-join\"}" + << std::endl; + std::cout.flush(); + std::cerr.flush(); + TerminateProcess(GetCurrentProcess(), 3); + } if (usesDxgiInput) { std::cerr << "[frame-drops] gpu_bridge_contended=" << contendedFrames.load() << std::endl; } @@ -1423,7 +1491,9 @@ int main(int argc, char* argv[]) { // the encoder's GPU readback, and audioMixer->stop() joined the only other // thread that writes to it. MFEncoder's own writerMutex_ deliberately does // NOT cover copyFrameToBuffer, so finalizing before those joins would race - // the staging texture -- do not reorder these. + // the staging texture -- do not reorder these. Reaching this line at all + // means wgcDrained was true above: the branch that was not is a + // TerminateProcess call, not a fallthrough. beginStopStep("encoder-finalize", shutdownBudgetMs); const bool screenFinalized = encoder.finalize(); logStopStep("encoder-finalize"); diff --git a/electron/native/wgc-capture/src/mf_encoder.cpp b/electron/native/wgc-capture/src/mf_encoder.cpp index 4058ca249..4130b1326 100644 --- a/electron/native/wgc-capture/src/mf_encoder.cpp +++ b/electron/native/wgc-capture/src/mf_encoder.cpp @@ -140,7 +140,7 @@ enum class SinkWriterCreateStage { SoftwareEncoderRegistration, CreateAttributes, DisableHardwareTransforms, - ConfigureDxgiManager, + EnableHardwareTransforms, CreateFile, CreateFragmentedMediaSink, CreateSinkWriter, @@ -248,10 +248,30 @@ HRESULT createSinkWriter( failedStage = SinkWriterCreateStage::DisableHardwareTransforms; return hr; } - } else if (dxgiDeviceManager != nullptr) { - HRESULT hr = MFCreateAttributes(&attributes, 3); + } else { + // Ask for hardware transforms whenever software is not forced -- + // whether or not a DXGI device manager came with the request. + // MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS defaults to FALSE, and + // leaving it unset (the old behaviour on the plain CPU-readback path) + // meant the sink writer never considered a hardware H.264 MFT even + // when one was registered and working: every "default" recording + // landed on the same software encoder forceSoftwareEncoder asks for + // explicitly, on any machine that had not separately opted into + // OPENSCREEN_WGC_ENABLE_DXGI_INPUT (getopenscreen/openscreen#460, + // confirmed by videoEncoderRuntime on real hardware: "default" read + // back "software" until the DXGI path was turned on, on a machine + // whose encoder is hardware-capable either way). + // + // A hardware MFT does not require the D3D manager to accept samples: + // without one it manages its own device and takes system-memory + // samples the same way the software encoder does, which is exactly + // the CPU-readback path this branch also serves. So the attribute is + // set unconditionally here; only the manager itself stays behind the + // null check, since supplying a manager the caller does not have would + // be undefined rather than merely declined. + HRESULT hr = MFCreateAttributes(&attributes, dxgiDeviceManager != nullptr ? 3 : 1); if (FAILED(hr)) { - std::cerr << "ERROR: MFCreateAttributes(DXGI sink writer) failed (hr=0x" + std::cerr << "ERROR: MFCreateAttributes(sink writer) failed (hr=0x" << std::hex << hr << std::dec << ")" << std::endl; failedStage = SinkWriterCreateStage::CreateAttributes; return hr; @@ -260,15 +280,17 @@ HRESULT createSinkWriter( if (FAILED(hr)) { std::cerr << "ERROR: Set MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS(TRUE) failed (hr=0x" << std::hex << hr << std::dec << ")" << std::endl; - failedStage = SinkWriterCreateStage::ConfigureDxgiManager; + failedStage = SinkWriterCreateStage::EnableHardwareTransforms; return hr; } - hr = attributes->SetUnknown(MF_SINK_WRITER_D3D_MANAGER, dxgiDeviceManager); - if (FAILED(hr)) { - std::cerr << "ERROR: Set MF_SINK_WRITER_D3D_MANAGER failed (hr=0x" - << std::hex << hr << std::dec << ")" << std::endl; - failedStage = SinkWriterCreateStage::ConfigureDxgiManager; - return hr; + if (dxgiDeviceManager != nullptr) { + hr = attributes->SetUnknown(MF_SINK_WRITER_D3D_MANAGER, dxgiDeviceManager); + if (FAILED(hr)) { + std::cerr << "ERROR: Set MF_SINK_WRITER_D3D_MANAGER failed (hr=0x" + << std::hex << hr << std::dec << ")" << std::endl; + failedStage = SinkWriterCreateStage::EnableHardwareTransforms; + return hr; + } } } @@ -382,6 +404,69 @@ bool resolveStreamSinkIndex(IMFMediaSink* mediaSink, const GUID& majorType, DWOR return false; } +// Did the video stream's encoder MFT actually land on hardware? +// +// BeginWriting() succeeding says nothing about this: even on the "default" +// path (see kVideoEncoderRuntime* in mf_encoder.h), which does now ask for +// MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS, that is a request and not a +// guarantee -- Media Foundation is still free to hand the sink writer a +// software MFT when no hardware one is registered or the driver refuses it. +// The only way to know which one it actually picked is to ask the pipeline it +// built, after the fact -- IMFSinkWriterEx::GetTransformForStream walks the +// MFTs the sink writer inserted for a stream, and a hardware MFT instance is +// required to expose MFT_ENUM_HARDWARE_URL_Attribute on its own attribute +// store (not just on the IMFActivate MFTEnumEx returns), which is what +// distinguishes it from a software one at this point. +// +// Every failure path here returns "unknown" rather than guessing: this runs +// after the sink writer is already committed to, so it must never be able to +// fail configureSinkWriterAttempt, and a wrong hardware/software guess in a +// bug report would be worse than an admitted "could not tell." +const char* detectVideoEncoderRuntime(IMFSinkWriter* sinkWriter, DWORD videoStreamIndex) { + Microsoft::WRL::ComPtr sinkWriterEx; + if (FAILED(sinkWriter->QueryInterface(IID_PPV_ARGS(&sinkWriterEx)))) { + return kVideoEncoderRuntimeUnknown; + } + + for (DWORD mftIndex = 0;; mftIndex += 1) { + GUID category{}; + Microsoft::WRL::ComPtr transform; + const HRESULT hr = + sinkWriterEx->GetTransformForStream(videoStreamIndex, mftIndex, &category, &transform); + if (hr == MF_E_INVALIDINDEX) { + // Walked the whole pipeline (converters, the encoder, anything + // else the topology loader inserted) without finding an encoder + // node. Should not happen -- an H.264 stream has to have one -- + // but this is diagnostics code, not the recording path, so an + // unexpected shape is "unknown", not a crash. + return kVideoEncoderRuntimeUnknown; + } + if (FAILED(hr)) { + return kVideoEncoderRuntimeUnknown; + } + if (category != MFT_CATEGORY_VIDEO_ENCODER) { + // A colour converter or similar the sink writer inserted ahead of + // the encoder. Keep walking; the encoder is further down. + continue; + } + + Microsoft::WRL::ComPtr transformAttributes; + if (FAILED(transform->GetAttributes(&transformAttributes))) { + return kVideoEncoderRuntimeUnknown; + } + UINT32 hardwareUrlLength = 0; + const HRESULT hardwareUrlHr = + transformAttributes->GetStringLength(MFT_ENUM_HARDWARE_URL_Attribute, &hardwareUrlLength); + if (SUCCEEDED(hardwareUrlHr)) { + return kVideoEncoderRuntimeHardware; + } + if (hardwareUrlHr == MF_E_ATTRIBUTENOTFOUND) { + return kVideoEncoderRuntimeSoftware; + } + return kVideoEncoderRuntimeUnknown; + } +} + void logSinkWriterCreateFailure( HRESULT sinkWriterHr, const char* createCall, @@ -513,6 +598,10 @@ const char* MFEncoder::videoEncoderSelection() const { return videoEncoderSelection_; } +const char* MFEncoder::videoEncoderRuntime() const { + return videoEncoderRuntime_; +} + const char* MFEncoder::containerFormat() const { return containerFormat_; } @@ -600,6 +689,7 @@ bool MFEncoder::initialize( // encoder, never reaching the software encoder the knob is aimed at. useDxgiInput_ = options.useDxgiInput && !options.injectDefaultSinkWriterFailureOnce; videoEncoderSelection_ = kVideoEncoderSelectionDefault; + videoEncoderRuntime_ = kVideoEncoderRuntimeUnknown; if (!succeeded(MFStartup(MF_VERSION), "MFStartup")) { return false; @@ -689,6 +779,7 @@ bool MFEncoder::initialize( audioStreamIndex_ = 0; hasAudioStream_ = false; videoEncoderSelection_ = kVideoEncoderSelectionDefault; + videoEncoderRuntime_ = kVideoEncoderRuntimeUnknown; containerFormat_ = kContainerFormatMp4; }; @@ -780,7 +871,7 @@ bool MFEncoder::initialize( "SetInputMediaType")) { return false; } - if (useDxgiInput_) { + if (!forceSoftwareEncoder) { applyHardwareRateControl(std::max(1, bitrate)); } if (!succeeded(sinkWriter_->BeginWriting(), "BeginWriting")) { @@ -788,6 +879,7 @@ bool MFEncoder::initialize( } videoEncoderSelection_ = selection; + videoEncoderRuntime_ = detectVideoEncoderRuntime(sinkWriter_.Get(), videoStreamIndex_); containerFormat_ = fragmented ? kContainerFormatFragmentedMp4 : kContainerFormatMp4; return true; }; @@ -1204,12 +1296,17 @@ bool MFEncoder::initializeVideoProcessor() { } void MFEncoder::applyHardwareRateControl(int bitrate) { - // The D3D manager switches the sink writer onto a hardware MFT, and those - // default to constant bitrate: a static desktop then spends the full - // configured budget doing nothing, 16.9 Mbps measured against the 1.95 the - // software encoder the CPU path lands on produced for the same screen. Same - // budget, opposite reading of it. Ask for VBR so the GPU path spends what - // the picture costs, which is what users have been getting all along. + // Enabling MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS can hand the sink + // writer a hardware MFT, and those default to constant bitrate: a static + // desktop then spends the full configured budget doing nothing, 16.9 Mbps + // measured against the 1.95 the software encoder produced for the same + // screen. Same budget, opposite reading of it. Ask for VBR so a hardware + // encoder spends what the picture costs, which is what the software + // encoder was already doing. Called whenever hardware transforms were + // requested, DXGI device manager or not (getopenscreen/openscreen#460) -- + // whether the sink writer actually landed on hardware is not knowable + // until after BeginWriting() (see MFEncoder::videoEncoderRuntime()), and + // this call is a no-op on a software MFT that ignores or lacks the knob. // // Best effort on purpose. An encoder that exposes neither knob still // produces a valid recording, and a bitrate we could not pin down is not diff --git a/electron/native/wgc-capture/src/mf_encoder.h b/electron/native/wgc-capture/src/mf_encoder.h index f8370874c..19fac7004 100644 --- a/electron/native/wgc-capture/src/mf_encoder.h +++ b/electron/native/wgc-capture/src/mf_encoder.h @@ -42,6 +42,28 @@ constexpr const char* kVideoEncoderSelectionDefault = "default"; constexpr const char* kVideoEncoderSelectionSoftwarePreferred = "software-preferred"; constexpr const char* kVideoEncoderSelectionSoftwareFallback = "software-fallback"; +// Whether BeginWriting() actually landed on a hardware-accelerated H.264 MFT. +// +// videoEncoderSelection() above says which *path* initialize() took -- +// whether the DXGI GPU pipeline was asked for, or software was forced -- but +// none of those labels says what Media Foundation itself picked, and that +// matters even now that createSinkWriter asks for hardware transforms on +// every path but the forced-software one: MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS +// asks, it does not guarantee -- a machine with no hardware H.264 MFT +// registered, or one whose driver refuses it, still lands on software. That +// gap is exactly what this exists to close for a bug report: "default" alone +// cannot tell a real hardware encode apart from software Media Foundation +// picked anyway, which was the whole ambiguity behind a slow-CPU stop timeout +// (getopenscreen/openscreen#460) before this field existed. +constexpr const char* kVideoEncoderRuntimeHardware = "hardware"; +constexpr const char* kVideoEncoderRuntimeSoftware = "software"; +// Introspection itself failed (no IMFSinkWriterEx, no encoder node found in +// the resolved topology, GetAttributes refused). Reported as its own value +// rather than guessed into hardware or software, because a bug report that +// cannot tell "we checked and it's software" from "we couldn't check" would +// draw the wrong conclusion either way. +constexpr const char* kVideoEncoderRuntimeUnknown = "unknown"; + // Which MP4 flavour the recording was actually written in. The fragmented sink // writes a self-describing moof+mdat pair roughly every second, so a helper the // shutdown watchdog force-exits leaves a file that plays up to the last @@ -97,6 +119,9 @@ class MFEncoder { bool writeAudio(const BYTE* data, DWORD byteCount, int64_t timestampHns, int64_t durationHns); bool finalize(); const char* videoEncoderSelection() const; + // Best-effort, read only after initialize() returns true. See the + // kVideoEncoderRuntime* constants above for what each value means. + const char* videoEncoderRuntime() const; // Which container initialize() settled on, which is not necessarily the one // it asked for: the fragmented sink degrades to the plain one rather than // failing a recording. A bug report that cannot tell the two apart cannot @@ -202,5 +227,6 @@ class MFEncoder { bool finalized_ = false; bool useDxgiInput_ = false; const char* videoEncoderSelection_ = kVideoEncoderSelectionDefault; + const char* videoEncoderRuntime_ = kVideoEncoderRuntimeUnknown; const char* containerFormat_ = kContainerFormatMp4; }; diff --git a/index.html b/index.html index a7ee0237c..a5d20360c 100644 --- a/index.html +++ b/index.html @@ -5,6 +5,20 @@ +
diff --git a/package-lock.json b/package-lock.json index e0ea6e09c..745a74e57 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "openscreen", - "version": "1.9.6", + "version": "1.10.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "openscreen", - "version": "1.9.6", + "version": "1.10.0", "dependencies": { "@fix-webm-duration/fix": "^1.0.1", "@langchain/anthropic": "^1.3.26", diff --git a/package.json b/package.json index 412f6dfb2..056e4aecb 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "openscreen", "private": true, - "version": "1.9.6", + "version": "1.10.0", "description": "Record your screen and polish the demo", "homepage": "https://getopenscreen.com/", "license": "MIT", diff --git a/scripts/test-windows-wgc-helper.mjs b/scripts/test-windows-wgc-helper.mjs index e1dc48148..ee40838e6 100644 --- a/scripts/test-windows-wgc-helper.mjs +++ b/scripts/test-windows-wgc-helper.mjs @@ -45,6 +45,20 @@ const WITH_STALLED_READBACK = process.env.OPENSCREEN_WGC_TEST_STALL_READBACK === "true" || process.argv.includes("--stall-readback"); const STALL_READBACK_MS = Number(process.env[STALL_READBACK_ENV] ?? 60_000); +const STALL_FRAME_CALLBACK_ENV = "OPENSCREEN_WGC_TEST_STALL_FRAME_CALLBACK_MS"; +/** + * Reproduces getopenscreen/openscreen#460 on ordinary hardware: stalls the WGC + * frame *callback* itself while it holds the frame lock, the shape that issue + * actually reproduced on Intel HD 520 ("A WGC frame callback did not finish"). + * Distinct from WITH_STALLED_READBACK above -- that stalls the writer's own + * readback, which quiesceCapture()'s drain cannot see (callbacksInFlight_ + * stays at zero), so it cannot exercise the video-writer-join skip this stall + * exists to test. + */ +const WITH_STALLED_FRAME_CALLBACK = + process.env.OPENSCREEN_WGC_TEST_STALL_FRAME_CALLBACK === "true" || + process.argv.includes("--stall-frame-callback"); +const STALL_FRAME_CALLBACK_MS = Number(process.env[STALL_FRAME_CALLBACK_ENV] ?? 60_000); const STOP_BUDGET_ENV = "OPENSCREEN_WGC_STOP_BUDGET_MS"; /** * The helper's global shutdown ceiling, pinned into its environment below so @@ -64,11 +78,15 @@ if (WITH_SOFTWARE_ENCODER && WITH_SOFTWARE_FALLBACK) { throw new Error("--software-encoder and --software-fallback are mutually exclusive"); } -function runHelper(config, { injectDefaultSinkWriterFailure = false, stallReadbackMs = 0 } = {}) { +function runHelper( + config, + { injectDefaultSinkWriterFailure = false, stallReadbackMs = 0, stallFrameCallbackMs = 0 } = {}, +) { return new Promise((resolve, reject) => { const env = { ...process.env }; delete env[INJECT_DEFAULT_SINK_WRITER_FAILURE_ENV]; delete env[STALL_READBACK_ENV]; + delete env[STALL_FRAME_CALLBACK_ENV]; env[STOP_BUDGET_ENV] = String(STOP_BUDGET_MS); if (injectDefaultSinkWriterFailure) { env[INJECT_DEFAULT_SINK_WRITER_FAILURE_ENV] = "1"; @@ -76,6 +94,9 @@ function runHelper(config, { injectDefaultSinkWriterFailure = false, stallReadba if (stallReadbackMs > 0) { env[STALL_READBACK_ENV] = String(stallReadbackMs); } + if (stallFrameCallbackMs > 0) { + env[STALL_FRAME_CALLBACK_ENV] = String(stallFrameCallbackMs); + } const child = spawn(HELPER_PATH, [JSON.stringify(config)], { env, stdio: ["pipe", "pipe", "pipe"], @@ -213,6 +234,48 @@ function startFixtureWindow() { }); } +/** + * Windows Graphics Capture delivers frames on compositor damage, not on a + * fixed clock -- on a genuinely idle desktop the frame pool can go a full + * test run without ever firing FrameArrived once. That is invisible to most + * of this harness, which just needs *a* frame eventually, but the + * stalled-frame-callback regression check needs one to land *inside* the + * DURATION_MS window specifically, so the stall this injects is actually the + * thing holding the frame lock when `stop` arrives. + * + * Moving the cursor alone does not reliably do this: most modern GPU/driver + * combinations composite the cursor on its own hardware overlay plane, so + * repositioning it never touches the desktop bitmap WGC captures (confirmed + * empirically here -- frames=0 with a cursor-only nudge running the whole + * test). A visible window changing position is not optional the way the + * cursor is; DWM has to redraw the area it moved across. Returns a stop + * function; always call it, paired failure or not, or the window and its + * PowerShell host outlive the test process. + */ +function startScreenActivity() { + const child = spawn( + "powershell", + [ + "-NoProfile", + "-Command", + "Add-Type -AssemblyName System.Windows.Forms; " + + "$f = New-Object System.Windows.Forms.Form; " + + "$f.StartPosition = 'Manual'; $f.Location = New-Object System.Drawing.Point(0,0); " + + "$f.Size = New-Object System.Drawing.Size(200,200); " + + "$f.TopMost = $true; $f.Show(); " + + "$x = 0; " + + "while ($true) { " + + "$f.Location = New-Object System.Drawing.Point($x, 0); " + + "$x = ($x + 20) % 200; " + + "[System.Windows.Forms.Application]::DoEvents(); " + + "Start-Sleep -Milliseconds 100; " + + "}", + ], + { stdio: ["ignore", "ignore", "ignore"], windowsHide: false }, + ); + return () => child.kill(); +} + function normalizeDeviceName(value) { return value .toLowerCase() @@ -413,16 +476,19 @@ const config = { }, }; +const stopScreenActivity = WITH_STALLED_FRAME_CALLBACK ? startScreenActivity() : null; let result; try { result = await runHelper(config, { injectDefaultSinkWriterFailure: WITH_SOFTWARE_FALLBACK, stallReadbackMs: WITH_STALLED_READBACK ? STALL_READBACK_MS : 0, + stallFrameCallbackMs: WITH_STALLED_FRAME_CALLBACK ? STALL_FRAME_CALLBACK_MS : 0, }); } finally { if (fixtureWindow) { fixtureWindow.child.kill(); } + stopScreenActivity?.(); } // The regression check for issue #252. With the frame lock deliberately wedged @@ -453,6 +519,51 @@ if (WITH_STALLED_READBACK) { process.exit(0); } +// The regression check for getopenscreen/openscreen#460: a frame callback +// wedged inside the driver, confirmed on real hardware via a Save Diagnostics +// report. Before the fix, video-writer-join burned its whole step budget +// joining a thread parked behind that same stuck callback -- this asserts +// both that the helper still exits promptly (not the ~13s that step's own +// budget alone would cost) and that it took the specific skip path rather +// than any other route to exiting. +if (WITH_STALLED_FRAME_CALLBACK) { + if (result.stopHung) { + throw new Error( + `Helper survived ${STOP_HANG_LIMIT_MS}ms past "stop" with a stalled frame callback. ` + + "Its shutdown watchdog did not fire (issue #460).", + ); + } + const steps = readStopTimingSteps(result.stderr); + if (!steps.includes("command-received")) { + throw new Error(`Helper never acknowledged "stop". Steps seen: ${steps.join(", ") || "none"}`); + } + if (!result.stderr.includes("reason=frame-callback-stuck")) { + throw new Error( + `Helper did not take the video-writer-join skip path. stderr:\n${result.stderr}`, + ); + } + // wgc-quiesce's own drain is a fixed 5000ms, so a healthy skip lands + // there plus the near-instant audio/microphone/webcam steps -- nowhere + // near the ~13s (5s drain + the 8s step budget) the join it replaces + // would have cost before this fix. + const STALLED_FRAME_CALLBACK_LATENCY_BUDGET_MS = 10_000; + if ( + result.stopLatencyMs !== null && + result.stopLatencyMs > STALLED_FRAME_CALLBACK_LATENCY_BUDGET_MS + ) { + throw new Error( + `Stop took ${result.stopLatencyMs}ms with a stalled frame callback, over the ` + + `${STALLED_FRAME_CALLBACK_LATENCY_BUDGET_MS}ms budget the video-writer-join skip should keep it under.`, + ); + } + console.log("WGC helper stalled-frame-callback stop check passed", { + stopLatencyMs: result.stopLatencyMs, + steps, + }); + fs.rmSync(outputPath, { force: true }); + process.exit(0); +} + assertStopWasClean(result); if (result.code !== 0) { @@ -554,6 +665,34 @@ if ( `WGC helper encoder selection was ${JSON.stringify(encoderSelection)}, expected ${expectedEncoderSelection} with preferSoftwareEncoder=${WITH_SOFTWARE_ENCODER}: ${result.stdout}`, ); } +// videoEncoderRuntime is separate from `video` above: it is what +// GetTransformForStream found in the sink writer's own resolved pipeline +// after BeginWriting(), not which configuration path was tried. "unknown" +// here on a run that otherwise passed means the introspection itself is +// broken (wrong COM call, wrong category, wrong attribute), not a real +// ambiguity -- a healthy sink writer always has exactly one encoder node. +if (!["hardware", "software", "unknown"].includes(encoderSelection.videoEncoderRuntime)) { + throw new Error( + `WGC helper reported an unrecognised videoEncoderRuntime: ${JSON.stringify(encoderSelection)}`, + ); +} +if (encoderSelection.videoEncoderRuntime === "unknown") { + throw new Error( + `WGC helper could not introspect its own sink writer for videoEncoderRuntime: ${JSON.stringify(encoderSelection)}`, + ); +} +// forceSoftwareEncoder disables hardware transforms explicitly +// (MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS=FALSE), so this is deterministic +// regardless of what the test machine has registered -- unlike the "default" +// path, whose runtime legitimately depends on the machine. +if ( + (WITH_SOFTWARE_ENCODER || WITH_SOFTWARE_FALLBACK) && + encoderSelection.videoEncoderRuntime !== "software" +) { + throw new Error( + `WGC helper forced the software encoder but videoEncoderRuntime was ${encoderSelection.videoEncoderRuntime}, expected software: ${JSON.stringify(encoderSelection)}`, + ); +} // Every fallback path has to stay fragmented, not just the nominal one. The // helper degrades to the plain container rather than failing a recording, so // without this the fix could quietly stop applying and every other assertion diff --git a/src/App.tsx b/src/App.tsx index 517ad7abd..aa7619d26 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -116,9 +116,9 @@ export default function App() { +
- {tEditor("loadingEditor")} + {tEditor("loadingEditor")}
} > diff --git a/src/components/ai-edition/CaptionsPane.placement.test.tsx b/src/components/ai-edition/CaptionsPane.placement.test.tsx index 27d35a0d9..f1ab095ac 100644 --- a/src/components/ai-edition/CaptionsPane.placement.test.tsx +++ b/src/components/ai-edition/CaptionsPane.placement.test.tsx @@ -1,21 +1,18 @@ // @vitest-environment jsdom -// The placement sliders take their bounds from `captionOffsetRange`, the same -// function the geometry clamps with. That shared range is the fix for the dead -// travel in #396 — the vertical slider used to advertise ±45 while the bottom -// anchor could only honour −45…+3 — so what these tests pin is the agreement -// between what the slider offers and what the band can do, not any one number. +// The placement controls after the anchor redesign. What these pin is the property +// the previous UI could not hold: every control names the edge it measures from, and +// nothing it can produce is a signed number or a dead affordance. +// +// The pane it replaced had four controls that overlapped — a band width nothing drew, +// an offset measured against that invisible band, and a text alignment fighting the +// offset for the same visual outcome — so the tests here are as much about what is +// ABSENT as about what is present. import "@testing-library/jest-dom"; -import { cleanup, render, screen } from "@testing-library/react"; +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { I18nProvider } from "@/contexts/I18nContext"; -import { - captionBandRect, - captionInkHeightPct, - captionOffsetRange, - DEFAULT_CAPTION_SETTINGS, - getCaptionSettings, -} from "@/lib/ai-edition/captions"; +import { getCaptionSettings } from "@/lib/ai-edition/captions"; import type { AxcutAsset, AxcutDocument } from "@/lib/ai-edition/schema"; import { useProjectStore } from "@/lib/ai-edition/store/projectStore"; import { useTranscriptionStore } from "@/lib/ai-edition/store/transcriptionStore"; @@ -81,6 +78,8 @@ function sliderFor(label: string): HTMLInputElement { return input; } +const button = (name: string) => screen.getByRole("button", { name }); + function show(captions: Record) { const document = documentWith(captions); useProjectStore.setState({ @@ -108,45 +107,96 @@ afterEach(() => { }); describe("caption placement controls", () => { - it("offers both axes", () => { + it("offers one anchor and one distance per axis", () => { show({}); - expect(sliderFor("Vertical offset")).toBeInTheDocument(); - expect(sliderFor("Horizontal offset")).toBeInTheDocument(); + expect(button("Bottom")).toHaveAttribute("aria-pressed", "true"); + expect(button("Top")).toHaveAttribute("aria-pressed", "false"); + expect(button("Center")).toHaveAttribute("aria-pressed", "true"); + expect(sliderFor("Distance from bottom")).toBeInTheDocument(); + }); + + it("names the edge the distance is measured from, and follows the anchor", () => { + // The old label said "Vertical offset" and the value could read "-7.3%", which + // corresponds to nothing in any subtitle format and to nothing a user can see. + show({ anchorV: "bottom" }); + expect(screen.getByText("Distance from bottom")).toBeInTheDocument(); + expect(screen.queryByText("Distance from top")).not.toBeInTheDocument(); + + fireEvent.click(button("Top")); + expect(screen.getByText("Distance from top")).toBeInTheDocument(); + expect(screen.queryByText("Distance from bottom")).not.toBeInTheDocument(); }); - it.each([ - "top", - "middle", - "bottom", - ] as const)("bounds the %s anchor's slider by what the band can actually reach", (verticalPosition) => { - const settings = show({ verticalPosition }); - const range = captionOffsetRange(settings); - const slider = sliderFor("Vertical offset"); - expect(Number(slider.min)).toBeCloseTo(range.y.min, 6); - expect(Number(slider.max)).toBeCloseTo(range.y.max, 6); + it("never offers a negative distance", () => { + show({}); + expect(Number(sliderFor("Distance from bottom").min)).toBe(0); + fireEvent.click(button("Left")); + expect(Number(sliderFor("Distance from left").min)).toBe(0); }); - it("puts both ends of the range on a step, so the edges stay reachable", () => { - // A fixed step of 1 would leave `max` off-grid for these fractional bounds and - // the caption would stop just short of the frame edge — the #396 complaint. - const settings = show({ verticalPosition: "bottom" }); - const slider = sliderFor("Vertical offset"); - const [min, max, step] = [slider.min, slider.max, slider.step].map(Number); - const steps = (max - min) / step; - expect(steps).toBeCloseTo(Math.round(steps), 6); - - // And landing on `max` really does put the ink on the frame's bottom edge — - // with the empty part of the band hanging off it, which is what buys the reach. - const band = captionBandRect({ ...settings, offsetY: max }); - expect(band.y + band.height / 2 + captionInkHeightPct(settings) / 2).toBeCloseTo(100, 6); - expect(band.y + band.height).toBeGreaterThan(100); + it("keeps the distance when the anchor flips, mirroring to the opposite edge", () => { + // The inset means the same thing on both anchors, so there is nothing to reset — + // unlike the old presets, which had to zero an offset that meant something else. + show({ anchorV: "bottom", insetY: 12 }); + fireEvent.click(button("Top")); + expect(sliderFor("Distance from top")).toHaveValue("12"); + }); + + it("hides the horizontal distance when centred instead of disabling it", () => { + // A centred block has no edge to measure from. A dead slider reads as a bug, so + // the control is absent rather than greyed out. + show({ anchorH: "center" }); + expect(screen.queryByText("Distance from left")).not.toBeInTheDocument(); + expect(screen.queryByText("Distance from right")).not.toBeInTheDocument(); + + fireEvent.click(button("Right")); + expect(sliderFor("Distance from right")).toBeEnabled(); + }); + + it("leaves no control disabled once a document is open", () => { + show({}); + for (const name of ["Bottom", "Top", "Left", "Center", "Right"]) { + expect(button(name)).toBeEnabled(); + } + expect(sliderFor("Distance from bottom")).toBeEnabled(); }); - it("disables the horizontal slider only when the band fills the frame", () => { - show({ width: 100 }); - expect(sliderFor("Horizontal offset")).toBeDisabled(); - cleanup(); - show({ width: DEFAULT_CAPTION_SETTINGS.width }); - expect(sliderFor("Horizontal offset")).toBeEnabled(); + it("writes the anchor and the inset straight through to the document", () => { + show({}); + fireEvent.click(button("Top")); + fireEvent.change(sliderFor("Distance from top"), { target: { value: "18.5" } }); + + const stored = useProjectStore.getState().document as AxcutDocument; + expect(getCaptionSettings(stored)).toMatchObject({ anchorV: "top", insetY: 18.5 }); + }); + + it("no longer offers the controls the redesign removed", () => { + // Band width drew nothing until the text happened to wrap; the separate text + // alignment fought the horizontal position for the same outcome. + show({}); + expect(screen.queryByText("Width")).not.toBeInTheDocument(); + expect(screen.queryByText("Text align")).not.toBeInTheDocument(); + expect(screen.queryByText("Vertical offset")).not.toBeInTheDocument(); + expect(screen.queryByText("Horizontal offset")).not.toBeInTheDocument(); + expect(screen.queryByRole("button", { name: "Middle" })).not.toBeInTheDocument(); + }); + + it("explains which way a long caption grows", () => { + show({ anchorV: "bottom" }); + expect(screen.getByText(/grow upward/i)).toBeInTheDocument(); + fireEvent.click(button("Top")); + expect(screen.getByText(/grow downward/i)).toBeInTheDocument(); + }); +}); + +describe("migrating a pre-anchor project into the pane", () => { + it("opens an old document on the anchor that reproduces where it was drawn", () => { + // A default bottom caption from the old model: band at 75%, ink centred in it, + // drawn block ending at 92.67% — so a 7.33% inset from the bottom. + show({ verticalPosition: "bottom", offsetY: 0, width: 80, textAlign: "center" }); + expect(button("Bottom")).toHaveAttribute("aria-pressed", "true"); + // The migrated value is the real distance, not a value snapped to the slider's + // step — the step governs dragging, not what a document may already hold. + expect(Number(sliderFor("Distance from bottom").value)).toBeCloseTo(7.333, 2); }); }); diff --git a/src/components/ai-edition/CaptionsPane.tsx b/src/components/ai-edition/CaptionsPane.tsx index 983852f1d..6d6a776c9 100644 --- a/src/components/ai-edition/CaptionsPane.tsx +++ b/src/components/ai-edition/CaptionsPane.tsx @@ -12,8 +12,12 @@ import { Captions as CaptionsIcon, Languages, Loader2, Trash2 } from "lucide-react"; import { useMemo, useState } from "react"; import { useScopedT } from "@/contexts/I18nContext"; -import type { CaptionTextAlign, CaptionVerticalPosition } from "@/lib/ai-edition/captions"; -import { captionOffsetRange, untranslatedUnits } from "@/lib/ai-edition/captions"; +import type { CaptionAnchorH, CaptionAnchorV } from "@/lib/ai-edition/captions"; +import { + CAPTION_INSET_X_MAX, + CAPTION_INSET_Y_MAX, + untranslatedUnits, +} from "@/lib/ai-edition/captions"; import { useProjectStore } from "@/lib/ai-edition/store/projectStore"; import { useTimelineTranscriptGate, @@ -25,16 +29,6 @@ import { ColorField } from "./ColorField"; import styles from "./NewEditorShell.module.css"; import { SliderCell, Toggle } from "./RightPanes"; -/** A hundred stops across whatever span the offset currently has. - * - * The bounds are geometry, so they are rarely round numbers. A fixed `step` of 1 - * would leave `max` unreachable whenever the span isn't a whole number of steps — - * the caption would stop just short of the frame edge, which is the very thing - * #396 is about. Deriving the step from the span puts both ends exactly on a stop. */ -function sliderStep(range: { min: number; max: number }): number { - return Math.max((range.max - range.min) / 100, Number.EPSILON); -} - /** The families `src/index.css` already loads for on-canvas text — anything else * would render in the preview but fall back to a default in the export canvas. */ const CAPTION_FONTS = [ @@ -123,11 +117,6 @@ export function CaptionsPane() { const disabled = !hasDocument; const languageOptions = useMemo(() => Object.values(translations), [translations]); - // The reach depends on the anchor, the width and the font size, so it moves as the - // user works. Taking the sliders' bounds from the same function the geometry clamps - // with is what keeps every position on them a position the band can actually take. - const offsetRange = useMemo(() => captionOffsetRange(settings), [settings]); - const handleTranslate = async () => { const doc = useProjectStore.getState().document; if (!doc) return; @@ -459,67 +448,85 @@ export function CaptionsPane() { ) : null} {/* ── Placement ──────────────────────────────────────────── */} + {/* One control per axis, each naming the edge it measures from. The old pane + had four that overlapped: a band width nothing drew, an offset measured + against that invisible band, and a text alignment fighting the offset for + the same visual outcome. */}
{t("captions.position")}
- - value={settings.verticalPosition} - disabled={disabled} - options={[ - { value: "top", label: t("captions.positionTop") }, - { value: "middle", label: t("captions.positionMiddle") }, - { value: "bottom", label: t("captions.positionBottom") }, - ]} - onChange={(verticalPosition) => void set({ verticalPosition })} - /> - - value={settings.textAlign} + + value={settings.anchorV} disabled={disabled} options={[ - { value: "left", label: t("captions.alignLeft") }, - { value: "center", label: t("captions.alignCenter") }, - { value: "right", label: t("captions.alignRight") }, + { value: "bottom", label: t("captions.anchorBottom") }, + { value: "top", label: t("captions.anchorTop") }, ]} - onChange={(textAlign) => void set({ textAlign })} + // No offset to reset: the inset means the same thing on both anchors, so + // flipping mirrors the caption to the same distance from the opposite edge. + onChange={(anchorV) => void set({ anchorV })} /> +

+ {settings.anchorV === "bottom" + ? t("captions.anchorHintBottom") + : t("captions.anchorHintTop")} +

setLive({ offsetY: v })} - onCommit={() => void commit()} - /> - setLive({ offsetX: v })} - onCommit={() => void commit()} - /> - setLive({ width: v })} + onChange={(v) => setLive({ insetY: v })} onCommit={() => void commit()} />
+ + value={settings.anchorH} + disabled={disabled} + options={[ + { value: "left", label: t("captions.alignLeft") }, + { value: "center", label: t("captions.alignCenter") }, + { value: "right", label: t("captions.alignRight") }, + ]} + onChange={(anchorH) => void set({ anchorH })} + /> + {/* Centre has no edge to measure from, so the control is ABSENT rather than + disabled — a dead slider reads as a bug. */} + {settings.anchorH === "center" ? null : ( +
+ setLive({ insetX: v })} + onCommit={() => void commit()} + /> +
+ )} + {/* ── Line length ────────────────────────────────────────── */}
{t("captions.lineLength")}
diff --git a/src/components/ai-edition/LeftPanel.tsx b/src/components/ai-edition/LeftPanel.tsx index 06a349ef1..2cafaebf2 100644 --- a/src/components/ai-edition/LeftPanel.tsx +++ b/src/components/ai-edition/LeftPanel.tsx @@ -1952,7 +1952,7 @@ function ChatStripPanel() { background: "var(--accent)", border: "1px solid var(--accent)", borderRadius: "var(--r-sm)", - color: "var(--bg)", + color: "var(--accent-on)", font: "500 12px var(--font-body)", cursor: "pointer", }} diff --git a/src/components/ai-edition/Modals.tsx b/src/components/ai-edition/Modals.tsx index 9ac9d1079..3be5687fd 100644 --- a/src/components/ai-edition/Modals.tsx +++ b/src/components/ai-edition/Modals.tsx @@ -17,51 +17,28 @@ import { type ReactNode, type PointerEvent as ReactPointerEvent, useEffect, + useMemo, useRef, useState, } from "react"; import { toFileUrl } from "@/components/video-editor/projectPersistence"; import type { CropRegion } from "@/components/video-editor/types"; -import { useScopedT } from "@/contexts/I18nContext"; +import { useI18n, useScopedT } from "@/contexts/I18nContext"; import { toAxcutTranscriptDsl } from "@/lib/ai-edition/document/transcribe"; -import type { AxcutClip, AxcutTranscript } from "@/lib/ai-edition/schema"; +import { + type AxcutClip, + type AxcutTranscript, + type TranscriptLanguageCode, + transcriptLanguageSchema, +} from "@/lib/ai-edition/schema"; import { formatSec, formatSeconds } from "@/lib/ai-edition/timeline/format"; +import { + languageLabel, + sortedLanguageOptions, +} from "@/lib/ai-edition/transcription/languageLabels"; import styles from "./NewEditorShell.module.css"; import type { VideoSource } from "./VirtualPreview"; -// ponytail: keep the UI's language list literal in one place. Mirrors -// `transcriptLanguageSchema` in schema/index.ts; if the schema gains a -// language, add it here too. -const REGEN_LANGUAGES = [ - "auto", - "en", - "fr", - "de", - "es", - "it", - "pt", - "nl", - "ja", - "ko", - "zh", -] as const; - -type TranscriptLanguage = (typeof REGEN_LANGUAGES)[number]; - -const LANGUAGE_LABELS: Record = { - auto: "Auto", - en: "EN", - fr: "FR", - de: "DE", - es: "ES", - it: "IT", - pt: "PT", - nl: "NL", - ja: "JA", - ko: "KO", - zh: "ZH", -}; - interface BaseModalProps { open: boolean; onClose: () => void; @@ -1539,6 +1516,17 @@ export function InsertSourceModal({ ); } +/** + * `AxcutTranscript.language` is `z.string().min(1)`, not validated against + * the known code list, so a stored transcript can hold a value no + * `
@@ -1900,7 +1896,7 @@ export function SourceTranscriptModal({ aria-label={t("mediaStage.regenerateAs")} value={regenLang} disabled={isTranscribing} - onChange={(e) => setRegenLang(e.target.value as TranscriptLanguage)} + onChange={(e) => setRegenLang(e.target.value as TranscriptLanguageCode)} style={{ width: "100%", padding: "10px 12px", @@ -1911,9 +1907,9 @@ export function SourceTranscriptModal({ font: "500 13px var(--font-body)", }} > - {REGEN_LANGUAGES.map((code) => ( + {regenLanguageOptions.map(({ code, label }) => ( ))} diff --git a/src/components/ai-edition/NewEditorShell.module.css b/src/components/ai-edition/NewEditorShell.module.css index 155b665eb..c842a535a 100644 --- a/src/components/ai-edition/NewEditorShell.module.css +++ b/src/components/ai-edition/NewEditorShell.module.css @@ -594,7 +594,7 @@ padding: var(--sp-3) var(--sp-4); border-radius: var(--r-sm); background: var(--primary, #34B27B); - color: #fff; + color: var(--accent-on); border: none; font-weight: 500; font-size: 0.875rem; diff --git a/src/components/ai-edition/RightPanes.tsx b/src/components/ai-edition/RightPanes.tsx index 5f76e355d..d12fe248a 100644 --- a/src/components/ai-edition/RightPanes.tsx +++ b/src/components/ai-edition/RightPanes.tsx @@ -143,22 +143,25 @@ function Pane({ title, icon, helpText, children }: PaneProps) { // keep the gradient palette small and curated — every block renders // in the picker and gets serialized to legacyEditor on save. +// Spans the same hues as COLOR_PALETTE below rather than leaning on the +// brand mint for half the grid — a wall of green reads as "we only +// have one color" rather than "pick a gradient." const GRAD_PRESETS: readonly string[] = [ "linear-gradient(135deg, #eaebed, #bcc0c6)", - "linear-gradient(135deg, #10b981, #eaebed)", - "linear-gradient(135deg, #6b7280, #bcc0c6)", - "linear-gradient(135deg, #eaebed, #10b981)", - "linear-gradient(135deg, #16171d, #6b7280)", - "linear-gradient(135deg, #bcc0c6, #16171d)", - "linear-gradient(135deg, #10b981, #6b7280)", - "linear-gradient(135deg, #eaebed, #10b981)", + "linear-gradient(135deg, #3b82f6, #8b5cf6)", + "linear-gradient(135deg, #8b5cf6, #ec4899)", + "linear-gradient(135deg, #f97316, #ec4899)", + "linear-gradient(135deg, #f59e0b, #f97316)", + "linear-gradient(135deg, #10b981, #3b82f6)", + "linear-gradient(135deg, #22c55e, #10b981)", "linear-gradient(135deg, #6b7280, #16171d)", - "linear-gradient(135deg, #bcc0c6, #10b981)", - "linear-gradient(135deg, #16171d, #6b7280)", - "linear-gradient(135deg, #eaebed, #bcc0c6)", - "linear-gradient(135deg, #10b981, #bcc0c6)", - "linear-gradient(135deg, #eaebed, #16171d)", - "linear-gradient(135deg, #6b7280, #10b981)", + "linear-gradient(135deg, #ec4899, #ef4444)", + "linear-gradient(135deg, #3b82f6, #22c55e)", + "linear-gradient(135deg, #8b5cf6, #3b82f6)", + "linear-gradient(135deg, #f59e0b, #ef4444)", + "linear-gradient(135deg, #16171d, #1e293b)", + "linear-gradient(135deg, #34d399, #3b82f6)", + "linear-gradient(135deg, #ef4444, #8b5cf6)", "linear-gradient(135deg, #bcc0c6, #eaebed)", ]; diff --git a/src/components/ai-edition/VirtualPreview.module.css b/src/components/ai-edition/VirtualPreview.module.css index 78d84a06f..6177a9f52 100644 --- a/src/components/ai-edition/VirtualPreview.module.css +++ b/src/components/ai-edition/VirtualPreview.module.css @@ -60,6 +60,7 @@ .iconButton:hover:not(:disabled) { background: var(--accent); + color: var(--accent-on); } .iconButton:disabled { diff --git a/src/components/ai-edition/v4/EditorShellV4.module.css b/src/components/ai-edition/v4/EditorShellV4.module.css index 342cedd4c..1e313e34c 100644 --- a/src/components/ai-edition/v4/EditorShellV4.module.css +++ b/src/components/ai-edition/v4/EditorShellV4.module.css @@ -389,7 +389,7 @@ padding: 0 14px; border-radius: 9px; background: var(--accent); - color: #fff; + color: var(--accent-on); border: 1px solid var(--accent); font-size: 13px; font-weight: 600; @@ -1126,7 +1126,7 @@ padding: 0 26px; border-radius: 16px; border: 1px solid var(--accent); - color: #fff; + color: var(--accent-on); background: var(--accent); font-size: 14px; font-weight: 600; @@ -1140,6 +1140,7 @@ .bigRecBtn.recording { border-color: var(--danger); background: var(--danger); + color: #fff; } .bigRecDot { width: 10px; @@ -1625,7 +1626,12 @@ display: grid; place-items: center; border-radius: 7px; - color: var(--muted); + /* The chip itself is a fixed dark frosted-glass overlay regardless of + theme (it sits on top of an arbitrary video thumbnail), so its icon + needs the "light text on a dark overlay" token, not --muted — which + is tuned for the app's own light/dark surfaces and reads as + near-invisible against this chip in light theme. */ + color: var(--overlay-text); background: color-mix(in srgb, #080a0d 55%, transparent); border: 1px solid rgba(255, 255, 255, 0.08); backdrop-filter: blur(8px); diff --git a/src/components/ai-edition/v4/EditorTopBar.tsx b/src/components/ai-edition/v4/EditorTopBar.tsx index a369da6a8..884a48da7 100644 --- a/src/components/ai-edition/v4/EditorTopBar.tsx +++ b/src/components/ai-edition/v4/EditorTopBar.tsx @@ -10,7 +10,6 @@ import { PanelLeft, RefreshCw, Save, - Settings, Sparkles, Sun, } from "lucide-react"; @@ -182,15 +181,6 @@ export function EditorTopBar({ > {theme === "dark" ? : } - ))} -
+
@@ -372,7 +383,7 @@ export function MediaStage({
{hasConflict && conflict?.conflictWith.type === "configurable" && ( -
- +
+ ⚠{" "} {t("alreadyUsedBy", { action: t(`actions.${conflict.conflictWith.action}`), @@ -189,14 +189,14 @@ export function ShortcutsConfigDialog() { @@ -209,32 +209,32 @@ export function ShortcutsConfigDialog() {
-

+

{t("fixed")}

{FIXED_SHORTCUTS.map(({ i18nKey, label, display }) => (
- + {t(`fixedActions.${i18nKey}`, { defaultValue: label })} - + {display}
))}
-

{t("helpText")}

+

{t("helpText")}