Skip to content

Commit

Permalink
Ensure ffmpeg process has ended on decoder shutdown (#8042)
Browse files Browse the repository at this point in the history
### 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`.
  • Loading branch information
Wumpf authored Nov 8, 2024
1 parent a834b77 commit 3a71ea8
Showing 1 changed file with 11 additions and 1 deletion.
12 changes: 11 additions & 1 deletion crates/store/re_video/src/decode/ffmpeg_h264/ffmpeg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 3a71ea8

Please sign in to comment.