Add Protocol documentation #2337

emlynmac · 2022-02-03T17:05:36Z

Short description of changes
Added high-level protocol documentation of the client-server UDP interface.

Context: Improve documentation for contributors

Status of this Pull Request
Ready for peer review

What is missing until this pull request can be merged?
No functional code changes; just needs reviewing for accuracy.

ann0see

Great. Thanks. Will have a look at it but clearly @softins is the expert here.

Meta: Shouldn’t this be on the website or in a docs/ folder? #2283

pljones · 2022-02-03T17:55:09Z

Meta: Shouldn’t this be on the website or in a docs/ folder? #2283

docs/ folder now it's there, I think. It's probably something only someone writing code is going to use, so I think it's best left in the code repo. Ideally, we'd have the API description in some meta-language that could generate source in C++ or PHP or whatever...

emlynmac · 2022-02-03T18:04:17Z

Meta: Shouldn’t this be on the website or in a docs/ folder? #2283

docs/ folder now it's there, I think. It's probably something only someone writing code is going to use, so I think it's best left in the code repo. Ideally, we'd have the API description in some meta-language that could generate source in C++ or PHP or whatever...

I’ll move it in a moment

ann0see · 2022-02-03T18:12:25Z

The docs folder still needs to be created by my PR

emlynmac · 2022-02-03T18:35:46Z

The docs folder still needs to be created by my PR

Ah

ann0see · 2022-02-05T21:26:30Z

Ok. The docs folder is now created. You can now move your file into the folder (after a rebase)

hoffie

Thanks a lot for providing this documentation! This is really valuable for new contributors. :)
As I don't want my review to count here as I think @softins and @pljones are way more qualified to assert correctness, I'll just leave this as a comment for now. :)

hoffie · 2022-02-05T21:41:33Z

docs/JAMULUS_PROTOCOL.md

+--
+The OPUS codec is used to compress the audio over the network and the packets are documented [here](https://datatracker.ietf.org/doc/html/rfc6716).
+
+Jamulus uses a custom OPUS encoder / decoder, giving some different frame sizes, but always uses a 48kHz sample rate. OPUS and OPUS64 codecs are the only supported options currently.


OPUS and especially OPUS64 sound like official modes, somehow. But I think it's just the names which Jamulus gave to these encoder configurations, right? So maybe that could be clarified?

Yes, I've just looked through the code, and the only difference between them is that OPUS configures the Opus codec to use 128 samples per frame, and OPUS64 configures it to use 64 samples per frame. But it's the same codec, just two different Jamulus modes.

docs/JAMULUS_PROTOCOL.md

Typo fix Co-authored-by: Christian Hoffmann <christian@hoffie.info>

ann0see · 2022-02-16T21:07:31Z

Is this ready to be merged after r 3.8.2 and a review?

softins · 2022-02-24T17:53:38Z

Is this ready to be merged after r 3.8.2 and a review?

Yes, I'm planning to review it, but life is rather busy right now, and it hasn't come to the top of the pile.

ann0see

I'm not into the protocol, but it's good to have a brief intruduction here. I'm not entirely sure, but I think Jamulus handles all packages with a specific size as audio packages and all others as control message packages?

Approving either way since the document is not critical

docs/JAMULUS_PROTOCOL.md

ann0see

Thanks.

softins

This document is a good start, and a descriptive "what happens" is definitely needed and useful.

I do have some comments, though, noted down as I read through:

In the message packet structure, it needs to be specified that 2-byte numeric items are sent in little-endian order. So a length of 300, for example, would be a byte of 0x2C (44) followed by a byte of 0x01 (for 256).
Related to the above point, the message types should be expressed as native numbers, e.g. CLIENT_ID (32, 0x0020), CLM_DISCONNECTION (1010, 0x03f2). These numbers then correspond to those in src/protocol.h, and those displayed in the Wireshark dissector. But in the packet, for example, the CLM_DISCONNECTION ID would be 0xf2 followed by 0x03.
"Some ... need to be acknowledged, some do not." could be elaborated to say that all connected-state messages (ID < 1000) need to be acknowledged, except for ACK itself, and connectionless messages (CLM_xxx, ID >= 1000) do not need to be acknowledged.

I'm happy either to make and push these changes myself, or to leave it to @emlynmac.

Specify byte order as little-endian Clarify which packets must be acknowledged.

emlynmac · 2022-03-05T16:36:45Z

Thanks for the review @softins.
For the acknowledgment of IDs < 1000, is there a time limit by which this must happen or a retransmit occurs? Would be good to include that too.

I've added the decimal values to the hex in the flows.

softins · 2022-03-05T16:52:24Z

Thanks for the review @softins. For the acknowledgment of IDs < 1000, is there a time limit by which this must happen or a retransmit occurs? Would be good to include that too.

Yes, the time limit is 400ms, which is defined in

jamulus/src/protocol.h

Lines 94 to 95 in 7f9f039

    
           // time out for message re-send if no acknowledgement was received 
        
           #define SEND_MESS_TIMEOUT_MS 400 // ms

I've added the decimal values to the hex in the flows.

Thanks. I think the hex versions should have their bytes swapped in the descriptions too, so they match the decimal.

It would have been nice if the protocol had used network byte order, but we're years beyond that now, for Jamulus!

emlynmac · 2022-03-05T23:03:41Z

Thanks. I think the hex versions should have their bytes swapped in the descriptions too, so they match the decimal.

I deliberately put them in this way around, so that the hex values match the Wireshark dissector trace values. It made it easier to see

ann0see · 2022-03-09T20:34:45Z

@softins is this now ready?

softins · 2022-03-09T23:52:45Z

@softins is this now ready?

Not sure, but I have no time to review again until Friday.

softins

Subject to a couple of comments, I'm happy to approve it. It would be good as time permits to expand it, but I don't think there's a reason to keep it languishing as a PR.

softins · 2022-03-12T17:46:36Z

docs/JAMULUS_PROTOCOL.md

+--
+The OPUS codec is used to compress the audio over the network and the packets are documented [here](https://datatracker.ietf.org/doc/html/rfc6716).
+
+Jamulus uses a custom OPUS encoder / decoder, giving some different frame sizes, but always uses a 48kHz sample rate. OPUS and OPUS64 codecs are the only supported options currently.


Yes, I've just looked through the code, and the only difference between them is that OPUS configures the Opus codec to use 128 samples per frame, and OPUS64 configures it to use 64 samples per frame. But it's the same codec, just two different Jamulus modes.

docs/JAMULUS_PROTOCOL.md

Co-authored-by: Tony Mountifield <tony@mountifield.org>

* Start of high-level protocol documentation * Add message diagram for connection * Base protocol documentation * More updates * Details of the audio transport * Move to docs directory * Update docs/JAMULUS_PROTOCOL.md Typo fix Co-authored-by: Christian Hoffmann <christian@hoffie.info> * Update docs/JAMULUS_PROTOCOL.md Typo fix Co-authored-by: Christian Hoffmann <christian@hoffie.info> * Update style * Add decimal values for message IDs in addition to the hex values Specify byte order as little-endian Clarify which packets must be acknowledged. * Mention the retransmit timeout. * Clarify Buffer size not Packet size Co-authored-by: Tony Mountifield <tony@mountifield.org> Co-authored-by: Christian Hoffmann <christian@hoffie.info> Co-authored-by: ann0see <20726856+ann0see@users.noreply.github.com> Co-authored-by: Tony Mountifield <tony@mountifield.org>

ann0see · 2022-06-18T20:04:01Z

CHANGELOG: Documentation: Add high-level technical documentation for the Jamulus protocol

ann0see reviewed Feb 3, 2022

View reviewed changes

emlynmac added 6 commits February 5, 2022 13:38

Start of high-level protocol documentation

a487076

Add message diagram for connection

d3fc6e5

Base protocol documentation

db19891

More updates

0d6637e

Details of the audio transport

dc941a2

Move to docs directory

025e6f6

emlynmac force-pushed the protocol-documentation branch from 47b0a49 to 025e6f6 Compare February 5, 2022 21:39

hoffie reviewed Feb 5, 2022

View reviewed changes

emlynmac and others added 2 commits February 5, 2022 15:32

Update docs/JAMULUS_PROTOCOL.md

2f1b3d3

Typo fix Co-authored-by: Christian Hoffmann <christian@hoffie.info>

Update docs/JAMULUS_PROTOCOL.md

ffef7e6

Typo fix Co-authored-by: Christian Hoffmann <christian@hoffie.info>

ann0see requested a review from softins February 20, 2022 18:47

ann0see approved these changes Mar 4, 2022

View reviewed changes

docs/JAMULUS_PROTOCOL.md Outdated Show resolved Hide resolved

Update style

88ce0ef

ann0see approved these changes Mar 4, 2022

View reviewed changes

softins reviewed Mar 5, 2022

View reviewed changes

Add decimal values for message IDs in addition to the hex values

42a3bff

Specify byte order as little-endian Clarify which packets must be acknowledged.

Mention the retransmit timeout.

edeb016

ann0see added this to the Release 3.9.0 milestone Mar 9, 2022

softins approved these changes Mar 12, 2022

View reviewed changes

Clarify Buffer size not Packet size

33acdd1

Co-authored-by: Tony Mountifield <tony@mountifield.org>

ann0see merged commit 9457baa into jamulussoftware:master Mar 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Protocol documentation #2337

Add Protocol documentation #2337

emlynmac commented Feb 3, 2022

ann0see left a comment •

edited

Loading

pljones commented Feb 3, 2022

emlynmac commented Feb 3, 2022

ann0see commented Feb 3, 2022

emlynmac commented Feb 3, 2022

ann0see commented Feb 5, 2022

hoffie left a comment

hoffie Feb 5, 2022

softins Mar 12, 2022

ann0see commented Feb 16, 2022

softins commented Feb 24, 2022

ann0see left a comment

ann0see left a comment

softins left a comment

emlynmac commented Mar 5, 2022

softins commented Mar 5, 2022

emlynmac commented Mar 5, 2022

ann0see commented Mar 9, 2022

softins commented Mar 9, 2022

softins left a comment

softins Mar 12, 2022

ann0see commented Jun 18, 2022 •

edited

Loading

Add Protocol documentation #2337

Add Protocol documentation #2337

Conversation

emlynmac commented Feb 3, 2022

ann0see left a comment • edited Loading

Choose a reason for hiding this comment

pljones commented Feb 3, 2022

emlynmac commented Feb 3, 2022

ann0see commented Feb 3, 2022

emlynmac commented Feb 3, 2022

ann0see commented Feb 5, 2022

hoffie left a comment

Choose a reason for hiding this comment

hoffie Feb 5, 2022

Choose a reason for hiding this comment

softins Mar 12, 2022

Choose a reason for hiding this comment

ann0see commented Feb 16, 2022

softins commented Feb 24, 2022

ann0see left a comment

Choose a reason for hiding this comment

ann0see left a comment

Choose a reason for hiding this comment

softins left a comment

Choose a reason for hiding this comment

emlynmac commented Mar 5, 2022

softins commented Mar 5, 2022

emlynmac commented Mar 5, 2022

ann0see commented Mar 9, 2022

softins commented Mar 9, 2022

softins left a comment

Choose a reason for hiding this comment

softins Mar 12, 2022

Choose a reason for hiding this comment

ann0see commented Jun 18, 2022 • edited Loading

ann0see left a comment •

edited

Loading

ann0see commented Jun 18, 2022 •

edited

Loading