Retry the OpenH264 download, and say what failed and how to try again

crates/mediad/src/picture/desktop.rs

@@ -1006,7 +1006,14 @@ fn capture_x11() -> Result<Rgba, String> {
     Ok(Rgba { width: width as u32, height: height as u32, pixels: reply.data, bgra: true })
 }
 
-const DOWNLOAD_FAILED: &str = "OpenH264 could not be downloaded. The call stays a voice call.";
+/// What the call is told when Cisco's host could not be reached. A VPN or a firewall that
+/// quietly drops the host is the usual reason, and that is said, so "Connecting…" is not
+/// read as a slow network.
+const DOWNLOAD_FAILED: &str = "OpenH264 could not be downloaded. A VPN or firewall may be blocking ciscobinary.openh264.org. The call stays a voice call. Turn share or camera on again to retry.";
+/// What the call is told when the download arrived but the disk would not take it.
+const SAVE_FAILED: &str = "OpenH264 could not be written to disk. The call stays a voice call. Turn share or camera on again to retry.";
+/// What the call is told when the download thread could not be started at all.
+const START_FAILED: &str = "OpenH264 could not start downloading. The call stays a voice call. Turn share or camera on again to retry.";
 
 enum Fetch {
     Idle,
@@ -1019,7 +1026,8 @@ fn fetch() -> &'static Mutex<Fetch> {
     FETCH.get_or_init(|| Mutex::new(Fetch::Idle))
 }
 
-/// Starts the download when the binary is not here yet, and returns at once.
+/// Starts the download when the binary is not here yet, and returns at once. One already on
+/// disk (a download that worked before is kept for every start after it) needs no network.
 pub fn warm() {
     let Ok(path) = library_path() else {
         return;
@@ -1050,23 +1058,24 @@ fn start_download(path: PathBuf) {
     *state = Fetch::Busy;
     drop(state);
     let started = std::thread::Builder::new().name("openh264".into()).spawn(move || {
-        let result = download(&path);
+        let result = download_with_retries(&path);
         if let Ok(mut state) = fetch().lock() {
             *state = match result {
                 Ok(()) => Fetch::Idle,
-                Err(err) => Fetch::Failed(err),
+                Err(words) => Fetch::Failed(words),
             };
         }
     });
     if started.is_err() {
         if let Ok(mut state) = fetch().lock() {
-            *state = Fetch::Failed(DOWNLOAD_FAILED.to_owned());
+            *state = Fetch::Failed(START_FAILED.to_owned());
         }
     }
 }
 
 /// Cisco's OpenH264 2.6.0 for this machine. A download in progress is [`CODEC_LATER`], not a
-/// failure, and this never blocks on the network.
+/// failure, and this never blocks on the network. A failed download says its words, and the
+/// next share or camera that is turned on starts a new download.
 fn load_api() -> Result<OpenH264API, String> {
     let path = library_path()?;
     if let Some(api) = open_cached(&path) {
@@ -1099,15 +1108,33 @@ fn open_cached(path: &Path) -> Option<OpenH264API> {
 
 fn library_path() -> Result<PathBuf, String> {
     let name = library_name().ok_or_else(|| "OpenH264 has no build for this system.".to_owned())?;
-    let dir = if let Some(local) = std::env::var_os("LOCALAPPDATA") {
+    let dir = cache_dir(
+        std::env::var_os("LOCALAPPDATA"),
+        std::env::var_os("HOME"),
+        std::env::temp_dir(),
+    );
+    std::fs::create_dir_all(&dir).map_err(|err| {
+        tracing::warn!("mediad: OpenH264's cache folder could not be made: {err}");
+        SAVE_FAILED.to_owned()
+    })?;
+    Ok(dir.join(name))
+}
+
+/// Where the downloaded codec is kept. `%LOCALAPPDATA%\tinyclient` on Windows,
+/// `~/.cache/tinyclient` wherever there is a home, and the temp folder last: a place that
+/// survives a restart, so Cisco is asked once and every later call finds the file on disk.
+fn cache_dir(
+    local: Option<std::ffi::OsString>,
+    home: Option<std::ffi::OsString>,
+    fallback: PathBuf,
+) -> PathBuf {
+    if let Some(local) = local {
         PathBuf::from(local).join("tinyclient")
-    } else if let Some(home) = std::env::var_os("HOME") {
+    } else if let Some(home) = home {
         PathBuf::from(home).join(".cache").join("tinyclient")
     } else {
-        std::env::temp_dir().join("tinyclient")
-    };
-    std::fs::create_dir_all(&dir).map_err(|err| err.to_string())?;
-    Ok(dir.join(name))
+        fallback.join("tinyclient")
+    }
 }
 
 fn library_name() -> Option<&'static str> {
@@ -1138,20 +1165,77 @@ fn library_url(name: &str) -> String {
     format!("https://ciscobinary.openh264.org/{name}.bz2")
 }
 
-fn download(path: &Path) -> Result<(), String> {
-    let name = path.file_name().and_then(|name| name.to_str()).ok_or_else(|| "OpenH264 has no name.".to_owned())?;
+/// How many tries the download gets before it gives up and says so. Three, each with its
+/// own 20 seconds: a flaky link gets its second chance, and a blocked one still ends in
+/// said words inside about a minute instead of a share that waits forever.
+const ATTEMPTS: u32 = 3;
+
+/// The pause after attempt `attempt` (0 comes after the first try) grows, so a busy or
+/// blocked network is not hammered: 1, 4, 16 seconds, and every later one 64.
+fn retry_delay(attempt: u32) -> Duration {
+    Duration::from_secs(1u64 << (attempt * 2).min(6))
+}
+
+/// The download, tried up to [`ATTEMPTS`] times with a growing pause. Every failure is
+/// logged precisely and said plainly once the tries are out.
+fn download_with_retries(path: &Path) -> Result<(), String> {
+    let mut words = DOWNLOAD_FAILED.to_owned();
+    for attempt in 0..ATTEMPTS {
+        if attempt > 0 {
+            std::thread::sleep(retry_delay(attempt - 1));
+        }
+        match download(path) {
+            Ok(()) => return Ok(()),
+            Err(failed) => {
+                tracing::warn!("mediad: OpenH264 download attempt {} of {ATTEMPTS} failed", attempt + 1);
+                words = failed.to_owned();
+            }
+        }
+    }
+    Err(words)
+}
+
+/// One try: Cisco's bz2 of this machine's library, written whole or not at all. The words of
+/// a failure name the step that went wrong; the precise error is only in the log. A stalled
+/// read is cut off after 20 seconds, so a VPN that takes the connection but not the bytes
+/// cannot hold this thread forever.
+fn download(path: &Path) -> Result<(), &'static str> {
+    let name = path.file_name().and_then(|name| name.to_str()).ok_or_else(|| {
+        tracing::warn!("mediad: OpenH264 has no file name to fetch");
+        DOWNLOAD_FAILED
+    })?;
     let url = library_url(name);
+    // The 20 seconds are the whole try's deadline, body read included (`into_reader` keeps
+    // it), so a VPN that takes the connection but not the bytes cannot hold this thread.
     let response = ureq::get(&url).timeout(Duration::from_secs(20)).call().map_err(|err| {
-        tracing::warn!("mediad: OpenH264 download failed: {err}");
-        DOWNLOAD_FAILED.to_owned()
+        tracing::warn!("mediad: OpenH264 could not be fetched from Cisco: {err}");
+        DOWNLOAD_FAILED
     })?;
     let mut packed = Vec::new();
-    response.into_reader().read_to_end(&mut packed).map_err(|err| err.to_string())?;
+    response
+        .into_reader()
+        .take(64 * 1024 * 1024)
+        .read_to_end(&mut packed)
+        .map_err(|err| {
+            tracing::warn!("mediad: OpenH264's download was cut short: {err}");
+            DOWNLOAD_FAILED
+        })?;
     let mut plain = Vec::new();
-    bzip2_rs::DecoderReader::new(&packed[..]).read_to_end(&mut plain).map_err(|err| err.to_string())?;
+    bzip2_rs::DecoderReader::new(&packed[..])
+        .read_to_end(&mut plain)
+        .map_err(|err| {
+            tracing::warn!("mediad: OpenH264's download did not uncompress: {err}");
+            DOWNLOAD_FAILED
+        })?;
     let tmp = path.with_extension("part");
-    std::fs::write(&tmp, &plain).map_err(|err| err.to_string())?;
-    std::fs::rename(&tmp, path).map_err(|err| err.to_string())?;
+    std::fs::write(&tmp, &plain).map_err(|err| {
+        tracing::warn!("mediad: OpenH264 could not be written to disk: {err}");
+        SAVE_FAILED
+    })?;
+    std::fs::rename(&tmp, path).map_err(|err| {
+        tracing::warn!("mediad: OpenH264 could not be moved into place: {err}");
+        SAVE_FAILED
+    })?;
     Ok(())
 }
 
@@ -1290,6 +1374,47 @@ mod tests {
         assert!(!url.contains("github.com"), "{url}");
     }
 
+    #[test]
+    fn the_download_is_tried_again_before_it_gives_up() {
+        assert_eq!(ATTEMPTS, 3, "a flaky link gets another try, a blocked one does not wait all day");
+        assert_eq!(retry_delay(0), Duration::from_secs(1));
+        assert_eq!(retry_delay(1), Duration::from_secs(4));
+        assert_eq!(retry_delay(2), Duration::from_secs(16));
+        assert!(retry_delay(9) == retry_delay(10), "the pause stops growing");
+        assert!(retry_delay(10) <= Duration::from_secs(64));
+    }
+
+    #[test]
+    fn the_words_say_what_failed_and_how_to_retry() {
+        for words in [DOWNLOAD_FAILED, SAVE_FAILED, START_FAILED] {
+            assert!(words.contains("OpenH264"), "{words}");
+            assert!(words.contains("voice call"), "{words}");
+            assert!(words.contains("retry"), "{words}");
+        }
+        assert!(DOWNLOAD_FAILED.contains("ciscobinary.openh264.org"), "{DOWNLOAD_FAILED}");
+        assert!(!SAVE_FAILED.contains("downloaded"), "{SAVE_FAILED}");
+    }
+
+    #[test]
+    fn the_codec_is_cached_where_a_restart_finds_it() {
+        let tmp = std::env::temp_dir();
+        let local = cache_dir(Some("C:\\Users\\ada".into()), None, tmp.clone());
+        assert_eq!(local, PathBuf::from("C:\\Users\\ada").join("tinyclient"));
+        let home = cache_dir(None, Some("/home/ada".into()), tmp.clone());
+        assert_eq!(home, PathBuf::from("/home/ada").join(".cache").join("tinyclient"));
+        assert_eq!(cache_dir(None, None, tmp.clone()), tmp.join("tinyclient"), "the temp folder is the last resort");
+    }
+
+    /// A file that is not Cisco's cannot be loaded (the loader checks the hash), so it is
+    /// taken away and downloaded again: nothing broken is ever kept.
+    #[test]
+    fn a_broken_cached_file_is_taken_away() {
+        let path = std::env::temp_dir().join("tinyclient-test-not-a-codec.dll");
+        std::fs::write(&path, b"not a codec").unwrap();
+        assert!(open_cached(&path).is_none());
+        assert!(!path.exists(), "the broken file is gone");
+    }
+
     /// A camera, or a screen, through real OpenH264: `cargo test -p mediad --lib -- --ignored`
     /// on a machine that has the binary. The software backend is opened by name, so a machine
     /// with a hardware encoder still tests OpenH264 here.

docs/product/06-calls.md

@@ -12,7 +12,7 @@ A Tiny room joins with sound when the server's audio is up: one send-only Opus m
 
 ## Pictures, on a Tiny room
 
-A Tiny call can send a camera and a screen, and it receives one H.264 picture. On a desktop, what it sends is encoded first by the machine's own hardware: on Windows, Media Foundation lists the hardware H.264 encoders (NVIDIA, Intel, AMD), and the first one that takes the job runs it at the size, frame rate and bitrate the share chose, at constrained baseline, with no download and hardly any CPU. A machine that lists no such encoder, or one that refuses its setup, falls back to OpenH264 constrained baseline, downloaded from Cisco the first time a picture is encoded. If that download fails, the call stays voice. macOS and Linux send through OpenH264 as they always have; whatever arrives is decoded through OpenH264 on every desktop.
+A Tiny call can send a camera and a screen, and it receives one H.264 picture. On a desktop, what it sends is encoded first by the machine's own hardware: on Windows, Media Foundation lists the hardware H.264 encoders (NVIDIA, Intel, AMD), and the first one that takes the job runs it at the size, frame rate and bitrate the share chose, at constrained baseline, with no download and hardly any CPU. A machine that lists no such encoder, or one that refuses its setup, falls back to OpenH264 constrained baseline, downloaded from Cisco the first time a picture is encoded, and tried three times with a growing pause before it gives up. A failed download is said in the call dock with the reason (a VPN that blocks Cisco's host is the usual one) and how to try again: turn the share or the camera on once more. Until then the call stays voice. The downloaded file is kept across starts (in `%LOCALAPPDATA%\tinyclient` on Windows, `~/.cache/tinyclient` elsewhere), so the network is asked once and later calls need none. A file on disk that turns out not to be Cisco's is taken away and fetched again. macOS and Linux send through OpenH264 as they always have; whatever arrives is decoded through OpenH264 on every desktop.
 
 The room forwards one picture: a share while someone else is sharing, otherwise one layer of another person's camera. The listener does not get their own picture. The client offers three video lines, the first two inactive until asked: a camera as two layers of one picture (640×360 and 320×180), one share fitted inside 1280×720, and one receive line for the picture the room forwards. Both the camera and the share run at 15 frames a second. Nothing plays until someone chooses Watch.
 

e2e/scenarios/openh264-download-retry/README.md

@@ -0,0 +1,13 @@
+# OpenH264 download retry
+
+Run `sh e2e/scenarios/openh264-download-retry/run.sh`. It needs no devices and no network:
+the scenario drives the tests of the download in `crates/mediad/src/picture/desktop.rs` —
+three tries with pauses of 1, 4 and 16 seconds, the words each kind of failure says (the
+host a VPN usually blocks, and "turn share or camera on again to retry"), the folder the
+codec is kept in so a restart finds it, and a cached file that is not Cisco's being taken
+away so it is fetched again.
+
+On the base the scenario fails: the tests do not exist, so cargo reports no matching tests.
+A download that is really blocked, on a real call, needs a network that drops
+`ciscobinary.openh264.org`; that is not something the lab can arrange, and this scenario
+does not claim it.

e2e/scenarios/openh264-download-retry/run.sh

@@ -0,0 +1,24 @@
+#!/bin/sh
+# The OpenH264 download's tests: three tries with a growing pause, words that say what failed
+# and how to try again, a cache folder a restart finds, and a broken cached file taken away.
+# No network and no devices: nothing here reaches Cisco.
+set -eu
+cd "$(dirname "$0")/../../.."
+log=$(mktemp)
+trap 'rm -f "$log"' EXIT HUP INT TERM
+run() {
+    name=$1
+    printf '%s\n' "openh264 download: $name"
+    if ! cargo test -p mediad --lib "$name" -- --exact --nocapture --test-threads=1 >"$log" 2>&1; then
+        cat "$log"
+        return 1
+    fi
+    cat "$log"
+    grep -Fq "running 1 test" "$log"
+    grep -Fq "test $name ... ok" "$log"
+}
+run picture::desktop::tests::the_download_is_tried_again_before_it_gives_up
+run picture::desktop::tests::the_words_say_what_failed_and_how_to_retry
+run picture::desktop::tests::the_codec_is_cached_where_a_restart_finds_it
+run picture::desktop::tests::a_broken_cached_file_is_taken_away
+printf '%s\n' 'PASS: a failed OpenH264 download is retried, says why and how to try again, and a good one is kept.'

e2e/scenarios/openh264-download-retry/scenario.json

@@ -0,0 +1,13 @@
+{
+  "id": "openh264-download-retry",
+  "title": "A failed OpenH264 download is tried again, says what failed and how to retry, and a good download is kept across starts",
+  "platforms": ["windows", "linux", "macos"],
+  "not_applicable": {
+    "android": "A phone encodes through its own platform encoder and never downloads OpenH264.",
+    "ios": "A phone encodes through its own platform encoder and never downloads OpenH264."
+  },
+  "needs": [],
+  "release": false,
+  "driver": "run.sh",
+  "proof_scope": "the retry count and pauses, the words a failure says, where the codec is cached, and a broken cached file being removed, unit-level; a blocked download on a real call needs a network that drops Cisco's host"
+}