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.

Sign up for GitHub

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

Jump to bottom

jbuf frame completeness #2754

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

jbuf frame completeness #2754

wants to merge 18 commits into from

Conversation

cspiel1
Copy link
Collaborator

@cspiel1 cspiel1 commented Oct 6, 2023 •
edited

This PR improves jbuf in order to

  • ensure frame completeness for video and correct order of frames,
  • reduce the buffered frames with return EAGAIN if it is possible, but not too early.
  • Correct order of frames/packets for audio streams.
  • Does not increase O-notation complexity for jbuf_put() nor jbuf_get().
  • Makes adaptive mode obsolete and removes it.

This PR still is a draft and will be compared to #2757.

@juha-h
Copy link
Collaborator

juha-h commented Oct 6, 2023

PR description reads:

config: remove jbuf mode adaptive

but docs/examples/config still has it for audio and video.

Have I misunderstood something?

@cspiel1
Copy link
Collaborator Author

cspiel1 commented Oct 6, 2023

Plots for video

  • without packet loss
    jbuf-video
  • with packet loss
    jbuf-video-loss

@cspiel1
Copy link
Collaborator Author

cspiel1 commented Oct 6, 2023 •
edited

Plots for audio

The second plot of each set is the ajb plot of the adaptive audio buffer. See https://github.com/baresip/re/blob/main/rem/aubuf/ajb.c !

Underruns in jbuf lead to underruns in the aubuf. Sometimes this can't be avoided, e.g. if there is a packet lost.

  • min size 1 without packet loss
    jbuf-audio
    ajb
  • min size 1 with packet loss
    jbuf-audio-loss
    ajb-loss
  • min size 0 without packet loss
    jbuf-audio-zero
    ajb-zero
  • min size 0 with packet loss
    jbuf-audio-zero-loss
    ajb-zero-loss

@sreimers
Copy link
Member

sreimers commented Oct 6, 2023 •
edited

Looks like for audio, the buffers does not play well together, if I see correctly we have 2-3 packets in jbuf (20-60ms) + up to 80ms within aubuf and this does not compensate a 50ms jitter without packet loss (leads to real aubuf underruns)?

@cspiel1
Copy link
Collaborator Author

cspiel1 commented Oct 9, 2023 •
edited

Looking at the ajb plots auf aubuf it looks like that the computed jitter does not increase fast enough to the expected value of 50ms. Ideally we would expect that there only underruns at the beginning of the simulated jitter. For these plots I used this jitter simulation:

sudo tc qdisc add dev ifb1 root netem delay 0ms 50ms loss 0.7%

Note that ajb is only active if configured with audio_buffer_mode adaptive.

This PR should touch only jbuf and

  • ensure frame completeness for video and correct order of frames.
  • Reduce the buffered frames with return EAGAIN if it is possible, but not too early.

The jbuf plots show that underruns because of out-of-order packets occur only at the start of the jitter. Additionally if there is a real packet loss. This is all we could expect from jbuf how it is used currently.

After this we could think about moving ajb from aubuf to jbuf and use a timer for jbuf_get() and the decoding.

@cspiel1 cspiel1 marked this pull request as ready for review October 9, 2023 09:28
@sreimers sreimers mentioned this pull request Oct 9, 2023
Draft
6 tasks
@alfredh alfredh added this to the v3.7.0 milestone Oct 12, 2023
@alfredh alfredh removed this from the v3.7.0 milestone Oct 23, 2023
@alfredh
Copy link
Collaborator

alfredh commented Oct 26, 2023

please resolve the conflicts ...

@cspiel1 cspiel1 marked this pull request as draft October 26, 2023 17:15
@cspiel1
Copy link
Collaborator Author

cspiel1 commented Oct 26, 2023

I will resolve the conflicts tomorrow.
@sreimers and me plan to compare this with: #2757

@alfredh
Copy link
Collaborator

alfredh commented Dec 2, 2023

please update the description with more details.

Can you please also rebase this on top of git HEAD ?

@cspiel1
config: remove jbuf mode adaptive
9f2a2f7
@cspiel1
config,stream: configure jbuf packet pool size for video
5b68829
@cspiel1
tools: jbuf plots - update plot data fields
927c73a
@cspiel1
tools: jbuf plots - rename underflow to waiting
076c4a4
@cspiel1
tools: jbuf plots - use re_trace.json
0e3e065
@cspiel1
tools: jbuf plots - add debug events x,y
0640b2d
@cspiel1
tools: jbuf - use jitter simulation of parent dir
035ddb6
@cspiel1
tools: jbuf plots - add call.sh
8b0e014 
- a script that performs a single call
- generate both jbuf and aubuf ajb plots
@cspiel1
conf: jbuf type value on
5fbe032
@cspiel1
jbuf: replace adaptive mode by frame completeness check
abf8fac 
A frame is a sequence of RTP packets with equal timestamp. Now `jbuf_get()`
does not pass packets before the oldest frame is complete.
@cspiel1
test: jbuf update first test
6dc3487
@cspiel1
jbuf: replace shrink timer by wait frame counter
85d546a
@cspiel1
jbuf: better err handling for jbuf_resize()
2806fae
@cspiel1
tools: activate packet loss for jitter simulation
ebfc771
@cspiel1
jbuf: audio fixes
cffc19c
@cspiel1
jbuf: support also min size zero
f648bc4
@cspiel1
config: remove jitter buffer type adaptive
710f2f8
@cspiel1
test: fix jbuf tests
a6e6074 
`jbuf_get()` now holds back packets/frames until a new frame was put into jbuf.
It returns EAGAIN in order to reduce buffered frames only one by one and
slowly.
@alfredh alfredh added this to the v3.9.0 milestone Dec 22, 2023
@alfredh alfredh removed this from the v3.9.0 milestone Jan 8, 2024
alfredh
alfredh reviewed Mar 24, 2024
View reviewed changes
src/conf.c
@@ -337,7 +337,7 @@ enum jbuf_type conf_get_jbuf_type(const struct pl *pl)
{
if (0 == pl_strcasecmp(pl, "off")) return JBUF_OFF;
if (0 == pl_strcasecmp(pl, "fixed")) return JBUF_FIXED;
if (0 == pl_strcasecmp(pl, "adaptive")) return JBUF_ADAPTIVE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps add a warning about the config option has been removed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alfredh alfredh alfredh left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@cspiel1 @juha-h @sreimers @alfredh