Repository navigation
Retry the OpenH264 download, and say what failed and how to try again - #252
Conversation
…ry again A screen share or camera behind a VPN that blocks Cisco's binary host failed the one download attempt and left the call with a generic sentence and no way back except guessing. The download is now tried three times with a growing pause (1s, 4s, 16s), each attempt capped at 20 seconds, and a stalled body read is cut off too, so a VPN that takes the connection but not the bytes cannot hold the fetch thread forever. When the tries are out, the call dock says what went wrong and how to retry: '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.' Turning share or camera on starts a new download (retry_codec, as before). Disk failures and a download thread that could not start say their own words instead of the network ones. A file cached from an earlier successful download is still reused (%LOCALAPPDATA\\tinyclient, ~/.cache/tinyclient), so the network is asked once; a cached file that is not Cisco's is taken away and fetched again. The Media Foundation hardware encoder that would retire the download is sketched in a doc comment, not built.
ureq's Request has no timeout_read; that knob lives on the agent. The overall 20-second deadline set per try is carried into the response reader (DeadlineStream), which is exactly the cut-off the stalled-read protection wanted, so the call is dropped and the comment now says so.
…bout a hardware encoder that has since shipped Co-Authored-By: Claude <noreply@anthropic.com>
Tiny Code Review✅ No blocking findings — see the review · run |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Tiny Code Review
✅ No blocking findings.
The retry loop, differentiated failure messages, size cap, and cache-path refactor are sound and the work stays on a background thread, so nothing blocks the call. The ureq timeout genuinely covers the body read, the temp-file-then-rename keeps writes whole-or-nothing, and the broken-cache removal is exercised by a test. No defects gate the merge.
Residual risk
- The docs line for the Windows cache path contains a literal tab:
%LOCALAPPDATA%<TAB>inyclientinstead of%LOCALAPPDATA%\tinyclient— a doc typo only, worth a follow-up edit. - A SAVE_FAILED (disk full, permissions) is retried with the full 1/4/16s network pauses even though a pause cannot help a disk error; only a small time cost, not a defect.
- A download that is truncated exactly at the 64 MiB cap is not reported as an error by
take; the decompressor or hash check would catch a wrong file, but a larger-than-expected upstream change would surface as a generic DOWNLOAD_FAILED. - A really blocked network takes roughly 80 seconds of background retries before the dock says why; this is stated in the PR and acceptable.
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Tiny Code Review
🟡 1 finding (0 blocking).
The retry loop, bounded read, distinct failure words, and atomic write are sound and the download stays on a background thread. One real defect: the new cache-path unit test hard-codes a Windows path with backslashes and will fail on Linux and macOS, which are two of the three platforms the scenario claims.
Residual risk
- A blocked network now takes up to about 75 seconds to report, all on the background
openh264thread; acceptable but worth knowing. a_broken_cached_file_is_taken_awayuses a fixed file name in the shared temp directory; concurrent runs on a multi-user machine could interfere, and the file is only removed viaopen_cached.- The scenario proves only unit-level behavior; a genuinely blocked download on a real call is not exercised.
| 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\\tinyclient")); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…n every desktop Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Tiny Code Review
✅ No blocking findings.
The change is well scoped: a background-thread retry loop with capped, growing pauses, distinct user-facing messages, a 64 MiB read cap, and atomic write-then-rename all match the repository's patterns, and the cache paths are unchanged so nothing breaks compatibility. The sleeping and network work stay on the dedicated openh264 thread, off the realtime paths. I found no blocker or major defects introduced by the diff.
Residual risk
- A disk-full or read-only disk (SAVE_FAILED) is retried three times with ~21 s of pauses, wasting about a minute before the same failure is reported; skipping retries for write errors would report it sooner.
- A download larger than the 64 MiB read cap would be silently truncated and then fail the bzip2 decode with the generic DOWNLOAD_FAILED words; Cisco's actual library is far smaller, so this is theoretical.
- The unit tests assert on constants and pure functions; the retry loop's control flow itself (that download_with_retries really loops and stops) is not exercised, only its parameters.
What changed and why
On a machine without a hardware H.264 encoder, the first share or camera downloads OpenH264 from Cisco. One failed try ended it, and the call dock only said the download failed. The download is now tried three times with pauses of 1, 4 and 16 seconds, each try cut off at 20 seconds. When it still fails, the dock says the likely reason (a VPN or firewall blocking
ciscobinary.openh264.org) and how to retry: turn the share or camera on again. A disk that will not take the file gets its own words. A cached file that is not Cisco's is removed and fetched again.These two commits were pushed as
feat/openh264-downloadon 10 November and never opened as a pull request. They are rebased onto main here (after #235, so OpenH264 is the fallback, not the only encoder), with the lab scenario added.Type
Platforms
Proof
Scenario: e2e/scenarios/openh264-download-retry
Lab run:
The scenario runs the download's unit tests: the retry count and pauses, the words each failure says, where the codec is cached, and a broken cached file being taken away. It reaches no network. A download that is really blocked on a real call is not proved here.
Compatibility
Client only. No protocol change. The cache folder is the same one as before.
Risk and rollback
A blocked network now takes about a minute and a half at worst to say so instead of 20 seconds, on a background thread; the call stays voice meanwhile, as before. Revert the PR.
Release note
A failed OpenH264 download is retried, and the call says why it failed and how to try again.
🤖 Generated with Claude Code