Duplicated Opus fields in our protobuf scheme

[TerryGeng]

As mentioned in #3918, our protocol is a mess when things come to Opus. There are so many Opus fields in all kinds of message types. As far as I know,

1. Inside CodecVersion.
2. Inside UserStats, to tell other users which codec this user is using, which will be rendered in UserInformation window.
3. Inside Authenticate, which should be removed.

I have traced the opus flag inside Authenticate message.

It looks like only few places referred to this field. Set:

1. murmur/Server.cpp, inside Server::recheckCodecVersions, tell clients the codecs the server supported. Interestingly, I believe the server doesn't decode audio packets.
2. mumble/ServerHandler.cpp, inside ServerHandler::serverConnectionConnected, tell the server if the client support Opus. But there's already a flag doing the same thing inside CodecVersion.

Read:

1. murmur/Message.cpp, inside Server::msgAuthenticate, set a flag to indicate the user support opus. But this field seems to be not very useful since codec information is included in voice packets.

Therefore, I believe it can be deleted without much to worry about.

What bothers me is https://github.com/mumble-voip/mumble/blob/e6f988304aaee20d12bbbec53ce8ec0e655d413b/src/murmur/Server.cpp#L2020-L2021 I think this part is certainly deprecated and need to be removed in the next version.

I open this issue since this is a somehow big change. I believe this flag must be a byproduct of the long history of mumble itself. So I'm not very sure about what kind of approach we need to adapt.

[streaps]

Why are you bothered by the opusthreshold? It's not really problematic.

I think it is time to remove the old CELT support though, which would make opusthreshold obsolete.

[Krzmbrzl]

The CodecVersion will become stale once we remove support for CELT (note that it won't be removed in order to make sure that the name won't be reused). https://github.com/mumble-voip/mumble/blob/21661ea860dad463ffef8652453c21ee11d12cfa/src/Mumble.proto#L475-L485

The only references to opus remaining are then in UserStats and in Authenticate where they both serve a valid purpose as these messages are used for very different things.

The recheckCodecVersions seems to also be a candidate for removal once CELT has been dropped.

tell the server if the client support Opus. But there's already a flag doing the same thing inside CodecVersion.

But CodecVersion is not sent from the client to the server. This is a message that (afaik) is only sent from the server.

In general though, fields should never be removed from the proto-file. They can be marked as deprecated, but they must not be removed in order for the protobuf-magic to keep working (allowing the protocol to grow without breaking backwards compatibility)

[TerryGeng]

@Krzmbrzl I think we can mark that field as reserved.

If you update a message type by entirely removing a field, or commenting it out, future users can reuse the field number when making their own updates to the type. This can cause severe issues if they later load old versions of the same .proto, including data corruption, privacy bugs, and so on. One way to make sure this doesn't happen is to specify that the field numbers (and/or names, which can also cause issues for JSON serialization) of your deleted fields are reserved. The protocol buffer compiler will complain if any future users try to use these field identifiers.

message Foo {
  reserved 2, 15, 9 to 11;
  reserved "foo", "bar";
}

[streaps]

@TerryGeng all the fields and the logic in the server seems to be needed, if you want to support old clients that don't support Opus at all. If a CELT-only client joins the room, all other clients in the room get the message that they shouldn't use Opus (CodecVersion.opus=false). opusthreshold=0 disables this behaviour and ignores that CELT-only clients cannot decode the stream.

I doubt that there are any CELT-only clients still used or that there is a technical reason to use CELT (the Opus codec includes a improved version of CELT anyway).

Btw, how are the protobuffer message names encoded? Are they fully transmitted over the wire?

[Krzmbrzl]

I think we can mark that field as reserved.

Yeah I guess that'd work :thinking:

Btw, how are the protobuffer message names encoded? Are they fully transmitted over the wire?

I don't know for sure but I'd assume that they're not. I think that they are transmitted as some sort of ID in the message's header...