-
Notifications
You must be signed in to change notification settings - Fork 368
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support H.264 on native via user installed ffmpeg executable #7962
Conversation
… linking to it anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM!
// (should be faster overall at no quality loss if the video is in this format). | ||
// Check `ffmpeg -pix_fmts` for full list. | ||
.rawvideo() // Output rgb24 on stdout. | ||
.spawn() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this call to spawn
block? i.e., will this cause a hickup on Windows if ffmpeg starts slowly?
If so, let's move it to the background thread. If not, let's add a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment on process starts being slow was entirely about the side of the new process :). But I'll make sure to give this another spin on Windows and check on this!
Moving it to background thread is a but cumbersome and of potentially dubious value because we need the process object & handle on the main thread potentially immediately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM!
Testing this out I get
test_video_h264.mp4 |
… for deprecated video formats
What
ffmpeg
CLI #7607Tested this with videos containing b frames, both long and short.
(note how PTS != DTS, indicating that there's a b-frame somewhere ;-))
Biggest risk overall with the approach taken is that we can't detect dropped frames properly
Left to do before landing:
re_video::decode::ffmpeg] /\//Users/andreas/dev/rerun-io/internal-test-assets/video/greyscale_360p_h264.mp4, codec: H.264 (avc1.64001E) decoder: [swscaler @ 0x3001d8000] [warning] deprecated pixel format used, make sure you did set range correctly
every time ffmpeg started, need to fix that to a log_once (if ffmpeg says it's deprecated then I figure we may report that 🤷, just don't spam)What's left to do (beyond general video bugs I logged while working on this) before calling this "done":
Checklist
main
build: rerun.io/viewernightly
build: rerun.io/viewerCHANGELOG.md
and the migration guideTo run all checks from
main
, comment on the PR with@rerun-bot full-check
.