Skip to content
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

Merged
merged 35 commits into from
Nov 4, 2024

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Oct 31, 2024

What

Tested this with videos containing b frames, both long and short.

image

(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:

  • test 4k 60fps video material
    • works smoothly in release, not at 60fps in debug builds on my mac because of rgb->rgba conversions on our side (see todos below)
  • test black & white (yuv400)
    • works but my video test material gave me: 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)
  • test yuv444/rgb video
    • worked, but the test 4.4s test video I used stops working starting at 3.2s, recovers fine though
      • Direct play browser: Firefox says the file is corrupt, Safari refuses to play. Chrome handles it
      • Rerun play browser: Same, but different errors
      • ... letting this issue slide for now 🤷

What's left to do (beyond general video bugs I logged while working on this) before calling this "done":

  • download ffmpeg for the user or at least tell users what to do when there's no ffmpeg (haven't checked even yet on the error message)
  • add version & ability check for ffmpeg?
  • consume 420p and friends directly to speed up processing
  • try to provoke frame dropping and see if we can detect & handle that - one of the shortcomings of the overall approach today is that we don't track this!

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!
  • If have noted any breaking changes to the log API in CHANGELOG.md and the migration guide

To run all checks from main, comment on the PR with @rerun-bot full-check.

Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM!

crates/store/re_video/src/decode/async_decoder_wrapper.rs Outdated Show resolved Hide resolved
crates/viewer/re_data_ui/src/video.rs Show resolved Hide resolved
crates/viewer/re_data_ui/src/video.rs Outdated Show resolved Hide resolved
// (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()
Copy link
Member

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.

Copy link
Member Author

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

crates/store/re_video/src/decode/ffmpeg.rs Outdated Show resolved Hide resolved
crates/store/re_video/src/decode/ffmpeg.rs Outdated Show resolved Hide resolved
crates/store/re_video/src/decode/ffmpeg.rs Show resolved Hide resolved
crates/store/re_video/src/decode/ffmpeg.rs Outdated Show resolved Hide resolved
crates/store/re_video/src/decode/ffmpeg.rs Outdated Show resolved Hide resolved
crates/store/re_video/src/decode/ffmpeg.rs Outdated Show resolved Hide resolved
Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM!

@emilk
Copy link
Member

emilk commented Oct 31, 2024

Testing this out I get

[2024-10-31T19:18:32Z WARN re_video::decode::ffmpeg] Video decode timestamps are expected to monotonically increase unless there was a decoder reset. This is probably a bug in Rerun.

test_video_h264.mp4

@Wumpf Wumpf merged commit b0cf325 into main Nov 4, 2024
35 of 37 checks passed
@Wumpf Wumpf deleted the andreas/ffmpeg-decoder-2 branch November 4, 2024 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants