Skip to content

Conversation

@henkdegroot
Copy link
Contributor

@henkdegroot henkdegroot commented Jan 10, 2022

Short description of changes

Remove preferred text from "What Is This?" for Buffer Delay.

Context: Fixes an issue?

Fixes: #2222

Does this change need documentation? What needs to be documented and how?

Yes, minor text update in the Software Manual would be advised to keep the "64, 128 and 256" samples options in-line with the details in the GUI.

Status of this Pull Request

Complete.
The next round of translation should apply this change to the local languages as well.

What is missing until this pull request can be merged?

Nothing

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@@ -186,7 +186,7 @@ CClientSettingsDlg::CClientSettingsDlg ( CClient* pNCliP, CClientSettings* pNSet
"<br>" + tr ( "Three buffer sizes are supported" ) +
Copy link
Collaborator

Choose a reason for hiding this comment

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

... the change itself here is okay -- but the whole strSndCrdBufDelay text could probably do with rewriting...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apart from the 128 Samples text, it is now in-line with the web-site. The 128 Samples on the web-site includes the preferred text (which is matching with the text shown in the GUI).

Copy link
Collaborator

Choose a reason for hiding this comment

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

First up, the extra use of QString() could be done away with in several places as I think tr() will happily take .arg() directly.

Then there's the fact checks...

Three buffer sizes are supported

Hm. If the sound card doesn't allow the app to set the buffer size, the app happily copes with the sound card buffer size. So this isn't entirely true. One could go as far as saying "Wrong!". Indeed, it finishes by saying "(Jamulus) will still work with this setting". (Arguable it's explained that "supported" means "one of these three" and "unsupported" means "not one of these three".)

256 samples: Should only be used on very slow computers or with a slow internet connection.

Not really sure the sound card buffer size is going to help with a slow network connection, I'm afraid... It'll still either be fast enough or not fast enough. There are other settings that do affect network bandwidth but this doesn't... so scores another "Wrong!".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Lines 214 to 218 also look spurious...

Copy link
Collaborator

@pljones pljones Jan 10, 2022

Choose a reason for hiding this comment

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

My approximate understanding of the processing is...

If Jamulus is using 64 sample network buffers, it can read a 64 samples from the audio buffer each time they're ready, i.e. once per audio buffer input - reads once and sends a 64 sample audio buffer, reads twice for a 128 sample buffer, four times for a 256 sample buffer, and so on - and then sends them after each read.

If Jamulus is using 128 sample network buffers, it can read 128 samples from the audio buffer each time they're ready, i.e. once per audio buffer input - it needs to read two 64 audio sample buffers before sending, or reads once for a 128 sample buffer, or can read and send twice for 256 sample buffer, and so on.

Input and Output buffer sizes have to match for Audio and Network. Audio and Network buffers do not have to match. Network buffer size has to be 64 ("small") or 128 (not "small"). Audio buffer size isn't constrained.

Latency in this sytem comes from a combination of factors:

  1. The size of the Audio buffer: this should be as low as possible, obviously, but having a value lower than the size of the network buffer introduces negligible improvement and introduces CPU overhead and I/O.
  2. The size of the Network buffer: this is more significant as it affects the delay introduced by jitter buffering
  3. Jitter buffering: this is even more significant as it magnifies the network buffer delay
  4. Ping: this adds to the other factors but is outside Jamulus's control

The unit of "delay" here is the time span represented by a sample at 48KHz (or, rather 64 or 128 samples, or whatever).

Note that actual jitter is also outside Jamulus's control - the jitter buffers are there to alleviate the effect of jitter to a more acceptable level.

Network bandwidth is unlikely to be a problem for most Client systems - the highest data rate is well under 1Mbps (around 900Kbps at 64 samples, high quality stereo). Increasing the audio buffer size does drop the data rate - with 256 samples, low quality mono audio, it can go under 200Kbps. The largest impact on bandwidth comes from turning on small network buffers when using 64 sample audio buffers.

The bandwidth usage, however, does not directly affect the latency. (The network buffer size, of course, does.)

Copy link
Collaborator

@pljones pljones Jan 10, 2022

Choose a reason for hiding this comment

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

No - it shouldn't be in the "What's This?" (and scare people ;) ). What's there now is "good enough" - if still too long.

It should be something short and relevant, really... Hm... Maybe something like:

"When enabled, this control allows selecting the size of the buffer Jamulus uses for handling sound. For details on how the buffers affect latency (delay in when you hear sound), see the [wiki](link...). The values here are:

  • 2.67ms (64): this gives lower latency when 'small network buffer' is selected
  • 5.33ms (128): this gives lower CPU and network usage, suggested when 'small network buffer' is not selected
  • 10.67ms (256): this gives still lower CPU and network usage at the expense of a small increase in latency, suggested when for low bandwidth connections or slow computers.

When not enabled, Jamulus does not have control of the buffer size but similar considerations apply to those mentioned above. Set up your sound device appropriately."

which still feels too long...

Copy link
Collaborator

Choose a reason for hiding this comment

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

And another thing - separate improvement...
image
"Device" on the "Audio/Network Setup" tab really, really needs to say "Sound Device" (or Audio Device). Yes, "everybody knows" it's not the network device...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pljones, I did not notice this until now (when I was fixing a conflict which I merged incorrect yesterday). All should now be back in sync with master and this change has been applied as well. I used Audio Device, because of the TAB name referring to Audio as well.

Copy link
Member

Choose a reason for hiding this comment

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

10.67ms (256) […] slow computers

I don’t think it’s mainly about "Slow computers" from my experience it’s mainly about cheap audio devices…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don’t think it’s mainly about "Slow computers"

Hence why I removed both text "Slow computers" and "Slow internet" and now just mention that 256 should only be used when 64 or 128 is causing issues :).

@henkdegroot henkdegroot marked this pull request as draft January 10, 2022 19:51
@henkdegroot henkdegroot marked this pull request as ready for review January 10, 2022 21:09
@ann0see ann0see merged commit 2f7146b into jamulussoftware:master Jan 13, 2022
@henkdegroot henkdegroot deleted the text-update branch January 14, 2022 05:02
@pljones pljones added this to the Release 3.8.2 milestone Feb 13, 2022
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.

64 and 128 sample size is indicated to be preferred (shown in different text)

3 participants