Rawaudio dev

[dingodoppelt]

Add a new "raw" audio quality setting

This PR adds uncompressed audio ("raw") to the quality settings so there is no Opus compression along the way
Discussion in #3654

This feature improves latency as well. I gained 2ms by using uncompressed audio while having a better audio quality.

CHANGELOG: Add uncompressed audio transmission - dedicated to the memory of Hans Petter Selasky (1982 - 2023)

Does this change need documentation? What needs to be documented and how?
Corresponding PR in jamulussoftware/jamuluswebsite #1133

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

[ann0see]

I'd prefer not to check for the Jamulus version number but rather based on capabilities - we don't have 4.0.0 out yet and it might break during the dev process.

[dingodoppelt]

I'd prefer not to check for the Jamulus version number but rather based on capabilities - we don't have 4.0.0 out yet and it might break during the dev process.

I wanted to reuse information already available as much as possible so I just added the code where there were version checks already implemented. (For sequence number and pan feature)
Capabilities would be nice but also would require more changes to client, channel, server and protocol which I don't really have an idea on how to make that backwards compatible. We should rather replace all version checks with some capabilities struct that client and server can agree upon so everything lands in one place. I just don't feel like the right person to take on that challenge and rather pursue my hacky approach, as long as it works for everybody.
The version check with 4.0.0 could be replaced by a point release 3.11.1 and would work right away.

[ann0see]

Tested it and yes, the noise would be unacceptable. What is our fallback if max is selected but the server doesn't support it?

[dingodoppelt]

Tested it and yes, the noise would be unacceptable. What is our fallback if max is selected but the server doesn't support it?

I just noticed that if you connect to a server with Max selected you get the noise unless you switch audio quality again while connected. The server code is fine and doesn't need changes, I misplaced the check for my introduced bRawAudioSupported in the client code. I'll have a closer look
Edit: Funny, the noise doesn't happen on legacy servers, only on rawaudio :D

[dingodoppelt]

Adding a slot to the client to reinit when it receives the server version seems to have fixed the noise issue.

[dingodoppelt]

We still get crashes on windows, especially when using more coplex setups including audio routing software. Linux, Mac and android builds work fine so far. Sounds great but still needs more testing and fixes

[dingodoppelt]

The last commits fixed the crash on windows and make the client fall back to opus reliably. This is now ready to be tested thoroughly.

[dingodoppelt]

A buffersize of 256 on Max quality setting gives garbled audio and the packet sizes seem wrong and contain blocks of zeroes. Only that particular setting is affected. Opus still works

[softins]

A buffersize of 256 on Max quality setting gives garbled audio and the packet sizes seem wrong and contain blocks of zeroes. Only that particular setting is affected. Opus still works

I plan to try out this enhancement over the next few days. I've had a look through the diffs so far. Could you specify exactly the steps to produce this error?

[dingodoppelt]

A buffersize of 256 on Max quality setting gives garbled audio and the packet sizes seem wrong and contain blocks of zeroes. Only that particular setting is affected. Opus still works

I plan to try out this enhancement over the next few days. I've had a look through the diffs so far. Could you specify exactly the steps to produce this error?

This is reproducible with a buffersize of 256 samples only. The packets should be well below MTU and show a length of 1026 in wireshark.
This is may be related to the use of the conversion buffer or the iSndCrdFramSizeFactor not being used in packet size calculation.

Edit: The packets become bigger than the MTU allows for on 256 samples buffersize and get fragmented once I corrected the calculation of the packet sizes. Does this mean we need to disable raw audio for buffersizes of 256 or is there some mechanism to receive fragmented packets?

[softins]

I've just tried a build of rawaudio-dev here, between two separate hosts: server on a pi, client on a PC. It doesn't seem to be a MTU or fragmentation issue. The UDP packets are only 1068 bytes in size, and not fragmented.

Using a buffer size of 10.67ms (256) results in each packet containing two frames of audio, each with its own sequence number. In that setting, I was seeing one packet every 10.67ms coming from the Windows client, but still one packet every 5.33ms coming back from the server. They alternated between having zeros in the first frame and zeros in the second frame. So it could possibly be some issue in server.cpp that doesn't exist in client.cpp

Note that the client will encode according to the settings in the Client Settings dialog, but the server will encode according to the information in received in the NETW_TRANSPORT_PROPS message it received from the client.

Talking of which, the codec field in the NETW_TRANSPORT_PROPS message should specify a different value for RAW, rather than still saying OPUS, like this:

jamulus/src/util.h

Lines 484 to 492 in 849e823

	// Audio compression type enum -------------------------------------------------
	enum EAudComprType
	{
	// used for protocol -> enum values must be fixed!
	CT_NONE = 0,
	CT_CELT = 1,
	CT_OPUS = 2,
	CT_OPUS64 = 3 // using OPUS with 64 samples frame size
	};

So when sending props for raw encoding, it should either use CT_NONE or define a new CT_RAW=4.

[dingodoppelt]

I've just tried a build of rawaudio-dev here, between two separate hosts: server on a pi, client on a PC. It doesn't seem to be a MTU or fragmentation issue. The UDP packets are only 1068 bytes in size, and not fragmented.

This build is not taking into account iSndCrdFrameSizeFactor. From what I understood it should be mostly 1 and my code seems to only work when it is. iCeltNumCodedBytes should be multiplied by iSndCrdFrameSizeFactor. On 256 samples buffer size it will create packets that get fragmented. Wireshark shows the fragmentation. Should I push these changes for you to test? I might have gotten something fundamentally wrong here, but I'd say the problem is mainly in the client since the server happily plays back everything you throw at it.

[softins]

Ah, so the issue is that the client is not sending enough data to satisfy the server, and the server is therefore adding in packets of zeros to maintain the data rate.

Fragmentation should not be an issue, at least with IPv4, as fragmentation and re-assembly happens transparently at the IP layer. In fact, I don't think it will occur anyway, as the traffic from the server is not fragmented. We should just get packets from the client at 5.33ms instead of 10.67ms.

In fact, I've been doing some tests with Wireshark of all the various data rates, qualities and mono/stereo, and it seems that the packet interval is normally half the buffer time specified in the Client Settings. Except when "Small buffers" is not checked, and then 2.67 (64) is exactly the same as 5.33 (128).

[softins]

This build is not taking into account iSndCrdFrameSizeFactor. From what I understood it should be mostly 1 and my code seems to only work when it is. iCeltNumCodedBytes should be multiplied by iSndCrdFrameSizeFactor. On 256 samples buffer size it will create packets that get fragmented. Wireshark shows the fragmentation. Should I push these changes for you to test?

Yes please - I'm building directly from your rawaudio-dev branch.

[softins]

I think in client.cpp around line 1486, you need also to do a similar loop as a few lines above:

for ( i = 0, j = 0; i < iSndCrdFrameSizeFactor; i++, j += iNumAudioChannels * iOPUSFrameSizeSamples )

I don't have any more time today to try it...

[softins]

I think it would be better to recognise the audio frame by a sentinel byte. Protocol frames begin with 00 00 and must have a good checksum. Otherwise they are considered to be audio. Opus frames always begin with 00 for mono and 04 for stereo. So maybe for raw audio, the audio data could be prepended with a byte of f0 for mono and f4 for stereo? Then it could be recognised unambiguously. Both client and server need to recognise the format of a received frame correctly without relying on an out-of-band context.

[dingodoppelt]

As far as I understood these sentinel bytes come from the opus codec itself and are not deliberately set by Jamulus as a message ID of sorts. I'd have to overwrite actual audio bytes for that to work with my code. Or am I wrong here?

[dingodoppelt]

I had misunderstood the packet size calculation and it seems fixed with the last commit.

[dingodoppelt]

Note that the client will encode according to the settings in the Client Settings dialog, but the server will encode according to the information in received in the NETW_TRANSPORT_PROPS message it received from the client.

Talking of which, the codec field in the NETW_TRANSPORT_PROPS message should specify a different value for RAW, rather than still saying OPUS, like this:

I think OPUS and OPUS64 only refer to the setting of small network buffers. It isn't related to the actual opus coding.

[softins]

Note that the client will encode according to the settings in the Client Settings dialog, but the server will encode according to the information in received in the NETW_TRANSPORT_PROPS message it received from the client.
Talking of which, the codec field in the NETW_TRANSPORT_PROPS message should specify a different value for RAW, rather than still saying OPUS, like this:

I think OPUS and OPUS64 only refer to the setting of small network buffers. It isn't related to the actual opus coding.

Maybe - I hadn't got around to examining how the value was used in the code. It just felt wrong for the message to state OPUS when it wasn't, and maybe a specific value could also be useful to the server.

[dingodoppelt]

Maybe - I hadn't got around to examining how the value was used in the code. It just felt wrong for the message to state OPUS when it wasn't, and maybe a specific value could also be useful to the server.

The server isn't aware of the opus quality setting. It is only being sent the packet size and feeds that into the opus codec. There is currently no other way for the server than to determine the codec (or none) by the expected packet sizes for rawaudio. OPUS and OPUS64 refer to 128 or 64 samples internal buffering, no relations to audio quality settings.

[softins]

Just tried the latest build. It's looking good in Wireshark and sounding good too. No fragmentation either, as the max packet size is only 1068 for stereo, max quality, 10.67ms(256).

[JoshuaDodds]

Have you guys also tested with small network buffers? This seems to work as well for me and with a huge improvement on latency as well (as expected). Tested on arm64 ubuntu server side and win11 for the client.

[softins]

Hi Nils, I have some significant suggestions to give you, which are too involved for just review comments.

I have created my own branch from your rawaudio-dev and will send you a link to my branch in a day or two.

[softins]

I have created my own branch from your rawaudio-dev and will send you a link to my branch in a day or two.

@dingodoppelt - https://github.com/softins/jamulus/tree/rawaudio-dev-softins is the link to my branch.

What I've done so far:

• Branched it from your rawaudio-dev
• Rebased it on the current main
• Changed the Jamulus.pro and ChangeLog from 3.11.1dev to 3.12.0dev
• Added a new protocol message RAWAUDIO_SUPPORTED
• When receiving a new connection, the server sends that message immediately after the CLIENT_ID message
• Removed the version comparison with 3.11.1 from the client, and instead handle the RAWAUDIO_SUPPORTED message to enable bRawAudioIsSupported.

I have built and tested it as server and client, and it seems to work as expected.

I have other suggestions yet to be coded, but I thought you might like to see these first.

[dingodoppelt]

I have built and tested it as server and client, and it seems to work as expected.

I have other suggestions yet to be coded, but I thought you might like to see these first.

That's great! Good to hear the code is in good hands now ;)
Looking forward to learning a few more things

[softins]

In the meantime, I've just updated my Wireshark dissector to understand raw audio and the RAWAUDIO_SUPPORTED message. See https://github.com/softins/jamulus-wireshark

[ann0see]

Added a new protocol message RAWAUDIO_SUPPORTED

Perfect. That's way better than the version check.

[dingodoppelt]

I have created my own branch from your rawaudio-dev and will send you a link to my branch in a day or two.

@dingodoppelt - https://github.com/softins/jamulus/tree/rawaudio-dev-softins is the link to my branch.

What I've done so far:

* Branched it from your `rawaudio-dev`

* Rebased it on the current `main`

* Changed the `Jamulus.pro` and `ChangeLog` from 3.11.1dev to 3.12.0dev

* Added a new protocol message `RAWAUDIO_SUPPORTED`

* When receiving a new connection, the server sends that message immediately after the `CLIENT_ID` message

* Removed the version comparison with 3.11.1 from the client, and instead handle the `RAWAUDIO_SUPPORTED` message to enable `bRawAudioIsSupported`.

I have built and tested it as server and client, and it seems to work as expected.

I have other suggestions yet to be coded, but I thought you might like to see these first.

@softins
I rebased my branch on your changes but I am still hesitant to push them as they'd break compatibility with clients and servers up until the last commit in this PR. Since the version number is now the same as the main branch which is also in production use, users and websites like JamulusJams and Jamulizer can't filter for rawaudio servers anymore.
It would really confuse active rawaudio-dev users to push now but I think the code is in a good state and sees a good daily use so I'll leave it as is and will push later on after some more commits have been made that might mitigate some of the breaking changes.

[dingodoppelt]

Added a new protocol message RAWAUDIO_SUPPORTED

Perfect. That's way better than the version check.

I think of it more as a refactor because I was already using a protocol message, only a different one. This is rather a new mechanism for a feature check which was previously done via the server version message and I feel the other version checks should be refactored to use this message as well to save on messages being sent and to not have the same functionality split over different parts of the code.
I'd therefore suggest to call it FEATURE_SET and make it an integer or bitmask that the client and server can agree upon. This would also be compatible with future changes or individual features added to forks.
This is kind of a regression as well because the server version is available to external apps unlike the newly introduced message, which is only sent to a client and the rawaudio feature therefore not visible to third-party apps like the Jamulus explorer.

[softins]

Using the version number to enable the rawaudio had a couple of drawbacks:

1. The version number didn't adequately reflect reality when parallel development is happening. 3.11.1 was not an official version number, of course, and I didn't feel it would be correct to keep it after rebasing the rawaudio changes onto the main branch once 3.12.0 was released. It is hard to keep clients and servers agreeing on whether rawaudio is available by version number. It makes the version number rather magic!

2. The server doesn't send the version and OS message until it has exchanged quite a few other messages with the client, which means quite a bit of audio traffic before the client knows raw audio is available. When I added RAWAUDIO_SUPPORTED, it made it be sent by the server immediately after the initial CLIENT_ID message response to the first frame of audio. That means a compatible client could start sending raw audio immediately. Older clients that don't understand RAWAUDIO_SUPPORTED will ACK it but otherwise ignore it.

3. Re the comment that users of Jamulus Explorer couldn't see if a server supported raw audio if the server was just a 3.12.0-dev, instead of a 3.11.1dev. That's true, but it's only a temporary issue. This feature will be known to be in all servers from 4.0.0 onwards. In the meantime, server operators running a 3.12.x version with rawaudio support could indicate that in the server description or welcome message.

We could possibly turn the RAWAUDIO_SUPPORTED into a FEATURE_SET message as you suggested, but to be honest, a single message that only needs to be sent once per connection isn’t really a problem anyway, and it allows different parts of the code only to be concerned about their particular message. So I’m undecided either way. Other features have their own message.

If we did introduce a FEATURE_SET kind of message, we would need to be sure that multiple forks adding their own features didn't conflict with each other by using the same bit or number value for their feature. The same is already true regarding protocol message IDs, but they would need to be distinct anyway before a new feature was accepted into main, so the dev team would be the arbiters.

Jamulus has always placed great importance on backward compatibility in both directions, and I don't think that will be able to change with the release of 4.0.0.

I also don't think that can apply to those who have been using the 3.11.1 versions. The takeup of that was quick and enthusiastic, but those users would by their nature very easily act on a notice that they needed to update their client or server to use the new mechanism instead of the version check. We also shouldn't let the enthusiasm for an important new feature hasten us to set something in stone without taking the time to get it right for the long term.

Refactoring / trimming-down of the Jamulus code could certainly happen in some places, but that can't be at the expense of backward compatibility. We can't remove protocol messages that older clients or servers rely on.

[foobarth]

As the author of Jamalizer i appreciate you guys giving a thought about visibility of raw audio servers, but i second @softins assumption that this will only be a temporary issue until the next major release when all (most) servers and clients update. Until then, i'm happy with parsing some more well-known raw audio versions to highlight them. After 4.0 it's redundant anyway and might be removed on Jamalizer because it no longer distinguishes servers.

[rdica]

I echo @foobarth position and will look for other ways to highlight supported servers on jamulusjams.com if possible.

On the server side is the intention to bake the PCM support in, or have a compile time option, or have a cli arg to enable it?

[dingodoppelt]

Ok, this is now pushed and the old clients won't work on new servers and vice versa. I'd ask everybody involved to spread the word that there is a new client and server.
@rdica @foobarth I know that jamulus-php can access a server's welcome message. We could have a known field in it to recognise a raw audio server, but I don't think it is recommended to query the welcome messages too often. Maybe cache a server's version and only query the welcome message on a version mismatch or something similar.
I don't know if there is any other way to get infos on raw audio capabilities. Maybe @softins or @pljones could provide their assessment on that issue.

[softins]

Great news! I have a better idea than the welcome message.

The way that the jamulus-php Explorer backend fetches the welcome message is to send a single frame of audio and to then wait for the CHAT_TEXT message. A new rawaudio server will send RAWAUDIO_SUPPORTED before the CHAT_TEXT.

So tomorrow I will update the backend to listen out for RAWAUDIO_SUPPORTED and report its receipt with a new item in the JSON it returns.

[softins]

On the server side is the intention to bake the PCM support in, or have a compile time option, or have a cli arg to enable it?

It will be a standard feature in the upcoming 4.0.0, always enabled, subject to testing of forward and backward compatibility.

The intention is to get this feature into main, add some other pending items, and to release 4.0.0 in the reasonably near future (hopefully, an alpha release in a small number of weeks from now).

[softins]

@dingodoppelt I forgot to push the changes I made to server.cpp. I've done so now. Please check them out and merge if you're happy with them.

[JoshuaDodds]

Although already mentioned that it was intentional, I do find it a bit counterintuitive for some types of users that an upgrade to 3.12.0dev from an older 3.11.0dev client will transparely fail to deliver the higher quality uncompressed audio on any server not running 3.12.0dev as well (even though there are technically quite alot of 3.11.1dev builds in the wild right now running and perfectly capable of supporting raw audio to a newer client.

Is this really the intention? Isn't there a more elegant way to handle this? Is there any existing way for the server to push a message to a client that has been downgraded to high quality because of the missing protocol handshake at connection init? Some sort of alert that they need to upgrade their to achieve max quality on this server ?

(Just thinking out loud)... I do agree with @softins that most of the early adopters are mostly staying tuned into the developments happening over here on this branch but it would still be "nice to have" if there was any easy way to implement?

[ann0see]

No. This never was an official release. Thus there's no backward compatibility requirement IMO.
Adding backwards compatibility here is contributing to technical debt.
I object adding any workaround as it's just an upgrade for a small number of people.

[softins]

The way that the jamulus-php Explorer backend fetches the welcome message is to send a single frame of audio and to then wait for the CHAT_TEXT message. A new rawaudio server will send RAWAUDIO_SUPPORTED before the CHAT_TEXT.

So tomorrow I will update the backend to listen out for RAWAUDIO_SUPPORTED and report its receipt with a new item in the JSON it returns.

This now done and live. The JSON returned from https://explorer.jamulus.io/servers.php?query=host:port will include the item 'rawaudio':true if it received a RAWAUDIO_SUPPORTED message from the server.

I've also updated Jamulus Explorer to show a banner above the welcome message if the server has advertised raw audio support as above.

[pljones]

At some point, the 65+ commits are going to need squashing into something more reasonable.

All (apart from one) of my Jamulus instances are now running this branch. (Mostly they're single-client Directories.)

[dingodoppelt]

At some point, the 65+ commits are going to need squashing into something more reasonable.

Done! (After quite an intense time with git ;)