From 97ac94dd7dec430ea063f05442e9ec2f2b59552f Mon Sep 17 00:00:00 2001 From: Michael Wilson Date: Tue, 31 Dec 2024 01:05:21 -0500 Subject: [PATCH] Better cancellation. (#57) The expiration mechanism for cancellation had an unintended side effect of preventing cancellation if one component of the song completed ahead of time. In other words, if a MIDI file finished playing but there is still audio to play, the song is no longer cancellable. Additionally, it would be possible, in some circumstances, for the completion of one aspect of a song to cancel others unexpectedly. The "expiration" concept was introduced to allow cancellation while still allowing a song to finish normally. This has been replaced with a simple concept of an atomic bool that indicates whether a song component (MIDI, DMX, or audio) has finished and, when used in combination with the new "notify" function, will allow a cancel_handle.wait() call to return without an actual cancellation happening. --- .github/workflows/mtrack.yaml | 2 +- .gitignore | 1 + .licensure.yml | 15 ++++- CHANGELOG.md | 14 +++++ Cargo.lock | 111 ++++++++++++++++------------------ src/audio/mock.rs | 7 ++- src/config.rs | 5 +- src/dmx/engine.rs | 3 - src/midi/midir.rs | 12 +++- src/midi/mock.rs | 7 ++- src/playsync.rs | 32 +++++----- 11 files changed, 116 insertions(+), 93 deletions(-) diff --git a/.github/workflows/mtrack.yaml b/.github/workflows/mtrack.yaml index 5058995..a6f0123 100644 --- a/.github/workflows/mtrack.yaml +++ b/.github/workflows/mtrack.yaml @@ -95,7 +95,7 @@ jobs: - uses: dtolnay/rust-toolchain@stable - uses: Swatinem/rust-cache@v2 - name: Install licensure - run: cargo install licensure@0.3.2 + run: cargo install licensure@0.5.1 - name: Check for licenses run: licensure --check -p diff --git a/.gitignore b/.gitignore index 323c058..e19e8ed 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ target .vscode cobertura.xml +lcov.info diff --git a/.licensure.yml b/.licensure.yml index 3802ddd..cd8673b 100644 --- a/.licensure.yml +++ b/.licensure.yml @@ -11,13 +11,26 @@ excludes: - .*\.yaml - .*\.wav - .*\.mid + - lcov.info licenses: - files: any ident: GPL-3.0 authors: - name: Michael Wilson email: mike@mdwn.dev - auto_template: true + template: |+ + Copyright (C) [year] [name of author] + + This program is free software: you can redistribute it and/or modify it under + the terms of the GNU General Public License as published by the Free Software + Foundation, version 3. + + This program is distributed in the hope that it will be useful, but WITHOUT + ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + FOR A PARTICULAR PURPOSE. See the GNU General Public License for more details. + + You should have received a copy of the GNU General Public License along with + this program. If not, see . comments: - columns: 80 diff --git a/CHANGELOG.md b/CHANGELOG.md index f7850f2..5c42571 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +The expiration mechanism for cancellation had an unintended side effect +of preventing cancellation if one component of the song completed ahead +of time. In other words, if a MIDI file finished playing but there is +still audio to play, the song is no longer cancellable. Additionally, it +would be possible, in some circumstances, for the completion of one +aspect of a song to cancel others unexpectedly. + +The "expiration" concept was introduced to allow cancellation while +still allowing a song to finish normally. This has been replaced with a +simple concept of an atomic bool that indicates whether a song component +(MIDI, DMX, or audio) has finished and, when used in combination with +the new "notify" function, will allow a cancel\_handle.wait() call to +return without an actual cancellation happening. + ## [0.1.8] - Better MacOS support. MacOS support is improved. It's not super thoroughly tested, but has been tested diff --git a/Cargo.lock b/Cargo.lock index 620c32c..673af42 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1,6 +1,6 @@ # This file is automatically @generated by Cargo. # It is not intended for manual editing. -version = 3 +version = 4 [[package]] name = "addr2line" @@ -99,9 +99,9 @@ dependencies = [ [[package]] name = "anyhow" -version = "1.0.94" +version = "1.0.95" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c1fd03a028ef38ba2276dce7e33fcd6369c158a1bca17946c4b1b701891c1ff7" +checksum = "34ac096ce696dc2fcabef30516bb13c0a68a11d30131d3df6f04711467681b04" [[package]] name = "autocfg" @@ -139,7 +139,7 @@ dependencies = [ "regex", "rustc-hash", "shlex", - "syn 2.0.90", + "syn 2.0.93", ] [[package]] @@ -174,9 +174,9 @@ checksum = "325918d6fe32f23b19878fe4b34794ae41fc19ddbe53b10571a4874d44ffd39b" [[package]] name = "cc" -version = "1.2.4" +version = "1.2.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9157bbaa6b165880c27a4293a474c91cdcf265cc68cc829bf10be0964a391caf" +checksum = "8d6dbb628b8f8555f86d0323c2eb39e3ec81901f4b83e091db8a6a76d316a333" dependencies = [ "jobserver", "libc", @@ -246,7 +246,7 @@ dependencies = [ "heck 0.5.0", "proc-macro2", "quote", - "syn 2.0.90", + "syn 2.0.93", ] [[package]] @@ -353,9 +353,9 @@ dependencies = [ [[package]] name = "crossbeam-deque" -version = "0.8.5" +version = "0.8.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "613f8cc01fe9cf1a3eb3d7f488fd2fa8388403e97039e2f73692932e291a770d" +checksum = "9dd111b7b7f7d55b72c0a6ae361660ee5853c9af73f70c3c2ef6858b950e2e51" dependencies = [ "crossbeam-epoch", "crossbeam-utils", @@ -372,9 +372,9 @@ dependencies = [ [[package]] name = "crossbeam-utils" -version = "0.8.20" +version = "0.8.21" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "22ec99545bb0ed0ea7bb9b8e1e9122ea386ff8a48c0922e43f36d45ab09e0e80" +checksum = "d0a5c400df2834b80a4c3327b3aad3a4c4cd4de0629063962b03235697506a28" [[package]] name = "dasp_sample" @@ -412,9 +412,9 @@ checksum = "37909eebbb50d72f9059c3b6d82c0463f2ff062c9e95845c43a6c9c0355411be" [[package]] name = "fixedbitset" -version = "0.4.2" +version = "0.5.7" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0ce7134b9999ecaf8bcd65542e436736ef32ddca1b3e06094cb6ec5755203b80" +checksum = "1d674e81391d1e1ab681a28d99df07927c6d4aa5b027d7da16ba32d1d21ecd99" [[package]] name = "futures" @@ -472,7 +472,7 @@ checksum = "162ee34ebcb7c64a8abebc059ce0fee27c2262618d7b60ed8faf72fef13c3650" dependencies = [ "proc-macro2", "quote", - "syn 2.0.90", + "syn 2.0.93", ] [[package]] @@ -513,15 +513,15 @@ checksum = "07e28edb80900c19c28f1072f2e8aeca7fa06b23cd4169cefe1af5aa3260783f" [[package]] name = "glob" -version = "0.3.1" +version = "0.3.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d2fabcfbdc87f4758337ca535fb41a6d701b65693ce38287d856d1674551ec9b" +checksum = "a8d1add55171497b4705a648c6b583acafb01d58050a51727785f0b2c8e0a2b2" [[package]] name = "hashbrown" -version = "0.15.2" +version = "0.14.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bf151400ff0baff5465007dd2f3e717f3fe502074ca563069ce3a6629d07b289" +checksum = "e5274423e17b7c9fc20b6e7e208532f9b19825d82dfd615708b70edd83df41f1" [[package]] name = "heck" @@ -537,11 +537,11 @@ checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" [[package]] name = "home" -version = "0.5.9" +version = "0.5.11" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e3d1354bf6b7235cb4a0576c2619fd4ed18183f689b12b006a0ee7329eeff9a5" +checksum = "589533453244b0995c858700322199b2becb13b627df2851f64a2775d024abcf" dependencies = [ - "windows-sys 0.52.0", + "windows-sys 0.59.0", ] [[package]] @@ -552,9 +552,9 @@ checksum = "62adaabb884c94955b19907d60019f4e145d091c75345379e70d1ee696f7854f" [[package]] name = "indexmap" -version = "2.7.0" +version = "2.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "62f822373a4fe84d4bb149bf54e584a7f4abec90e072ed49cda0edea5b95471f" +checksum = "68b900aa2f7301e21c36462b170ee99994de34dff39a4a6a528e80e7376d07e5" dependencies = [ "equivalent", "hashbrown", @@ -639,9 +639,9 @@ checksum = "bbd2bcb4c963f2ddae06a2efc7e9f3591312473c50c6685e1f298068316e66fe" [[package]] name = "libc" -version = "0.2.168" +version = "0.2.169" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5aaeb2981e0606ca11d79718f8bb01164f1d6ed75080182d3abf017e6d244b6d" +checksum = "b5aba8db14291edd000dfcc4d620c7ebfb122c613afb886ca8803fa4e128a20a" [[package]] name = "libloading" @@ -724,9 +724,9 @@ checksum = "68354c5c6bd36d73ff3feceb05efa59b6acb7626617f4962be322a825e61f79a" [[package]] name = "miniz_oxide" -version = "0.8.0" +version = "0.8.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e2d80299ef12ff69b16a84bb182e3b9df68b5a91574d3d4fa6e41b65deec4df1" +checksum = "4ffbe83022cedc1d264172192511ae958937694cd57ce297164951b8b3568394" dependencies = [ "adler2", ] @@ -829,7 +829,7 @@ checksum = "ed3955f1a9c7c0c15e092f9c887db08b1fc683305fdf6eb6684f22555355e202" dependencies = [ "proc-macro2", "quote", - "syn 2.0.90", + "syn 2.0.93", ] [[package]] @@ -859,14 +859,14 @@ dependencies = [ "proc-macro-crate", "proc-macro2", "quote", - "syn 2.0.90", + "syn 2.0.93", ] [[package]] name = "object" -version = "0.36.5" +version = "0.36.7" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "aedf0a2d09c573ed1d8d85b30c119153926a2b36dce0ab28322c09a117a4683e" +checksum = "62948e14d923ea95ea2c7c86c71013138b66525b86bdc08d2dcc262bdb497b87" dependencies = [ "memchr", ] @@ -945,9 +945,9 @@ dependencies = [ [[package]] name = "petgraph" -version = "0.6.5" +version = "0.6.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b4c5cc86750666a3ed20bdaf5ca2a0344f9c67674cae0515bec2da16fbaa47db" +checksum = "c94eb96835f05ec51384814c9b2daef83f68486f67a0e2e9680e0f698dca808e" dependencies = [ "fixedbitset", "indexmap", @@ -1055,9 +1055,9 @@ dependencies = [ [[package]] name = "quote" -version = "1.0.37" +version = "1.0.38" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b5b9d34b8991d19d98081b46eacdd8eb58c6f2b201139f7c5f643cc155a633af" +checksum = "0e4dccaaaf89514f546c693ddc140f729f958c247918a13380cccc6078391acc" dependencies = [ "proc-macro2", ] @@ -1177,22 +1177,22 @@ checksum = "94143f37725109f92c262ed2cf5e59bce7498c01bcc1502d7b9afe439a4e9f49" [[package]] name = "serde" -version = "1.0.216" +version = "1.0.217" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0b9781016e935a97e8beecf0c933758c97a5520d32930e460142b4cd80c6338e" +checksum = "02fc4265df13d6fa1d00ecff087228cc0a2b5f3c0e87e258d8b94a156e984c70" dependencies = [ "serde_derive", ] [[package]] name = "serde_derive" -version = "1.0.216" +version = "1.0.217" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "46f859dbbf73865c6627ed570e78961cd3ac92407a2d117204c49232485da55e" +checksum = "5a9bf7cf98d04a2b28aead066b7496853d4779c9cc183c440dbac457641e19a0" dependencies = [ "proc-macro2", "quote", - "syn 2.0.90", + "syn 2.0.93", ] [[package]] @@ -1250,9 +1250,9 @@ checksum = "3c5e1a9a646d36c3599cd173a41282daf47c44583ad367b8e6837255952e5c67" [[package]] name = "spin_sleep" -version = "1.2.1" +version = "1.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "64bd7227d85bfd1b8df51e0d83da36d9baaee85eb75730386ef8e3ab6f2a2ea3" +checksum = "4196b31c8c1dc443543be4f4d0e827657fbf2b87387e5c8f229b14f1c046718a" dependencies = [ "windows-sys 0.59.0", ] @@ -1276,9 +1276,9 @@ dependencies = [ [[package]] name = "syn" -version = "2.0.90" +version = "2.0.93" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "919d3b74a5dd0ccd15aeb8f93e7006bd9e14c295087c9896a110f490752bcf31" +checksum = "9c786062daee0d6db1132800e623df74274a0a87322d8e183338e01b3d98d058" dependencies = [ "proc-macro2", "quote", @@ -1315,7 +1315,7 @@ checksum = "4fee6c4efc90059e10f81e6d42c60a18f76588c3d74cb83a0b242a2b6c7504c1" dependencies = [ "proc-macro2", "quote", - "syn 2.0.90", + "syn 2.0.93", ] [[package]] @@ -1347,7 +1347,7 @@ checksum = "693d596312e88961bc67d7f1f97af8a70227d9f90c31bba5806eec004978d752" dependencies = [ "proc-macro2", "quote", - "syn 2.0.90", + "syn 2.0.93", ] [[package]] @@ -1386,7 +1386,7 @@ checksum = "395ae124c09f9e6918a2310af6038fba074bcf474ac352496d5910dd59a2226d" dependencies = [ "proc-macro2", "quote", - "syn 2.0.90", + "syn 2.0.93", ] [[package]] @@ -1479,7 +1479,7 @@ dependencies = [ "log", "proc-macro2", "quote", - "syn 2.0.90", + "syn 2.0.93", "wasm-bindgen-shared", ] @@ -1514,7 +1514,7 @@ checksum = "30d7a95b763d3c45903ed6c81f156801839e5ee968bb07e534c44df0fcd330c2" dependencies = [ "proc-macro2", "quote", - "syn 2.0.90", + "syn 2.0.93", "wasm-bindgen-backend", "wasm-bindgen-shared", ] @@ -1628,7 +1628,7 @@ checksum = "f6fc35f58ecd95a9b71c4f2329b911016e6bec66b3f2e6a4aad86bd2e99e2f9b" dependencies = [ "proc-macro2", "quote", - "syn 2.0.90", + "syn 2.0.93", ] [[package]] @@ -1639,7 +1639,7 @@ checksum = "08990546bf4edef8f431fa6326e032865f27138718c587dc21bc0265bbcb57cc" dependencies = [ "proc-macro2", "quote", - "syn 2.0.90", + "syn 2.0.93", ] [[package]] @@ -1660,15 +1660,6 @@ dependencies = [ "windows-targets 0.42.2", ] -[[package]] -name = "windows-sys" -version = "0.52.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "282be5f36a8ce781fad8c8ae18fa3f9beff57ec1b52cb3de0789201425d9a33d" -dependencies = [ - "windows-targets 0.52.6", -] - [[package]] name = "windows-sys" version = "0.59.0" diff --git a/src/audio/mock.rs b/src/audio/mock.rs index fb08798..6f37a8d 100644 --- a/src/audio/mock.rs +++ b/src/audio/mock.rs @@ -70,8 +70,10 @@ impl super::Device for Device { let (sleep_tx, sleep_rx) = mpsc::channel::<()>(); self.is_playing.store(true, Ordering::Relaxed); + let finished = Arc::new(AtomicBool::new(false)); let join_handle = { let cancel_handle = cancel_handle.clone(); + let finished = finished.clone(); // Wait until the song is cancelled or until the song is done. thread::spawn(move || { play_barrier.wait(); @@ -80,11 +82,12 @@ impl super::Device for Device { let _ = sleep_rx.recv_timeout(song.duration); // Expire at the end of playback. - cancel_handle.expire(); + finished.store(true, Ordering::Relaxed); + cancel_handle.notify(); }) }; - cancel_handle.wait(); + cancel_handle.wait(finished); sleep_tx.send(())?; let join_result = join_handle.join(); diff --git a/src/config.rs b/src/config.rs index 0816488..d788fa1 100644 --- a/src/config.rs +++ b/src/config.rs @@ -141,11 +141,10 @@ pub fn init_player_and_controller( crate::playlist::Playlist::from_songs(songs)?, status_events, ); - let controller = crate::controller::Controller::new( + crate::controller::Controller::new( player, player_config.controller.driver(midi_device.clone())?, - ); - controller + ) } fn get_songs_path(player_path: &PathBuf, songs: String) -> PathBuf { diff --git a/src/dmx/engine.rs b/src/dmx/engine.rs index d81287c..ade51a3 100644 --- a/src/dmx/engine.rs +++ b/src/dmx/engine.rs @@ -151,7 +151,6 @@ impl Engine { play_barrier.wait(); player.play(&dmx_midi_sheet.sheet); - cancel_handle.expire(); play_finished.store(true, std::sync::atomic::Ordering::Relaxed); }) }) @@ -173,8 +172,6 @@ impl Engine { .expect("Empty barrier join handle should join immediately"); }); - cancel_handle.wait(); - if cancel_handle.is_cancelled() { info!("DMX playback has been cancelled."); } diff --git a/src/midi/midir.rs b/src/midi/midir.rs index a1d50ad..df31ba8 100644 --- a/src/midi/midir.rs +++ b/src/midi/midir.rs @@ -17,7 +17,10 @@ use std::{ error::Error, fmt, mem, ops::Add, - sync::{Arc, Barrier, Mutex}, + sync::{ + atomic::{AtomicBool, Ordering}, + Arc, Barrier, Mutex, + }, thread, time::{self, Instant}, }; @@ -140,8 +143,10 @@ impl super::Device for Device { "Playing song MIDI." ); + let finished = Arc::new(AtomicBool::new(false)); let join_handle = { let cancel_handle = cancel_handle.clone(); + let finished = finished.clone(); // Wrap the midir connection in a cancel connection so that we can stop playback. let midir_connection = output.connect(output_port, "mtrack player")?; @@ -158,11 +163,12 @@ impl super::Device for Device { thread::spawn(move || { play_barrier.wait(); player.play(&midi_sheet.sheet); - cancel_handle.expire(); + finished.store(true, Ordering::Relaxed); + cancel_handle.notify(); }) }; - cancel_handle.wait(); + cancel_handle.wait(finished); if cancel_handle.is_cancelled() { info!("MIDI playback has been cancelled."); diff --git a/src/midi/mock.rs b/src/midi/mock.rs index 6a18444..2d46655 100644 --- a/src/midi/mock.rs +++ b/src/midi/mock.rs @@ -139,8 +139,10 @@ impl super::Device for Device { let (sleep_tx, sleep_rx) = mpsc::channel::<()>(); + let finished = Arc::new(AtomicBool::new(false)); let join_handle = { let cancel_handle = cancel_handle.clone(); + let finished = finished.clone(); // Wait until the song is cancelled or until the song is done. thread::spawn(move || { play_barrier.wait(); @@ -148,11 +150,12 @@ impl super::Device for Device { let _ = sleep_rx.recv_timeout(song.duration); // Expire at the end of playback. - cancel_handle.expire(); + finished.store(true, Ordering::Relaxed); + cancel_handle.notify(); }) }; - cancel_handle.wait(); + cancel_handle.wait(finished); sleep_tx.send(())?; if join_handle.join().is_err() { return Err("Error while joining thread!".into()); diff --git a/src/playsync.rs b/src/playsync.rs index 5b7495b..c674a02 100644 --- a/src/playsync.rs +++ b/src/playsync.rs @@ -11,14 +11,13 @@ // You should have received a copy of the GNU General Public License along with // this program. If not, see . // -use std::sync::{Arc, Condvar, Mutex}; +use std::sync::{atomic::AtomicBool, atomic::Ordering, Arc, Condvar, Mutex}; /// Represents the current cancel state. #[derive(PartialEq)] enum CancelState { Untouched, Cancelled, - Expired, } /// A cancel handle is passed to the device during a play operation. It's the player's responsibility @@ -45,25 +44,23 @@ impl CancelHandle { *self.cancelled.lock().expect("Error getting lock") == CancelState::Cancelled } - /// Waits for the cancel handle to expire or be cancelled. - pub fn wait(&self) { + /// Waits for the cancel handle to be cancelled or for finished to be set to true. + pub fn wait(&self, finished: Arc) { let _unused = self .condvar .wait_while( self.cancelled.lock().expect("Error getting lock"), - |cancelled| *cancelled == CancelState::Untouched, + |cancelled| { + *cancelled == CancelState::Untouched && !finished.load(Ordering::Relaxed) + }, ) .expect("Error getting lock"); } - /// Expire the cancel handle. This will let all active cancel handle waits proceed without - /// setting the handle to cancelled. - pub fn expire(&self) { - let mut cancel_state = self.cancelled.lock().expect("Error getting lock"); - if *cancel_state == CancelState::Untouched { - *cancel_state = CancelState::Expired; - self.condvar.notify_all(); - } + /// Notifies the cancel handle to see if this the song has been cancelled or if the + /// particular element has finished. + pub fn notify(&self) { + self.condvar.notify_all(); } /// Cancel the device process. @@ -71,7 +68,7 @@ impl CancelHandle { let mut cancel_state = self.cancelled.lock().expect("Error getting lock"); if *cancel_state == CancelState::Untouched { *cancel_state = CancelState::Cancelled; - self.condvar.notify_all(); + self.notify(); } } } @@ -89,7 +86,7 @@ mod test { let join = { let cancel_handle = cancel_handle.clone(); - thread::spawn(move || cancel_handle.wait()) + thread::spawn(move || cancel_handle.wait(Arc::new(AtomicBool::new(false)))) }; cancel_handle.cancel(); @@ -98,16 +95,15 @@ mod test { } #[test] - fn test_cancel_handle_expired() { + fn test_cancel_handle_finished() { let cancel_handle = CancelHandle::new(); assert!(!cancel_handle.is_cancelled()); let join = { let cancel_handle = cancel_handle.clone(); - thread::spawn(move || cancel_handle.wait()) + thread::spawn(move || cancel_handle.wait(Arc::new(AtomicBool::new(true)))) }; - cancel_handle.expire(); assert!(join.join().is_ok()); assert!(!cancel_handle.is_cancelled()); }