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

Make mp4 parsing **a lot** faster & tremendously lower memory overhead #7860

Merged
merged 13 commits into from
Oct 22, 2024

Conversation

jprochazk
Copy link
Member

@jprochazk jprochazk commented Oct 22, 2024

What

Some rough numbers:

main@de4c389   297.43   µs
this branch      3.8957 µs

Measured on Big_Buck_Bunny_1080_10s_av1.mp4 from our test assets (downloaded via tests/assets/download_test_assets.py)

Total Rerun memory usage for a 297 MiB video (the entire Sintel movie):

main@de4c389   573 MiB
this branch    325 MiB

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.

@jprochazk
Copy link
Member Author

We still copy each sample when we enqueue a chunk, which feels like it should not be necessary. It would be nice to have direct access to the video data arrow buffer in decoders, so that we can index into it there, but I wasn't 100% sure how to make that work here so I left it out.

@Wumpf Wumpf self-requested a review October 22, 2024 12:30
@Wumpf Wumpf changed the title Optimize mp4 parsing Make mp4 parsing faster & reduce memory overhead Oct 22, 2024
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

similar to the commentary on the parent PR I'm super happy about this being so low-hanging, but also like there the distinction of "what data is this" needs more love. In partiulcar VideoData is already a type and now we're passing a lot of video_data which is not related to it.
Ideally we'd use some reference counted object from the guts of Blob, but I haven't looked into whether that's viable (we really don't want to end up passing around raw arrow buffers, infecting everything with that dependency)

If we advertise this in the changelog, we should have before after perf & memory numbers. Just give it a quick spin with one of the larger videos on our internal video repo, thank you! :)

crates/store/re_video/examples/frames.rs Outdated Show resolved Hide resolved
crates/store/re_video/src/demux/mod.rs Show resolved Hide resolved
crates/viewer/re_data_ui/src/blob.rs Outdated Show resolved Hide resolved
@jprochazk
Copy link
Member Author

(trying to measure this by adding a benchmark)

@jprochazk
Copy link
Member Author

jprochazk commented Oct 22, 2024

Some rough numbers:

main@de4c389   297.43   µs
this branch      3.8957 µs

Measured on Big_Buck_Bunny_1080_10s_av1.mp4 from our test assets (downloaded via tests/assets/download_test_assets.py)

Total Rerun memory usage for a 297 MiB video (the entire Sintel movie):

main@de4c389   573 MiB
this branch    325 MiB

@jprochazk
Copy link
Member Author

jprochazk commented Oct 22, 2024

Running a nightly build to check if the bench works: https://github.com/rerun-io/rerun/actions/runs/11461677788 (it did not)

@jprochazk
Copy link
Member Author

jprochazk commented Oct 22, 2024

Bench was missing nasm for the build, maybe it works now: https://github.com/rerun-io/rerun/actions/runs/11461912756/job/31892063198 - will cancel it if the benchmark succeeds - it downloaded the test assets and successfully compiled, good enough

@jprochazk jprochazk requested a review from Wumpf October 22, 2024 14:08
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

awesome!

crates/store/re_video/benches/video_load_bench.rs Outdated Show resolved Hide resolved
@Wumpf Wumpf changed the title Make mp4 parsing faster & reduce memory overhead Make mp4 parsing a lot faster & reduce memory overhead Oct 22, 2024
@Wumpf Wumpf changed the title Make mp4 parsing a lot faster & reduce memory overhead Make mp4 parsing **a lot** faster & tremendously lower memory overhead Oct 22, 2024
@jprochazk jprochazk merged commit 613a35b into main Oct 22, 2024
27 of 28 checks passed
@jprochazk jprochazk deleted the jan/zerocopy-mp4-read branch October 22, 2024 16:13
.unwrap()
.parent()
.ancestors()
.nth(3)
Copy link
Member

Choose a reason for hiding this comment

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

🤯

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