Add warning for using an old version of ffmpeg, be accomodating for log quirks of older versions #8005
Conversation
Wumpf
added
exclude from changelog
| && minor < FFMPEG_MINIMUM_VERSION_MINOR) | ||
| { | ||
| re_log::warn_once!( | ||
| "FFmpeg version is {}. Only versions >= {FFMPEG_MINIMUM_VERSION_MAJOR}.{FFMPEG_MINIMUM_VERSION_MINOR} are officially supported", |
There was a problem hiding this comment.
We should make all error messages actionable
| "FFmpeg version is {}. Only versions >= {FFMPEG_MINIMUM_VERSION_MAJOR}.{FFMPEG_MINIMUM_VERSION_MINOR} are officially supported", | |
| "The found `ffmpeg` binary on your system has version {}. Rerun has only been tested with >= {FFMPEG_MINIMUM_VERSION_MAJOR}.{FFMPEG_MINIMUM_VERSION_MINOR}. Please update your `ffmpeg` version.", |
There was a problem hiding this comment.
Even better: include the link to the latest ffmpeg version, as provided by ffmpeg-sidecar!
There was a problem hiding this comment.
…and maybe we should just return an error instead
There was a problem hiding this comment.
Don't want to return an error since chances that things just work are quite high. But yeah let's log an error with url to make it more visible!
The can't-parse situation is a different story, granted
There was a problem hiding this comment.
changed my mind again after seeing that 7.1-essentials_build-www.gyan.dev is a version string we might get. This is too wild to make it any of this an error, stays a warning
But adding the download link
| re_log::warn_once!( | ||
| "Failed to parse FFmpeg version: {}", | ||
| ffmpeg_version.raw_log_message | ||
| ); |
There was a problem hiding this comment.
I think at this point we should return an error, so that we show the "Install ffmpeg" stuff in the selection panel, perhaps with a different message
|
Btw, it would be super-useful to send an analytics events when:
|
|
analytics even on ffmpeg missing might be a bit too spammy, no? We can't assume that a first time user has ffmpeg installed |
…og quirks of older versions
Wumpf
force-pushed
the
andreas/ffmpeg-version-check
branch
from
6d2c164 to
d4daf35
Compare
What
ffmpegCLI #7607Most replacing randomness with warnings. I haven't deep dived on which version will work but 5.1 went fine with my test data minus a bit of log spam which I fixed here.
I considered adding a check for coded availability, but this seems a bit cumbersome given that any off-the-shelf ffmpeg install comes with h264: the problem is that while we get a full string of compile options that ffmpeg was built with, we can't really infer the decoders from this since any given library may fulfill various roles. Instead we'd need to parse the output of
ffmpeg -codecs. We should eventually! But doesn't seem urgent right now.Checklist
mainbuild: rerun.io/viewernightlybuild: rerun.io/viewerCHANGELOG.mdand the migration guideTo run all checks from
main, comment on the PR with@rerun-bot full-check.