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

jbuf: refactor frame calculation #788

Merged
merged 4 commits into from
May 15, 2023
Merged

jbuf: refactor frame calculation #788

merged 4 commits into from
May 15, 2023

Conversation

sreimers
Copy link
Member

@sreimers sreimers commented May 5, 2023

A video frame is usually divided into smaller packets. Since the rtp marker bit is not very reliable (packet can be lost) or has a different meaning for audio codecs (talk spurt), frames should be detected by timestamp after sequence deduplication and ordering.

@sreimers sreimers force-pushed the jbuf_video branch 2 times, most recently from 900fd8a to a135ec9 Compare May 5, 2023 12:45
@sreimers sreimers changed the title jbuf: fix video frame calculation jbuf: frame calculation May 5, 2023
@sreimers sreimers changed the title jbuf: frame calculation jbuf: refactor frame calculation May 5, 2023
@@ -153,7 +154,7 @@ static void jbuf_destructor(void *data)
*
* @param jbp Pointer to returned jitter buffer
* @param min Minimum delay in [frames]
Copy link
Collaborator

@cspiel1 cspiel1 May 8, 2023

Choose a reason for hiding this comment

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

frames --> packets. Also here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, because there should be at least min frames (f->nf) in the buffer to make sure the video codec can use them. The max must be packets, because we can't pre-calculate how many video packets are one frame (this varies a lot by codec, image size, state...).

@cspiel1
Copy link
Collaborator

cspiel1 commented May 8, 2023

Is this still correct that jbuf_get "gets one frame from the jitter buffer"?

@sreimers
Copy link
Member Author

Is this still correct that jbuf_get "gets one frame from the jitter buffer"?

Fixed, is now renamed to one packet

@sreimers sreimers marked this pull request as ready for review May 14, 2023 16:16
sreimers added a commit to baresip/baresip that referenced this pull request May 14, 2023
@alfredh
Copy link
Contributor

alfredh commented May 15, 2023

I think this one should be ready to merge.

apart from that, if we want to be more pragmatic, we could make a separate PR only
for the renaming (packet/frame), which is a non-functional change ...

@sreimers
Copy link
Member Author

I rebased and cleaned up, so renaming is separate commit now. I will rebase merge to keep these commits.

@sreimers sreimers merged commit 948c6c1 into main May 15, 2023
@sreimers sreimers deleted the jbuf_video branch May 15, 2023 08:22
sreimers added a commit to baresip/baresip that referenced this pull request May 15, 2023
* config: add different options for audio and video jitter buffer

* fix audio.jbtype fallback

* update min/max config description

baresip/re#788
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.

3 participants