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

Estimate Overall Delay time calculation too optimistic? #2979

Open
KonFey opened this issue Dec 25, 2022 · 3 comments
Open

Estimate Overall Delay time calculation too optimistic? #2979

KonFey opened this issue Dec 25, 2022 · 3 comments
Labels
feature request Feature request

Comments

@KonFey
Copy link

KonFey commented Dec 25, 2022

Describe the bug
This is a very minor thing I guess, but to my understanding the effect of buffering is not shown correcly in the Jamulus main window / not calculated correctly in
CClient::EstimatedOverallDelay. It uses a factor of 0.7 for local and remote buffers - with the comment that the buffers usually a bit larger than required. That may be true, and the achievable delay would be less if the buffers were set correctly.

But I aussume that the display of delay should show the delay currently experienced, not the delay potentially achievable with better buffer setting.
Or do I miss something?

To Reproduce

No other measurement is available for the real experienced delay, so the value shown may just be off / too low compared to real experienced delay.

Expected behavior

My understanding of the buffer implementation is that it evens out network delay and jitter, and adds a fixed delay, so the delay of e.g. a buffer size of 4 is 4 times the block duration.
so the code should read

const float fTotalJitterBufferDelayMs = fSystemBlockDurationMs * ( GetSockBufNumFrames() + GetServerSockBufNumFrames() );

instead of
const float fTotalJitterBufferDelayMs = fSystemBlockDurationMs * ( GetSockBufNumFrames() + GetServerSockBufNumFrames() ) * 0.7f;

Operating system

any

Version of Jamulus
3.9.1

Additional context

I am prototyping a statisitics console on connection quality that should help to monitor long time quality of connections to the server. So I read a lot of jamulus source code and try to figure out the statistics calculations currently used. This when I encountered this calculation that I do not understand.

@KonFey KonFey added the bug Something isn't working label Dec 25, 2022
@ann0see
Copy link
Member

ann0see commented Dec 25, 2022

Hi @KonFey

Thanks for reporting.
Although I can't say much about the calculation of the delay, I also think that the delay is not fully accurate (and can never be as the hardware delay needs to be added too). Volker or @softins might know more about this.

@ann0see ann0see added the feature request Feature request label Dec 25, 2022
@ann0see ann0see added this to Triage in Tracking (old) via automation Dec 25, 2022
@ann0see ann0see removed the bug Something isn't working label Dec 25, 2022
@hoffie
Copy link
Member

hoffie commented Dec 29, 2022

Can't say anything specific offhand either, but --showanalyzerconsole (a hidden command line flag) might help provide some additional insights?

@softins
Copy link
Member

softins commented Jan 2, 2023

Volker or @softins might know more about this.

It's not an area of code I have ever studied, but the OP's point sounds valid. I guess some experimentation might be worthwhile, while running Wireshark on the client machine to capture traffic in both directions. Connected to a remote server with no other traffic, one could try sending, say a sound with a hard attack, e.g. a piano note, while observing the displayed overall delay. Then subsequently analyse the Wireshark capture to determine the actual delay between the outgoing attack edge and that in the returned audio. Maybe repeating the exercise at various manual jitter buffer settings. That should indicate whether the 0.7f is required or not, or maybe needs a more accurate formula.

I might have the time to try the above later this week, but if anyone else is able to as well, that would be great.

@pljones pljones removed this from Triage in Tracking (old) Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Feature request
Projects
Status: Triage
Development

No branches or pull requests

4 participants