From 3a71ea8794605bb24ddfe448e03a52e9cd4c1774 Mon Sep 17 00:00:00 2001 From: Andreas Reich Date: Fri, 8 Nov 2024 09:30:32 +0100 Subject: [PATCH] Ensure ffmpeg process has ended on decoder shutdown (#8042) ### What There's two critical issues leading up to this: * unlike our sleeping decoder thread whose shut down may be delayed, these processes put a lot of resource strain on the system * [zombie processes](https://en.wikipedia.org/wiki/Zombie_process) may stay up indefinitely until we `wait` for their result This unfortunately can reintroduce hickups on seeking backwards. This is more telling about our poor decode strategy for backward seeks than anything else. Something we'll have to tackle in the near future. ### Checklist * [x] I have read and agree to [Contributor Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and the [Code of Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md) * [x] I've included a screenshot or gif (if applicable) * [x] I have tested the web demo (if applicable): * Using examples from latest `main` build: [rerun.io/viewer](https://rerun.io/viewer/pr/8042?manifest_url=https://app.rerun.io/version/main/examples_manifest.json) * Using full set of examples from `nightly` build: [rerun.io/viewer](https://rerun.io/viewer/pr/8042?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json) * [x] The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG * [x] If applicable, add a new check to the [release checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)! * [x] If have noted any breaking changes to the log API in `CHANGELOG.md` and the migration guide - [PR Build Summary](https://build.rerun.io/pr/8042) - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html) To run all checks from `main`, comment on the PR with `@rerun-bot full-check`. --- .../store/re_video/src/decode/ffmpeg_h264/ffmpeg.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/crates/store/re_video/src/decode/ffmpeg_h264/ffmpeg.rs b/crates/store/re_video/src/decode/ffmpeg_h264/ffmpeg.rs index d4ef2acd60c6..743936de625c 100644 --- a/crates/store/re_video/src/decode/ffmpeg_h264/ffmpeg.rs +++ b/crates/store/re_video/src/decode/ffmpeg_h264/ffmpeg.rs @@ -317,8 +317,18 @@ impl Drop for FfmpegProcessAndListener { .store(true, std::sync::atomic::Ordering::Release); // Kill the ffmpeg process itself. + // It's important that we wait for it to finish, otherwise the process may enter a zombie state, see https://en.wikipedia.org/wiki/Zombie_process. + // Also, a nice side effect of wait is that it ensures that stdin is closed. // This should wake up the listen thread if it is sleeping, but that may take a while. - self.ffmpeg.kill().ok(); + { + let kill_result = self.ffmpeg.kill(); + let wait_result = self.ffmpeg.wait(); + re_log::debug!( + "FFmpeg kill result: {:?}, wait result: {:?}", + kill_result, + wait_result + ); + } // Unfortunately, even with the above measures, it can still happen that the listen threads take occasionally 100ms and more to shut down. // (very much depending on the system & OS, typical times may be low with large outliers)