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

CHANGE: Use Protobuf for UDP messages #5594

Merged
merged 15 commits into from
Mar 27, 2022

Conversation

Krzmbrzl
Copy link
Member

Previously Mumble was using a custom binary format for transmitting data
via UDP (mainly audio). This has worked for a long time but besides
being inconvenient for 3rdParty implementors (they had to manually
re-implement encoding and decoding support for this format) this format
was not very flexible and changes to the data format were very hard.

In order to improve on this situation, this commit introduces changes
that allow to use Protobuf for the UDP messages as well (it's already
used for TCP). With that it should be relatively easy to extend/change
the UDP packet formats in the future and 3rdParty implementors can now
simply use Protobuf to handle decoding/encoding packets for them (much
less work and much less prone to errors).

Since the new Protobuf format is incompatible with the old UDP format,
this commit also includes support for dealing with older clients or
servers that don't recognize the new protocol yet. That way the new
protocol format is only used if both the client and the server are
recent enough to have it implemented (assumed to be the case >=1.5.0).

Note also that the server will make sure that clients using the old and
the new format can seamlessly communicate with one another.

Therefore, on the surface it should not be noticeable to the user which
protocol is currently used.

Note also that the new protocol format only supports Opus as an audio
codec. If one of the legacy codecs is to be used, the legacy packet
format has to be used as well. However, all codecs except for Opus will
be removed from Mumble in the future anyway.

Fixes #4350

NOTE: When this gets merged, all clients and servers claiming to support the protocol version 1.5.0 that do not include these changes, will become incompatible with the ones that do integrate these changes. This is important for everyone closely following our upstream master branch. A simple update to the latest code changes (including the changes from this PR) should be enough to restore full compatibility.
All servers and clients of version < 1.5.0 will be not affected by this as this PR is completely backwards compatible with those clients.

Checks

@Krzmbrzl
Copy link
Member Author

Krzmbrzl commented Mar 12, 2022

I'll format and squash the commits after review.


@radioactiveman you are the maintainer of the AUR Mumble package that tracks Mumble master, right? If so the note in the above PR description is probably important for you (and your users) ☝️

@Krzmbrzl Krzmbrzl requested a review from davidebeatrici March 12, 2022 08:59
@radioactiveman
Copy link
Contributor

@radioactiveman you are the maintainer of the AUR Mumble package that tracks Mumble master, right? If so the note in the above PR description is probably important for you (and your users)

@Krzmbrzl: Yes, that's right. But if I understand it correctly, Mumble clients compiled from master are compatible with older versions (server and client). Only if the server is updated as well, this may cause compatibility issues with older clients. Is this correct?

Related to this PR: Please adopt the hint by @Polynomial-C in #5584 also here for the new gsl make-dependency.

@Krzmbrzl
Copy link
Member Author

But if I understand it correctly, Mumble clients compiled from master are compatible with older versions (server and client). Only if the server is updated as well, this may cause compatibility issues with older clients. Is this correct?

Everything is compatible with each another, as long as everything that claims to run version 1.5.0 includes this PR. As soon as there is a communication of two instances claiming to run 1.5.0, where only one of them includes this PR (e.g. a server built from master before this PR was merged and a client built from master after the merge (or vice versa)), there will be an incompatibility and voice will not work. The reason for this is that the peer that has the changes from this PR included will use the new protocol (as it assumes that everything claiming to be >= 1.5.0 understands it), which won't be understood by the second peer in this scenario.

Related to this PR: Please adopt the hint by Polynomial-C in #5584 also here for the new gsl make-dependency.

Will do 👍

@Krzmbrzl Krzmbrzl force-pushed the feat-protobuf-for-UDP branch from a4e9e4f to eb7f7b8 Compare March 16, 2022 19:07
The "Guidelines Support Library" (GSL) is a library that implements
functions and types suggested by the C++ Core Guidelines. This should
make it relatively straight forward to follow these guidelines and thus
to write better C++ code.

This explicitly adds the Microsoft GSL as that seems to be the most
popular (and complete) GSL implementation available.
Besides the typedef that is meant to be used in code, there is also one
that encodes a type of the exact size (32bit) a raw version is expected
to have.
The manipulation of raw version specifiers was turned into constexpr
functions, in order for them to be usable in constexpr expressions.
In the server browser, the tooltip for the different entries shows the
packet loss. It is displayed as a percentage and as a x/y stat. The
latter currently displays <received>/<sent> which is very confusing,
given that the title for that row is "Packet loss" implying x would
describe the lost packages instead of the received ones.

Thus this commit changes this to the expected behavior. Furthermore it
makes sure that <lost> is always <= <sent>.
@Krzmbrzl Krzmbrzl force-pushed the feat-protobuf-for-UDP branch 2 times, most recently from 19cc026 to dd0d387 Compare March 22, 2022 19:13
@Krzmbrzl Krzmbrzl force-pushed the feat-protobuf-for-UDP branch from dd0d387 to 4d49c42 Compare March 26, 2022 13:48
Previously Mumble was using a custom binary format for transmitting data
via UDP (mainly audio). This has worked for a long time but besides
being inconvenient for 3rdParty implementors (they had to manually
re-implement encoding and decoding support for this format) this format
was not very flexible and changes to the data format were very hard.

In order to improve on this situation, this commit introduces changes
that allow to use Protobuf for the UDP messages as well (it's already
used for TCP). With that it should be relatively easy to extend/change
the UDP packet formats in the future and 3rdParty implementors can now
simply use Protobuf to handle decoding/encoding packets for them (much
less work and much less prone to errors).

Since the new Protobuf format is incompatible with the old UDP format,
this commit also includes support for dealing with older clients or
servers that don't recognize the new protocol yet. That way the new
protocol format is only used if both the client and the server are
recent enough to have it implemented (assumed to be the case >=1.5.0).

Note also that the server will make sure that clients using the old and
the new format can seamlessly communicate with one another.

Therefore, on the surface it should not be noticeable to the user which
protocol is currently used.

Note also that the new protocol format only supports Opus as an audio
codec. If one of the legacy codecs is to be used, the legacy packet
format has to be used as well. However, all codecs except for Opus will
be removed from Mumble in the future anyway.

Fixes mumble-voip#4350
Previously the opusthreshold config option had a default value of 100
meaning that the first non-Opus client would cause everyone to fall back
to one of the legacy codecs.

Since Opus has been implemented for ages now and we eventually want to
get rid of the legacy codecs, this commit changes the default value of
this config option to be zero, meaning that the server will always
enforce the use of the Opus codec (regardless of what clients are
connected).
This message type is explicitly handled before calling into the
different msg* implementations. Therefore, msgUDPTunnel could never be
called in the current state of the code base.

However, we can't remove the implementation completely, because of the
macro voodoo that is used to declare and switch over the different
message types (we can't remove the entry from the macro, since in there
we still need it for defining the respective enum entry).
This should allow to get detailed insights into the audio processing
performance when using the Tracy profiler.
The update includes the changes made to the upstream code since 2016
plus an additional fix for a memory leak encountered in Mumble's usecase
(in the new audio cache implementation introduced with switching Mumble
audio to the Protobuf format).
This speeds up the invocation of runClangFormat.sh dramatically.
@Krzmbrzl Krzmbrzl force-pushed the feat-protobuf-for-UDP branch from 4d49c42 to 928513d Compare March 27, 2022 07:50
@Krzmbrzl Krzmbrzl added the feature-request This issue or PR deals with a new feature label Mar 27, 2022
@Krzmbrzl Krzmbrzl merged commit 74faaba into mumble-voip:master Mar 27, 2022
@Krzmbrzl
Copy link
Member Author

@BombJovi see #5604 - you are not the only one seeing this error. In order to avoid it, you can use -Dwarnings-as-errors=OFF when invoking cmake.

Commenting here on GitHub is generally the preferred way to go, however it's usually more convenient if you commented on the respective pull request instead of directly on the commit itself as GItHub's UI in that regard just plain sucks (it's really, really hard to actually find these comments on larger commits)

@BombJovi
Copy link
Contributor

Right on! Thank you for teaching me how how to proceed, Krzmbrzl! I'll for sure keep that in mind next time if I encounter any issues.

@Krzmbrzl Krzmbrzl deleted the feat-protobuf-for-UDP branch November 9, 2022 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request This issue or PR deals with a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Protocol: Use ProtoBuf for UDP messages
4 participants