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

Passthrough yuv format for ffmpeg-h264 decoder when possible #8002

Merged
merged 13 commits into from
Nov 6, 2024

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Nov 5, 2024

What

Recognizing ahead of time what format a piece of encoded data is unfortunately not that easy. To accomplish that I fell into the rabbit hole of parsing the Sequence Parameter Set (SPS). Which will hopefully be useful for other things in the future!

Other than that, it turned out that determining the color space & color range are again different cans of worms, so I'm being now a lot more explicit with ffmpeg on what we expect.
Which.. is yet another rabbit hole! Pretty much no matter what I tried I couldn't replicate exact colors some sample videos compared to VLC (unlike we did with AV1 where we have a lot more solid grasp on all these things and get satisfying parity!). But also here different video players seem to do slightly different things, so I decided to not follow this thread further for the moment as this whole affair already cost me a lot more time that I wanted to invest originally.

Partially disabled unfortunately since we need this fix:

image

(note that this uses 422, the video source material is 420 though)

Fixes slow 4k video handling:

Before:
image

After:
image

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.

@Wumpf Wumpf added 🚀 performance Optimization, memory use, etc exclude from changelog PRs with this won't show up in CHANGELOG.md 🎞️ video labels Nov 5, 2024
@emilk
Copy link
Member

emilk commented Nov 6, 2024

We have the fix 🚀 https://github.com/nathanbabcock/ffmpeg-sidecar/releases/tag/v2.0.2

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.

Nice!

Cargo.toml Outdated Show resolved Hide resolved
crates/store/re_video/src/decode/ffmpeg.rs Show resolved Hide resolved
if sps_units.next().is_some() {
// This is rather strange. Must mean that some pictures refer to one SPS and some to another!
// We don't know what to do with this.
re_log::trace_once!("Found more than one sequence parameter set (SPS) in the AVCC box of {debug_name}.");
Copy link
Member

Choose a reason for hiding this comment

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

I suggest we use warn in debug builds. We could add some helper for that in re_log

Copy link
Member Author

Choose a reason for hiding this comment

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

after refactoring the whole thing to a method and - sparked by that - modularizing stuff more, I decided to just always warn. I'd rather have people complain about this than not finding these issues in our understanding!

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/h264_sps.rs Outdated Show resolved Hide resolved
crates/store/re_video/src/decode/h264_sps.rs Outdated Show resolved Hide resolved
crates/store/re_video/src/decode/h264_sps.rs Outdated Show resolved Hide resolved
/// Semantics are defined in [ITU-T H.264 (04/2017)](http://wikil.lwwhome.cn:28080/wp-content/uploads/2018/08/T-REC-H.264-201704%E8%8B%B1%E6%96%87.pdf)
#[derive(Debug)]
#[allow(dead_code)]
pub struct H264Sps {
Copy link
Member

Choose a reason for hiding this comment

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

I assume this appears in all H.264 stream, regardless of container format? i.e. that is why it belongs in re_video, and not re_mp4?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, the SPS is part of the h264 spec (as linked in the comment). So this will still be useful when streaming in data as we'll want to pass through the AVCC box (which typically contains the SPS among other things). At least I think at this point that we want to insist on that rather than consuming AnnexB (I recently read up some more on how this works in WebCodecs which btw. is a great blueprint for our mux-free/streaming api ;-) [.........])

@Wumpf Wumpf force-pushed the andreas/h264-yuv-passthrough branch from dc59d9e to 383e6c6 Compare November 6, 2024 14:51
@Wumpf Wumpf merged commit 4ef1be8 into main Nov 6, 2024
36 checks passed
@Wumpf Wumpf deleted the andreas/h264-yuv-passthrough branch November 6, 2024 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exclude from changelog PRs with this won't show up in CHANGELOG.md 🚀 performance Optimization, memory use, etc 🎞️ video
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants