Do not let a departed session end the one that replaced it
Rejoining a call connected, published, and was torn down a second later. Teardown is spawned rather than waited for — that was the fix for the freeze on Leave — so the task of a call that has ended now outlives the call. It finishes, reports that it ended, and that report lands in the same global queue the *new* call is reading. The caller sees "the call ended" and tears down a call that had just come up. So statuses carry the generation they were produced under, and anything a replaced session still has to say is dropped rather than acted on. Bumped on every start and every stop; skipped rather than returned on, since stopping at a stale entry would strand the fresh ones queued behind it. Two tests, and a lock the tests take: there is one session in this library so there is one in its tests, and cargo runs them in parallel — a test asserting nothing is live was failing because another was mid-call. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
b1758f5 parent: e86c31c modified
crates/jolt-moq/src/lib.rs +96 -6 | @@ -61,8 +61,20 @@ static DIAL: Scratch = Scratch::new(); | ||
| 61 | 61 | #[derive(Default)] |
| 62 | 62 | struct Slot { |
| 63 | 63 | session: Option<AvMediaSession>, |
| 64 | - /// Status updates from the media task, waiting to be polled. | |
| 65 | - status: Vec<Status>, | |
| 64 | + /// Which session the statuses below belong to. | |
| 65 | + /// | |
| 66 | + /// Tearing a call down is spawned rather than waited for, so the task of a | |
| 67 | + /// call that has ended outlives the call — and finishes, and reports that | |
| 68 | + /// it ended, possibly after the *next* call has started. Without a way to | |
| 69 | + /// tell whose news this is, the old session's dying breath tears down the | |
| 70 | + /// new one, which is exactly what rejoining a call did. | |
| 71 | + /// | |
| 72 | + /// Bumped on every start and every stop, so anything a departed session | |
| 73 | + /// still has to say is discarded rather than acted on. | |
| 74 | + generation: u64, | |
| 75 | + /// Status updates from the media task, waiting to be polled, each with the | |
| 76 | + /// generation it was produced under. | |
| 77 | + status: Vec<(u64, Status)>, | |
| 66 | 78 | /// The status most recently handed out, whose fields the accessors read. |
| 67 | 79 | current: Option<Status>, |
| 68 | 80 | /// The live session's frame store, and the generation last handed out per |
| @@ -204,8 +216,11 @@ pub unsafe extern "C" fn joltmoq_start( | ||
| 204 | 216 | // `AvMediaSession::start` spawns onto the ambient runtime, so it has to |
| 205 | 217 | // be entered. The status closure it takes runs on a worker thread, |
| 206 | 218 | // which is why all it does is push onto a queue the caller drains. |
| 219 | + slot.generation = slot.generation.wrapping_add(1); | |
| 220 | + let generation = slot.generation; | |
| 221 | + | |
| 207 | 222 | let _entered = runtime.enter(); |
| 208 | - let session = AvMediaSession::start(config, |update| { | |
| 223 | + let session = AvMediaSession::start(config, move |update| { | |
| 209 | 224 | let status = match update { |
| 210 | 225 | AvMediaUpdate::Live { |
| 211 | 226 | has_camera, |
| @@ -219,7 +234,11 @@ pub unsafe extern "C" fn joltmoq_start( | ||
| 219 | 234 | AvMediaUpdate::Failed(e) => Status::Failed(e), |
| 220 | 235 | }; |
| 221 | 236 | if let Ok(mut slot) = session_slot().lock() { |
| 222 | - slot.status.push(status); | |
| 237 | + // Dropped on the floor if this session is no longer the | |
| 238 | + // current one — see `Slot::generation`. | |
| 239 | + if slot.generation == generation { | |
| 240 | + slot.status.push((generation, status)); | |
| 241 | + } | |
| 223 | 242 | } |
| 224 | 243 | }); |
| 225 | 244 | |
| @@ -250,6 +269,12 @@ pub extern "C" fn joltmoq_stop() { | ||
| 250 | 269 | slot.video = None; |
| 251 | 270 | slot.seen.clear(); |
| 252 | 271 | slot.frame = None; |
| 272 | + // Nothing this session says from here on is about the call the caller | |
| 273 | + // is in, because it is not in one — and may be in a different one by | |
| 274 | + // the time the task gets round to saying it. | |
| 275 | + slot.generation = slot.generation.wrapping_add(1); | |
| 276 | + slot.status.clear(); | |
| 277 | + slot.current = None; | |
| 253 | 278 | slot.session.take() |
| 254 | 279 | }); |
| 255 | 280 | let Some(mut session) = session else { |
| @@ -387,11 +412,16 @@ pub const JOLTMOQ_STATUS_FAILED: c_int = 3; | ||
| 387 | 412 | #[no_mangle] |
| 388 | 413 | pub extern "C" fn joltmoq_poll_status() -> c_int { |
| 389 | 414 | with_slot(JOLTMOQ_STATUS_NONE, |slot| { |
| 415 | + // Drop anything a replaced session left behind, rather than stopping | |
| 416 | + // at it: answering NONE on a stale entry would strand every fresh one | |
| 417 | + // queued behind it. | |
| 418 | + let current = slot.generation; | |
| 419 | + slot.status.retain(|(generation, _)| *generation == current); | |
| 390 | 420 | if slot.status.is_empty() { |
| 391 | 421 | slot.current = None; |
| 392 | 422 | return JOLTMOQ_STATUS_NONE; |
| 393 | 423 | } |
| 394 | - let status = slot.status.remove(0); | |
| 424 | + let (_, status) = slot.status.remove(0); | |
| 395 | 425 | let code = match &status { |
| 396 | 426 | Status::Live { .. } => JOLTMOQ_STATUS_LIVE, |
| 397 | 427 | Status::Ended => JOLTMOQ_STATUS_ENDED, |
| @@ -696,6 +726,18 @@ mod tests { | ||
| 696 | 726 | unsafe { CStr::from_ptr(ptr) }.to_string_lossy().into_owned() |
| 697 | 727 | } |
| 698 | 728 | |
| 729 | + /// There is one session in this library, so there is one in its tests, and | |
| 730 | + /// cargo runs them in parallel. Anything touching the global slot takes | |
| 731 | + /// this first; without it a test asserting "nothing is live" fails because | |
| 732 | + /// another was mid-call at the time. | |
| 733 | + static ONE_AT_A_TIME: Mutex<()> = Mutex::new(()); | |
| 734 | + | |
| 735 | + fn exclusive() -> std::sync::MutexGuard<'static, ()> { | |
| 736 | + // A test that panicked while holding it poisoned it; that is the | |
| 737 | + // failure being reported, not a reason to fail every test after it. | |
| 738 | + ONE_AT_A_TIME.lock().unwrap_or_else(|e| e.into_inner()) | |
| 739 | + } | |
| 740 | + | |
| 699 | 741 | #[test] |
| 700 | 742 | fn a_device_list_is_tab_and_newline_delimited() { |
| 701 | 743 | let list = vec![ |
| @@ -777,6 +819,7 @@ mod tests { | ||
| 777 | 819 | |
| 778 | 820 | #[test] |
| 779 | 821 | fn nothing_is_live_before_a_call_and_every_poll_answers_empty() { |
| 822 | + let _guard = exclusive(); | |
| 780 | 823 | assert_eq!(joltmoq_is_live(), 0); |
| 781 | 824 | assert_eq!(joltmoq_poll_status(), JOLTMOQ_STATUS_NONE); |
| 782 | 825 | assert_eq!(read(joltmoq_status_text()), ""); |
| @@ -789,6 +832,7 @@ mod tests { | ||
| 789 | 832 | |
| 790 | 833 | #[test] |
| 791 | 834 | fn a_control_with_no_call_under_it_is_inert() { |
| 835 | + let _guard = exclusive(); | |
| 792 | 836 | // A UI that sends a mute as the call is ending must not take the |
| 793 | 837 | // process with it. |
| 794 | 838 | joltmoq_set_muted(1); |
| @@ -802,6 +846,7 @@ mod tests { | ||
| 802 | 846 | |
| 803 | 847 | #[test] |
| 804 | 848 | fn stopping_does_not_hold_the_lock_a_status_callback_needs() { |
| 849 | + let _guard = exclusive(); | |
| 805 | 850 | // The deadlock this guards: `stop` used to wait for the media task |
| 806 | 851 | // while holding the slot, and the task's status callback locks the |
| 807 | 852 | // slot to deliver an update — so neither could finish. Here a thread |
| @@ -814,7 +859,8 @@ mod tests { | ||
| 814 | 859 | std::thread::spawn(move || { |
| 815 | 860 | for _ in 0..200 { |
| 816 | 861 | if let Ok(mut slot) = session_slot().lock() { |
| 817 | - slot.status.push(Status::Ended); | |
| 862 | + let generation = slot.generation; | |
| 863 | + slot.status.push((generation, Status::Ended)); | |
| 818 | 864 | slot.status.clear(); |
| 819 | 865 | } |
| 820 | 866 | std::thread::sleep(Duration::from_millis(1)); |
| @@ -830,8 +876,52 @@ mod tests { | ||
| 830 | 876 | .expect("the status thread never got the lock back"); |
| 831 | 877 | } |
| 832 | 878 | |
| 879 | + #[test] | |
| 880 | + fn a_departed_session_cannot_end_the_one_that_replaced_it() { | |
| 881 | + let _guard = exclusive(); | |
| 882 | + // Rejoining a call did exactly this. Teardown is spawned, so the old | |
| 883 | + // task finishes after the new call has started, and its "ended" landed | |
| 884 | + // in the same queue — where it read as the new call ending. | |
| 885 | + if let Ok(mut slot) = session_slot().lock() { | |
| 886 | + slot.generation = 7; | |
| 887 | + slot.status.clear(); | |
| 888 | + // The old session (6) signing off, and the live one (7) saying it | |
| 889 | + // is up. Queued in that order, which is the order that hurt. | |
| 890 | + slot.status.push((6, Status::Ended)); | |
| 891 | + slot.status.push(( | |
| 892 | + 7, | |
| 893 | + Status::Live { | |
| 894 | + has_camera: false, | |
| 895 | + has_mic: true, | |
| 896 | + }, | |
| 897 | + )); | |
| 898 | + } | |
| 899 | + | |
| 900 | + // The stale "ended" is skipped, not acted on, and not left blocking | |
| 901 | + // the fresh one behind it. | |
| 902 | + assert_eq!(joltmoq_poll_status(), JOLTMOQ_STATUS_LIVE); | |
| 903 | + assert_eq!(joltmoq_status_has_mic(), 1); | |
| 904 | + assert_eq!(joltmoq_poll_status(), JOLTMOQ_STATUS_NONE); | |
| 905 | + | |
| 906 | + if let Ok(mut slot) = session_slot().lock() { | |
| 907 | + slot.generation = 0; | |
| 908 | + slot.status.clear(); | |
| 909 | + slot.current = None; | |
| 910 | + } | |
| 911 | + } | |
| 912 | + | |
| 913 | + #[test] | |
| 914 | + fn stopping_makes_the_next_call_a_new_generation() { | |
| 915 | + let _guard = exclusive(); | |
| 916 | + let before = session_slot().lock().map(|s| s.generation).unwrap_or(0); | |
| 917 | + joltmoq_stop(); | |
| 918 | + let after = session_slot().lock().map(|s| s.generation).unwrap_or(0); | |
| 919 | + assert_ne!(before, after, "a stopped session must stop being current"); | |
| 920 | + } | |
| 921 | + | |
| 833 | 922 | #[test] |
| 834 | 923 | fn a_frame_poll_yields_each_feed_once_until_it_changes() { |
| 924 | + let _guard = exclusive(); | |
| 835 | 925 | let video = VideoFrameStore::new(); |
| 836 | 926 | video.set("nandi", 1, 1, std::sync::Arc::from(vec![9u8; 4])); |
| 837 | 927 | if let Ok(mut slot) = session_slot().lock() { |
| @@ -61,8 +61,20 @@ static DIAL: Scratch = Scratch::new(); | |||
| 61 | #[derive(Default)] | 61 | #[derive(Default)] |
| 62 | struct Slot { | 62 | struct Slot { |
| 63 | session: Option<AvMediaSession>, | 63 | session: Option<AvMediaSession>, |
| 64 | - /// Status updates from the media task, waiting to be polled. | 64 | + /// Which session the statuses below belong to. |
| 65 | - status: Vec<Status>, | 65 | + /// |
| 66 | + /// Tearing a call down is spawned rather than waited for, so the task of a | ||
| 67 | + /// call that has ended outlives the call — and finishes, and reports that | ||
| 68 | + /// it ended, possibly after the *next* call has started. Without a way to | ||
| 69 | + /// tell whose news this is, the old session's dying breath tears down the | ||
| 70 | + /// new one, which is exactly what rejoining a call did. | ||
| 71 | + /// | ||
| 72 | + /// Bumped on every start and every stop, so anything a departed session | ||
| 73 | + /// still has to say is discarded rather than acted on. | ||
| 74 | + generation: u64, | ||
| 75 | + /// Status updates from the media task, waiting to be polled, each with the | ||
| 76 | + /// generation it was produced under. | ||
| 77 | + status: Vec<(u64, Status)>, | ||
| 66 | /// The status most recently handed out, whose fields the accessors read. | 78 | /// The status most recently handed out, whose fields the accessors read. |
| 67 | current: Option<Status>, | 79 | current: Option<Status>, |
| 68 | /// The live session's frame store, and the generation last handed out per | 80 | /// The live session's frame store, and the generation last handed out per |
| @@ -204,8 +216,11 @@ pub unsafe extern "C" fn joltmoq_start( | |||
| 204 | // `AvMediaSession::start` spawns onto the ambient runtime, so it has to | 216 | // `AvMediaSession::start` spawns onto the ambient runtime, so it has to |
| 205 | // be entered. The status closure it takes runs on a worker thread, | 217 | // be entered. The status closure it takes runs on a worker thread, |
| 206 | // which is why all it does is push onto a queue the caller drains. | 218 | // which is why all it does is push onto a queue the caller drains. |
| 219 | + slot.generation = slot.generation.wrapping_add(1); | ||
| 220 | + let generation = slot.generation; | ||
| 221 | + | ||
| 207 | let _entered = runtime.enter(); | 222 | let _entered = runtime.enter(); |
| 208 | - let session = AvMediaSession::start(config, |update| { | 223 | + let session = AvMediaSession::start(config, move |update| { |
| 209 | let status = match update { | 224 | let status = match update { |
| 210 | AvMediaUpdate::Live { | 225 | AvMediaUpdate::Live { |
| 211 | has_camera, | 226 | has_camera, |
| @@ -219,7 +234,11 @@ pub unsafe extern "C" fn joltmoq_start( | |||
| 219 | AvMediaUpdate::Failed(e) => Status::Failed(e), | 234 | AvMediaUpdate::Failed(e) => Status::Failed(e), |
| 220 | }; | 235 | }; |
| 221 | if let Ok(mut slot) = session_slot().lock() { | 236 | if let Ok(mut slot) = session_slot().lock() { |
| 222 | - slot.status.push(status); | 237 | + // Dropped on the floor if this session is no longer the |
| 238 | + // current one — see `Slot::generation`. | ||
| 239 | + if slot.generation == generation { | ||
| 240 | + slot.status.push((generation, status)); | ||
| 241 | + } | ||
| 223 | } | 242 | } |
| 224 | }); | 243 | }); |
| 225 | 244 | ||
| @@ -250,6 +269,12 @@ pub extern "C" fn joltmoq_stop() { | |||
| 250 | slot.video = None; | 269 | slot.video = None; |
| 251 | slot.seen.clear(); | 270 | slot.seen.clear(); |
| 252 | slot.frame = None; | 271 | slot.frame = None; |
| 272 | + // Nothing this session says from here on is about the call the caller | ||
| 273 | + // is in, because it is not in one — and may be in a different one by | ||
| 274 | + // the time the task gets round to saying it. | ||
| 275 | + slot.generation = slot.generation.wrapping_add(1); | ||
| 276 | + slot.status.clear(); | ||
| 277 | + slot.current = None; | ||
| 253 | slot.session.take() | 278 | slot.session.take() |
| 254 | }); | 279 | }); |
| 255 | let Some(mut session) = session else { | 280 | let Some(mut session) = session else { |
| @@ -387,11 +412,16 @@ pub const JOLTMOQ_STATUS_FAILED: c_int = 3; | |||
| 387 | #[no_mangle] | 412 | #[no_mangle] |
| 388 | pub extern "C" fn joltmoq_poll_status() -> c_int { | 413 | pub extern "C" fn joltmoq_poll_status() -> c_int { |
| 389 | with_slot(JOLTMOQ_STATUS_NONE, |slot| { | 414 | with_slot(JOLTMOQ_STATUS_NONE, |slot| { |
| 415 | + // Drop anything a replaced session left behind, rather than stopping | ||
| 416 | + // at it: answering NONE on a stale entry would strand every fresh one | ||
| 417 | + // queued behind it. | ||
| 418 | + let current = slot.generation; | ||
| 419 | + slot.status.retain(|(generation, _)| *generation == current); | ||
| 390 | if slot.status.is_empty() { | 420 | if slot.status.is_empty() { |
| 391 | slot.current = None; | 421 | slot.current = None; |
| 392 | return JOLTMOQ_STATUS_NONE; | 422 | return JOLTMOQ_STATUS_NONE; |
| 393 | } | 423 | } |
| 394 | - let status = slot.status.remove(0); | 424 | + let (_, status) = slot.status.remove(0); |
| 395 | let code = match &status { | 425 | let code = match &status { |
| 396 | Status::Live { .. } => JOLTMOQ_STATUS_LIVE, | 426 | Status::Live { .. } => JOLTMOQ_STATUS_LIVE, |
| 397 | Status::Ended => JOLTMOQ_STATUS_ENDED, | 427 | Status::Ended => JOLTMOQ_STATUS_ENDED, |
| @@ -696,6 +726,18 @@ mod tests { | |||
| 696 | unsafe { CStr::from_ptr(ptr) }.to_string_lossy().into_owned() | 726 | unsafe { CStr::from_ptr(ptr) }.to_string_lossy().into_owned() |
| 697 | } | 727 | } |
| 698 | 728 | ||
| 729 | + /// There is one session in this library, so there is one in its tests, and | ||
| 730 | + /// cargo runs them in parallel. Anything touching the global slot takes | ||
| 731 | + /// this first; without it a test asserting "nothing is live" fails because | ||
| 732 | + /// another was mid-call at the time. | ||
| 733 | + static ONE_AT_A_TIME: Mutex<()> = Mutex::new(()); | ||
| 734 | + | ||
| 735 | + fn exclusive() -> std::sync::MutexGuard<'static, ()> { | ||
| 736 | + // A test that panicked while holding it poisoned it; that is the | ||
| 737 | + // failure being reported, not a reason to fail every test after it. | ||
| 738 | + ONE_AT_A_TIME.lock().unwrap_or_else(|e| e.into_inner()) | ||
| 739 | + } | ||
| 740 | + | ||
| 699 | #[test] | 741 | #[test] |
| 700 | fn a_device_list_is_tab_and_newline_delimited() { | 742 | fn a_device_list_is_tab_and_newline_delimited() { |
| 701 | let list = vec![ | 743 | let list = vec![ |
| @@ -777,6 +819,7 @@ mod tests { | |||
| 777 | 819 | ||
| 778 | #[test] | 820 | #[test] |
| 779 | fn nothing_is_live_before_a_call_and_every_poll_answers_empty() { | 821 | fn nothing_is_live_before_a_call_and_every_poll_answers_empty() { |
| 822 | + let _guard = exclusive(); | ||
| 780 | assert_eq!(joltmoq_is_live(), 0); | 823 | assert_eq!(joltmoq_is_live(), 0); |
| 781 | assert_eq!(joltmoq_poll_status(), JOLTMOQ_STATUS_NONE); | 824 | assert_eq!(joltmoq_poll_status(), JOLTMOQ_STATUS_NONE); |
| 782 | assert_eq!(read(joltmoq_status_text()), ""); | 825 | assert_eq!(read(joltmoq_status_text()), ""); |
| @@ -789,6 +832,7 @@ mod tests { | |||
| 789 | 832 | ||
| 790 | #[test] | 833 | #[test] |
| 791 | fn a_control_with_no_call_under_it_is_inert() { | 834 | fn a_control_with_no_call_under_it_is_inert() { |
| 835 | + let _guard = exclusive(); | ||
| 792 | // A UI that sends a mute as the call is ending must not take the | 836 | // A UI that sends a mute as the call is ending must not take the |
| 793 | // process with it. | 837 | // process with it. |
| 794 | joltmoq_set_muted(1); | 838 | joltmoq_set_muted(1); |
| @@ -802,6 +846,7 @@ mod tests { | |||
| 802 | 846 | ||
| 803 | #[test] | 847 | #[test] |
| 804 | fn stopping_does_not_hold_the_lock_a_status_callback_needs() { | 848 | fn stopping_does_not_hold_the_lock_a_status_callback_needs() { |
| 849 | + let _guard = exclusive(); | ||
| 805 | // The deadlock this guards: `stop` used to wait for the media task | 850 | // The deadlock this guards: `stop` used to wait for the media task |
| 806 | // while holding the slot, and the task's status callback locks the | 851 | // while holding the slot, and the task's status callback locks the |
| 807 | // slot to deliver an update — so neither could finish. Here a thread | 852 | // slot to deliver an update — so neither could finish. Here a thread |
| @@ -814,7 +859,8 @@ mod tests { | |||
| 814 | std::thread::spawn(move || { | 859 | std::thread::spawn(move || { |
| 815 | for _ in 0..200 { | 860 | for _ in 0..200 { |
| 816 | if let Ok(mut slot) = session_slot().lock() { | 861 | if let Ok(mut slot) = session_slot().lock() { |
| 817 | - slot.status.push(Status::Ended); | 862 | + let generation = slot.generation; |
| 863 | + slot.status.push((generation, Status::Ended)); | ||
| 818 | slot.status.clear(); | 864 | slot.status.clear(); |
| 819 | } | 865 | } |
| 820 | std::thread::sleep(Duration::from_millis(1)); | 866 | std::thread::sleep(Duration::from_millis(1)); |
| @@ -830,8 +876,52 @@ mod tests { | |||
| 830 | .expect("the status thread never got the lock back"); | 876 | .expect("the status thread never got the lock back"); |
| 831 | } | 877 | } |
| 832 | 878 | ||
| 879 | + #[test] | ||
| 880 | + fn a_departed_session_cannot_end_the_one_that_replaced_it() { | ||
| 881 | + let _guard = exclusive(); | ||
| 882 | + // Rejoining a call did exactly this. Teardown is spawned, so the old | ||
| 883 | + // task finishes after the new call has started, and its "ended" landed | ||
| 884 | + // in the same queue — where it read as the new call ending. | ||
| 885 | + if let Ok(mut slot) = session_slot().lock() { | ||
| 886 | + slot.generation = 7; | ||
| 887 | + slot.status.clear(); | ||
| 888 | + // The old session (6) signing off, and the live one (7) saying it | ||
| 889 | + // is up. Queued in that order, which is the order that hurt. | ||
| 890 | + slot.status.push((6, Status::Ended)); | ||
| 891 | + slot.status.push(( | ||
| 892 | + 7, | ||
| 893 | + Status::Live { | ||
| 894 | + has_camera: false, | ||
| 895 | + has_mic: true, | ||
| 896 | + }, | ||
| 897 | + )); | ||
| 898 | + } | ||
| 899 | + | ||
| 900 | + // The stale "ended" is skipped, not acted on, and not left blocking | ||
| 901 | + // the fresh one behind it. | ||
| 902 | + assert_eq!(joltmoq_poll_status(), JOLTMOQ_STATUS_LIVE); | ||
| 903 | + assert_eq!(joltmoq_status_has_mic(), 1); | ||
| 904 | + assert_eq!(joltmoq_poll_status(), JOLTMOQ_STATUS_NONE); | ||
| 905 | + | ||
| 906 | + if let Ok(mut slot) = session_slot().lock() { | ||
| 907 | + slot.generation = 0; | ||
| 908 | + slot.status.clear(); | ||
| 909 | + slot.current = None; | ||
| 910 | + } | ||
| 911 | + } | ||
| 912 | + | ||
| 913 | + #[test] | ||
| 914 | + fn stopping_makes_the_next_call_a_new_generation() { | ||
| 915 | + let _guard = exclusive(); | ||
| 916 | + let before = session_slot().lock().map(|s| s.generation).unwrap_or(0); | ||
| 917 | + joltmoq_stop(); | ||
| 918 | + let after = session_slot().lock().map(|s| s.generation).unwrap_or(0); | ||
| 919 | + assert_ne!(before, after, "a stopped session must stop being current"); | ||
| 920 | + } | ||
| 921 | + | ||
| 833 | #[test] | 922 | #[test] |
| 834 | fn a_frame_poll_yields_each_feed_once_until_it_changes() { | 923 | fn a_frame_poll_yields_each_feed_once_until_it_changes() { |
| 924 | + let _guard = exclusive(); | ||
| 835 | let video = VideoFrameStore::new(); | 925 | let video = VideoFrameStore::new(); |
| 836 | video.set("nandi", 1, 1, std::sync::Arc::from(vec![9u8; 4])); | 926 | video.set("nandi", 1, 1, std::sync::Arc::from(vec![9u8; 4])); |
| 837 | if let Ok(mut slot) = session_slot().lock() { | 927 | if let Ok(mut slot) = session_slot().lock() { |
modified
crates/jolt-vidya/src/app.rs +47 -0 | @@ -289,6 +289,14 @@ struct Handler { | ||
| 289 | 289 | /// for it before the first present deadlocks. |
| 290 | 290 | may_present: bool, |
| 291 | 291 | frames: u32, |
| 292 | + /// The size, in pixels, the frame now being built laid itself out against. | |
| 293 | + /// | |
| 294 | + /// A resize that lands *while* the caller walks its tree leaves the pass | |
| 295 | + /// measuring one size and the buffer another, and the strip between them | |
| 296 | + /// is painted with nothing but the clear colour — the charcoal band that | |
| 297 | + /// follows the edge being dragged. Comparing this with the window tells | |
| 298 | + /// the caller to walk the tree again before any of it is presented. | |
| 299 | + frame_dims: [u32; 2], | |
| 292 | 300 | /// `VIDYA_CAPTURE=<path>`: dump one painted frame, then stop. |
| 293 | 301 | capture: Option<String>, |
| 294 | 302 | /// `VIDYA_RESIZE_AT` parsed: (frame, width, height), in frame order. |
| @@ -513,6 +521,7 @@ impl App { | ||
| 513 | 521 | error: None, |
| 514 | 522 | may_present: true, |
| 515 | 523 | frames: 0, |
| 524 | + frame_dims: [0, 0], | |
| 516 | 525 | capture: std::env::var("VIDYA_CAPTURE").ok(), |
| 517 | 526 | resize_at: std::env::var("VIDYA_RESIZE_AT") |
| 518 | 527 | .unwrap_or_default() |
| @@ -659,9 +668,47 @@ impl App { | ||
| 659 | 668 | }; |
| 660 | 669 | |
| 661 | 670 | let input = gl.winit_state.take_egui_input(&gl.window); |
| 671 | + // The size that input reports the window to be, kept for | |
| 672 | + // `resized_mid_frame` to compare the window against once the caller | |
| 673 | + // has finished walking its tree. | |
| 674 | + let size = gl.window.inner_size(); | |
| 675 | + let dims = [size.width.max(1), size.height.max(1)]; | |
| 662 | 676 | let ctx = &self.handler.egui_ctx; |
| 663 | 677 | ctx.begin_pass(input); |
| 664 | 678 | self.stack.push_root(ctx); |
| 679 | + self.handler.frame_dims = dims; | |
| 680 | + } | |
| 681 | + | |
| 682 | + /// Whether the window changed size after this frame started laying out. | |
| 683 | + /// | |
| 684 | + /// True means the pass now open measured a window that no longer exists, | |
| 685 | + /// and presenting it would paint a smaller layout into a larger buffer. | |
| 686 | + /// The caller answers by discarding the pass and walking the tree again; | |
| 687 | + /// the tree is retained and holds no sizes of its own, so a second walk is | |
| 688 | + /// simply the same tree laid out against the window as it now is. | |
| 689 | + pub fn resized_mid_frame(&mut self) -> bool { | |
| 690 | + if !self.stack.is_active() { | |
| 691 | + return false; | |
| 692 | + } | |
| 693 | + self.pump(Duration::ZERO); | |
| 694 | + let Some(gl) = self.handler.gl.as_ref() else { | |
| 695 | + return false; | |
| 696 | + }; | |
| 697 | + let size = gl.window.inner_size(); | |
| 698 | + [size.width.max(1), size.height.max(1)] != self.handler.frame_dims | |
| 699 | + } | |
| 700 | + | |
| 701 | + /// Close the open pass and throw its output away, presenting nothing. | |
| 702 | + /// | |
| 703 | + /// egui has to be told a pass ended whether or not anybody wants what it | |
| 704 | + /// produced — leaving one open would make the next `begin_pass` panic — | |
| 705 | + /// so this ends it and drops the result on the floor. | |
| 706 | + pub fn discard_frame(&mut self) { | |
| 707 | + if !self.stack.is_active() { | |
| 708 | + return; | |
| 709 | + } | |
| 710 | + self.stack.unwind(); | |
| 711 | + let _ = self.handler.egui_ctx.end_pass(); | |
| 665 | 712 | } |
| 666 | 713 | |
| 667 | 714 | /// Close the pass, then hand the frame to the event loop to present. |
| @@ -289,6 +289,14 @@ struct Handler { | |||
| 289 | /// for it before the first present deadlocks. | 289 | /// for it before the first present deadlocks. |
| 290 | may_present: bool, | 290 | may_present: bool, |
| 291 | frames: u32, | 291 | frames: u32, |
| 292 | + /// The size, in pixels, the frame now being built laid itself out against. | ||
| 293 | + /// | ||
| 294 | + /// A resize that lands *while* the caller walks its tree leaves the pass | ||
| 295 | + /// measuring one size and the buffer another, and the strip between them | ||
| 296 | + /// is painted with nothing but the clear colour — the charcoal band that | ||
| 297 | + /// follows the edge being dragged. Comparing this with the window tells | ||
| 298 | + /// the caller to walk the tree again before any of it is presented. | ||
| 299 | + frame_dims: [u32; 2], | ||
| 292 | /// `VIDYA_CAPTURE=<path>`: dump one painted frame, then stop. | 300 | /// `VIDYA_CAPTURE=<path>`: dump one painted frame, then stop. |
| 293 | capture: Option<String>, | 301 | capture: Option<String>, |
| 294 | /// `VIDYA_RESIZE_AT` parsed: (frame, width, height), in frame order. | 302 | /// `VIDYA_RESIZE_AT` parsed: (frame, width, height), in frame order. |
| @@ -513,6 +521,7 @@ impl App { | |||
| 513 | error: None, | 521 | error: None, |
| 514 | may_present: true, | 522 | may_present: true, |
| 515 | frames: 0, | 523 | frames: 0, |
| 524 | + frame_dims: [0, 0], | ||
| 516 | capture: std::env::var("VIDYA_CAPTURE").ok(), | 525 | capture: std::env::var("VIDYA_CAPTURE").ok(), |
| 517 | resize_at: std::env::var("VIDYA_RESIZE_AT") | 526 | resize_at: std::env::var("VIDYA_RESIZE_AT") |
| 518 | .unwrap_or_default() | 527 | .unwrap_or_default() |
| @@ -659,9 +668,47 @@ impl App { | |||
| 659 | }; | 668 | }; |
| 660 | 669 | ||
| 661 | let input = gl.winit_state.take_egui_input(&gl.window); | 670 | let input = gl.winit_state.take_egui_input(&gl.window); |
| 671 | + // The size that input reports the window to be, kept for | ||
| 672 | + // `resized_mid_frame` to compare the window against once the caller | ||
| 673 | + // has finished walking its tree. | ||
| 674 | + let size = gl.window.inner_size(); | ||
| 675 | + let dims = [size.width.max(1), size.height.max(1)]; | ||
| 662 | let ctx = &self.handler.egui_ctx; | 676 | let ctx = &self.handler.egui_ctx; |
| 663 | ctx.begin_pass(input); | 677 | ctx.begin_pass(input); |
| 664 | self.stack.push_root(ctx); | 678 | self.stack.push_root(ctx); |
| 679 | + self.handler.frame_dims = dims; | ||
| 680 | + } | ||
| 681 | + | ||
| 682 | + /// Whether the window changed size after this frame started laying out. | ||
| 683 | + /// | ||
| 684 | + /// True means the pass now open measured a window that no longer exists, | ||
| 685 | + /// and presenting it would paint a smaller layout into a larger buffer. | ||
| 686 | + /// The caller answers by discarding the pass and walking the tree again; | ||
| 687 | + /// the tree is retained and holds no sizes of its own, so a second walk is | ||
| 688 | + /// simply the same tree laid out against the window as it now is. | ||
| 689 | + pub fn resized_mid_frame(&mut self) -> bool { | ||
| 690 | + if !self.stack.is_active() { | ||
| 691 | + return false; | ||
| 692 | + } | ||
| 693 | + self.pump(Duration::ZERO); | ||
| 694 | + let Some(gl) = self.handler.gl.as_ref() else { | ||
| 695 | + return false; | ||
| 696 | + }; | ||
| 697 | + let size = gl.window.inner_size(); | ||
| 698 | + [size.width.max(1), size.height.max(1)] != self.handler.frame_dims | ||
| 699 | + } | ||
| 700 | + | ||
| 701 | + /// Close the open pass and throw its output away, presenting nothing. | ||
| 702 | + /// | ||
| 703 | + /// egui has to be told a pass ended whether or not anybody wants what it | ||
| 704 | + /// produced — leaving one open would make the next `begin_pass` panic — | ||
| 705 | + /// so this ends it and drops the result on the floor. | ||
| 706 | + pub fn discard_frame(&mut self) { | ||
| 707 | + if !self.stack.is_active() { | ||
| 708 | + return; | ||
| 709 | + } | ||
| 710 | + self.stack.unwind(); | ||
| 711 | + let _ = self.handler.egui_ctx.end_pass(); | ||
| 665 | } | 712 | } |
| 666 | 713 | ||
| 667 | /// Close the pass, then hand the frame to the event loop to present. | 714 | /// Close the pass, then hand the frame to the event loop to present. |
modified
crates/jolt-vidya/src/lib.rs +32 -5 | @@ -522,17 +522,44 @@ pub extern "C" fn vidya_node_replace(parent: c_int, old_child: c_int, new_child: | ||
| 522 | 522 | }) |
| 523 | 523 | } |
| 524 | 524 | |
| 525 | +/// How many times a frame is walked again because the window resized under it. | |
| 526 | +/// | |
| 527 | +/// A drag produces a resize most frames, so one retry is the common case and | |
| 528 | +/// two is a drag fast enough to move twice inside a single walk. Past that the | |
| 529 | +/// frame goes out at whatever size it last measured: a cap is what keeps a | |
| 530 | +/// continuous drag from being an unbounded loop that never presents at all, | |
| 531 | +/// and never presenting is worse than presenting a frame one step behind. | |
| 532 | +const RESIZE_RETRIES: u32 = 2; | |
| 533 | + | |
| 525 | 534 | /// Paint the whole tree as one frame: a `vidya_begin_frame`, the walk, and a |
| 526 | 535 | /// `vidya_end_frame`. Inert with no window open. |
| 536 | +/// | |
| 537 | +/// The walk can happen more than once. A resize arriving while the tree is | |
| 538 | +/// being walked leaves the layout measuring the old window and the buffer | |
| 539 | +/// sized to the new one, and everything between the two is painted with the | |
| 540 | +/// clear colour — which is the band of bare background that follows the edge | |
| 541 | +/// while a window is dragged. The tree is retained and carries no sizes of its | |
| 542 | +/// own, so the answer is simply to throw the half-measured pass away and walk | |
| 543 | +/// it again against the window as it now is, before anything is presented. | |
| 527 | 544 | #[no_mangle] |
| 528 | 545 | pub extern "C" fn vidya_tree_frame() { |
| 529 | 546 | with_app((), |app| { |
| 530 | - app.begin_frame(); | |
| 531 | - TREE.with_borrow_mut(|tree| { | |
| 532 | - if let Some((ui, theme)) = app.ui() { | |
| 533 | - tree.paint(ui, theme); | |
| 547 | + for attempt in 0..RESIZE_RETRIES { | |
| 548 | + app.begin_frame(); | |
| 549 | + TREE.with_borrow_mut(|tree| { | |
| 550 | + if let Some((ui, theme)) = app.ui() { | |
| 551 | + tree.paint(ui, theme); | |
| 552 | + } | |
| 553 | + }); | |
| 554 | + // The last attempt keeps whatever it measured. Discarding here | |
| 555 | + // instead would leave no open pass for `end_frame` to present, and | |
| 556 | + // a drag long enough to exhaust the retries would stop painting | |
| 557 | + // altogether — the one outcome worse than a frame behind. | |
| 558 | + if attempt + 1 == RESIZE_RETRIES || !app.resized_mid_frame() { | |
| 559 | + break; | |
| 534 | 560 | } |
| 535 | - }); | |
| 561 | + app.discard_frame(); | |
| 562 | + } | |
| 536 | 563 | app.end_frame(); |
| 537 | 564 | }); |
| 538 | 565 | } |
| @@ -522,17 +522,44 @@ pub extern "C" fn vidya_node_replace(parent: c_int, old_child: c_int, new_child: | |||
| 522 | }) | 522 | }) |
| 523 | } | 523 | } |
| 524 | 524 | ||
| 525 | +/// How many times a frame is walked again because the window resized under it. | ||
| 526 | +/// | ||
| 527 | +/// A drag produces a resize most frames, so one retry is the common case and | ||
| 528 | +/// two is a drag fast enough to move twice inside a single walk. Past that the | ||
| 529 | +/// frame goes out at whatever size it last measured: a cap is what keeps a | ||
| 530 | +/// continuous drag from being an unbounded loop that never presents at all, | ||
| 531 | +/// and never presenting is worse than presenting a frame one step behind. | ||
| 532 | +const RESIZE_RETRIES: u32 = 2; | ||
| 533 | + | ||
| 525 | /// Paint the whole tree as one frame: a `vidya_begin_frame`, the walk, and a | 534 | /// Paint the whole tree as one frame: a `vidya_begin_frame`, the walk, and a |
| 526 | /// `vidya_end_frame`. Inert with no window open. | 535 | /// `vidya_end_frame`. Inert with no window open. |
| 536 | +/// | ||
| 537 | +/// The walk can happen more than once. A resize arriving while the tree is | ||
| 538 | +/// being walked leaves the layout measuring the old window and the buffer | ||
| 539 | +/// sized to the new one, and everything between the two is painted with the | ||
| 540 | +/// clear colour — which is the band of bare background that follows the edge | ||
| 541 | +/// while a window is dragged. The tree is retained and carries no sizes of its | ||
| 542 | +/// own, so the answer is simply to throw the half-measured pass away and walk | ||
| 543 | +/// it again against the window as it now is, before anything is presented. | ||
| 527 | #[no_mangle] | 544 | #[no_mangle] |
| 528 | pub extern "C" fn vidya_tree_frame() { | 545 | pub extern "C" fn vidya_tree_frame() { |
| 529 | with_app((), |app| { | 546 | with_app((), |app| { |
| 530 | - app.begin_frame(); | 547 | + for attempt in 0..RESIZE_RETRIES { |
| 531 | - TREE.with_borrow_mut(|tree| { | 548 | + app.begin_frame(); |
| 532 | - if let Some((ui, theme)) = app.ui() { | 549 | + TREE.with_borrow_mut(|tree| { |
| 533 | - tree.paint(ui, theme); | 550 | + if let Some((ui, theme)) = app.ui() { |
| 551 | + tree.paint(ui, theme); | ||
| 552 | + } | ||
| 553 | + }); | ||
| 554 | + // The last attempt keeps whatever it measured. Discarding here | ||
| 555 | + // instead would leave no open pass for `end_frame` to present, and | ||
| 556 | + // a drag long enough to exhaust the retries would stop painting | ||
| 557 | + // altogether — the one outcome worse than a frame behind. | ||
| 558 | + if attempt + 1 == RESIZE_RETRIES || !app.resized_mid_frame() { | ||
| 559 | + break; | ||
| 534 | } | 560 | } |
| 535 | - }); | 561 | + app.discard_frame(); |
| 562 | + } | ||
| 536 | app.end_frame(); | 563 | app.end_frame(); |
| 537 | }); | 564 | }); |
| 538 | } | 565 | } |
modified
crates/jolt-vidya/src/tree.rs +32 -0 | @@ -231,6 +231,14 @@ pub struct Tree { | ||
| 231 | 231 | /// uploaded from it. A frame that arrives twice between paints overwrites |
| 232 | 232 | /// the first, so a 30fps source cannot outrun a 60fps window into a queue. |
| 233 | 233 | feeds: HashMap<String, Feed>, |
| 234 | + /// The width a centred row measured last frame, by node id. A row is | |
| 235 | + /// indented to the middle of the space it is given, and nothing here | |
| 236 | + /// knows how wide it is until it has been painted once — so the previous | |
| 237 | + /// frame's width is what the indent is computed from. Kept here rather | |
| 238 | + /// than written back onto the node: props are cleared and rewritten on | |
| 239 | + /// every re-render, and a row would jump to the left edge for a frame on | |
| 240 | + /// every keystroke typed into it. | |
| 241 | + row_widths: HashMap<u32, f32>, | |
| 234 | 242 | pending: VecDeque<Event>, |
| 235 | 243 | /// The event most recently dequeued by `poll`, whose fields the accessors |
| 236 | 244 | /// read. Held here so the ABI can return a payload without out-parameters. |
| @@ -380,6 +388,7 @@ impl Default for Tree { | ||
| 380 | 388 | root: 0, |
| 381 | 389 | textures: HashMap::new(), |
| 382 | 390 | feeds: HashMap::new(), |
| 391 | + row_widths: HashMap::new(), | |
| 383 | 392 | pending: VecDeque::new(), |
| 384 | 393 | current: None, |
| 385 | 394 | }; |
| @@ -815,6 +824,29 @@ impl Tree { | ||
| 815 | 824 | tree.paint_children(id, ui, theme); |
| 816 | 825 | }); |
| 817 | 826 | }); |
| 827 | + } else if props.str("align") == "center" { | |
| 828 | + // `:align :center` puts a row on the middle of the | |
| 829 | + // width rather than against its left edge — what a | |
| 830 | + // compose bar wants on a window wider than the | |
| 831 | + // line being typed into it. | |
| 832 | + // | |
| 833 | + // Indented rather than laid out centred: egui | |
| 834 | + // places a row as it goes, and knows how wide it | |
| 835 | + // came out only once it is painted. The width it | |
| 836 | + // measured last frame is what the indent is | |
| 837 | + // computed from, which is exact for a row whose | |
| 838 | + // contents keep their size and one frame late for | |
| 839 | + // one that changes. | |
| 840 | + let last = tree.row_widths.get(&id).copied().unwrap_or(0.0); | |
| 841 | + ui.horizontal(|ui| { | |
| 842 | + let avail = ui.available_width(); | |
| 843 | + ui.add_space(((avail - last) * 0.5).max(0.0)); | |
| 844 | + let left = ui.cursor().min.x; | |
| 845 | + ui.spacing_mut().item_spacing = axis; | |
| 846 | + tree.paint_children(id, ui, theme); | |
| 847 | + let width = (ui.min_rect().max.x - left).max(0.0); | |
| 848 | + tree.row_widths.insert(id, width); | |
| 849 | + }); | |
| 818 | 850 | } else if props.bool("wrap", true) { |
| 819 | 851 | ui.horizontal_wrapped(|ui| { |
| 820 | 852 | ui.spacing_mut().item_spacing = axis; |
| @@ -231,6 +231,14 @@ pub struct Tree { | |||
| 231 | /// uploaded from it. A frame that arrives twice between paints overwrites | 231 | /// uploaded from it. A frame that arrives twice between paints overwrites |
| 232 | /// the first, so a 30fps source cannot outrun a 60fps window into a queue. | 232 | /// the first, so a 30fps source cannot outrun a 60fps window into a queue. |
| 233 | feeds: HashMap<String, Feed>, | 233 | feeds: HashMap<String, Feed>, |
| 234 | + /// The width a centred row measured last frame, by node id. A row is | ||
| 235 | + /// indented to the middle of the space it is given, and nothing here | ||
| 236 | + /// knows how wide it is until it has been painted once — so the previous | ||
| 237 | + /// frame's width is what the indent is computed from. Kept here rather | ||
| 238 | + /// than written back onto the node: props are cleared and rewritten on | ||
| 239 | + /// every re-render, and a row would jump to the left edge for a frame on | ||
| 240 | + /// every keystroke typed into it. | ||
| 241 | + row_widths: HashMap<u32, f32>, | ||
| 234 | pending: VecDeque<Event>, | 242 | pending: VecDeque<Event>, |
| 235 | /// The event most recently dequeued by `poll`, whose fields the accessors | 243 | /// The event most recently dequeued by `poll`, whose fields the accessors |
| 236 | /// read. Held here so the ABI can return a payload without out-parameters. | 244 | /// read. Held here so the ABI can return a payload without out-parameters. |
| @@ -380,6 +388,7 @@ impl Default for Tree { | |||
| 380 | root: 0, | 388 | root: 0, |
| 381 | textures: HashMap::new(), | 389 | textures: HashMap::new(), |
| 382 | feeds: HashMap::new(), | 390 | feeds: HashMap::new(), |
| 391 | + row_widths: HashMap::new(), | ||
| 383 | pending: VecDeque::new(), | 392 | pending: VecDeque::new(), |
| 384 | current: None, | 393 | current: None, |
| 385 | }; | 394 | }; |
| @@ -815,6 +824,29 @@ impl Tree { | |||
| 815 | tree.paint_children(id, ui, theme); | 824 | tree.paint_children(id, ui, theme); |
| 816 | }); | 825 | }); |
| 817 | }); | 826 | }); |
| 827 | + } else if props.str("align") == "center" { | ||
| 828 | + // `:align :center` puts a row on the middle of the | ||
| 829 | + // width rather than against its left edge — what a | ||
| 830 | + // compose bar wants on a window wider than the | ||
| 831 | + // line being typed into it. | ||
| 832 | + // | ||
| 833 | + // Indented rather than laid out centred: egui | ||
| 834 | + // places a row as it goes, and knows how wide it | ||
| 835 | + // came out only once it is painted. The width it | ||
| 836 | + // measured last frame is what the indent is | ||
| 837 | + // computed from, which is exact for a row whose | ||
| 838 | + // contents keep their size and one frame late for | ||
| 839 | + // one that changes. | ||
| 840 | + let last = tree.row_widths.get(&id).copied().unwrap_or(0.0); | ||
| 841 | + ui.horizontal(|ui| { | ||
| 842 | + let avail = ui.available_width(); | ||
| 843 | + ui.add_space(((avail - last) * 0.5).max(0.0)); | ||
| 844 | + let left = ui.cursor().min.x; | ||
| 845 | + ui.spacing_mut().item_spacing = axis; | ||
| 846 | + tree.paint_children(id, ui, theme); | ||
| 847 | + let width = (ui.min_rect().max.x - left).max(0.0); | ||
| 848 | + tree.row_widths.insert(id, width); | ||
| 849 | + }); | ||
| 818 | } else if props.bool("wrap", true) { | 850 | } else if props.bool("wrap", true) { |
| 819 | ui.horizontal_wrapped(|ui| { | 851 | ui.horizontal_wrapped(|ui| { |
| 820 | ui.spacing_mut().item_spacing = axis; | 852 | ui.spacing_mut().item_spacing = axis; |