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

add dts extractor #61 #81

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

add dts extractor #61 #81

wants to merge 4 commits into from

Conversation

Curid
Copy link
Contributor

@Curid Curid commented Mar 26, 2023

This is a rust port of the gortsplib dts extractor. I don't really understand how it works, but my test stream doesn't stutter anymore.

I know you're busy, I don't expect this to be reviewed anytime soon.

@scottlamb
Copy link
Owner

scottlamb commented Aug 3, 2023

First, thank you!

I don't own any cameras that produce B frames, so...

and...

I don't really understand how it works, but my test stream doesn't stutter anymore.

I don't really either and need to make another pass through to develop that understanding and compare this technique with gstreamer's h264timestamper [edit: code here]. Haven't looked closely enough yet to see if gstreamer and rtsp-simple-server (now called mediamtx apparently) are doing the same thing (and, if not, which is better). But I made one pass through and left some comments anyway.

src/lib.rs Outdated Show resolved Hide resolved
src/codec/h264.rs Outdated Show resolved Hide resolved
src/codec/h264.rs Outdated Show resolved Hide resolved
src/codec/h264.rs Outdated Show resolved Hide resolved
src/codec/h264.rs Outdated Show resolved Hide resolved
r.read_u32((log2_max_frame_num_minus4 + 4).into(), "frame_num")
.map_err(DtsExtractorError::BitReader)?;

if let FrameMbsFlags::Fields {
Copy link
Owner

Choose a reason for hiding this comment

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

  • More specific comment: If let Enum::VariantA { .. } = Enum::VariantB is dead code. Looks like the right side here should come from a field in the sps; maybe plumb it through as a parameter?
  • Broader comment: can we use h264_reader::nal::slice::SliceHeader instead of this function's parsing logic? might keep things more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SliceHeader::from_bits() takes SPS and PPS as arguments, I'm not sure parsing PPS in parse_sps_and_pps and plumbing it in would be more readable.

src/codec/h264/dts_extractor.rs Outdated Show resolved Hide resolved
@Curid
Copy link
Contributor Author

Curid commented Aug 4, 2023

The gstreamer implementation seems to add latency. I'm not sure if it's just some initial delay like with the MediaMTX implementation but otherwise that's a problem.

In order to determine the DTS of each frame, this element may need to hold back a few frames in case the codec data indicates that frame reordering is allowed for the given stream. That means this element may introduce additional latency for the DTS decision.

// Allows to extract DTS from PTS.
#[derive(Debug)]
pub struct DtsExtractor {
prev_dts_filled: bool,
Copy link
Owner

Choose a reason for hiding this comment

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

More idiomatic to use prev_dts: Option<i64> rather than the separate field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, it seems like extract_inner() relies on the prev_dts zero value.

src/codec/h264/dts_extractor.rs Outdated Show resolved Hide resolved
src/codec/h264.rs Outdated Show resolved Hide resolved
@Curid
Copy link
Contributor Author

Curid commented May 25, 2024

Found an explanation for how this algorithm works: https://github.com/bluenviron/mediamtx/issues/1002#issuecomment-2115930927

@Curid
Copy link
Contributor Author

Curid commented Jul 23, 2024

Would it be better to first add the dts extractor as separate module that people can use manually before integrating it into the Depacketizer?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants