mumble-voip / mumble

Mumble is an open-source, low-latency, high quality voice chat software.
https://www.mumble.info
Other
6.31k stars 1.11k forks source link

[Feature Request]: Capability negotiation #4224

Open streaps opened 4 years ago

streaps commented 4 years ago

I feel a little bit uneasy on how new features that affects the protocol are developed. Sometime version checks for client and/or server version are added ad-hoc in some PR and it doesn't get documented anywhere. I also wonder what the implications are for other clients and servers (Grumble, Mumla, mumble-web and various client libraries, bots, etc).

How should a client (developer) decide which version number to send, if changes and new versions are not documented anymore? Can client and server really rely on these version numbers? What if a client supports one feature of version 1.a.b, but doesn't support another required feature of 1.a.b? What if the developer of a client doesn't know about these checks or which version disables or enables stuff.

When and how (in the development process) get theses changes that affects other client and server discussed and decided?

It's also not clear what the intended meaning of the version is. Is it UI related (as "uiVersion" suggests) or is it a protocol version that is used independently of the real client version (as PYMUMBLE_PROTOCOL_VERSION suggest)?

Some protocols have capability negotiation, some use protocol version, some don't have any of this and people use whatever seems to work. How does it work in the Mumble protocol? I guess using protocol buffers avoids many of the issues that other protocols try to solve with capability negotiation.

I don't know of any major (potential) compatibility problem, but I also don't know if there are any critical version checks in Mumble or Murmur. Without documentation it is impossible to know without digging into the C++ Mumble code.

Krzmbrzl commented 4 years ago

First off the version is always the mumble client or server version (from the official ones). uiVersion doesn't have anything to do with UI. The prefix stands for unsigned int. That's something that really confused me in the beginning as well though.

How should a client (developer) decide which version number to send, if changes and new versions are not documented anymore?

I guess it'd come down to "does my implementation support everything that the official client/server of that particular version supports. In order to know what that is, you'd simply have to check the proto file from the tag in question.

What if a client supports one feature of version 1.a.b, but doesn't support another required feature of 1.a.b?

That's not supported atm. If you encounter such a situation in your implementation, you either have to add the missing functionality or you find a way to deal with the consequences (whatever they might be).

I guess using protocol buffers avoids many of the issues that other protocols try to solve with capability negotiation.

That's exactly the case. Protocol buffers stay backwards compatible forever regardless of what new messages/fields are added to the spec (provided you don't start re-using old names, etc.). Therefore most (if not all) version checks are done in order to not confuse the user (e.g. why show a context menu entry for something that the server the user is connected to doesn't understand?). If a client was to not check for the server user and send it a message that the server doesn't understand, then that message is simply discarded. All that happens then is that this new functionality doesn't work on that server.

In regards to the documentation: I think it is pretty much nonsense to have the documentation on the protocol in a separate repository. This kind of stuff should be added as a comment in the proto file and based on these comments the web-documentation should be generated automatically. That way the documentation stays in sync with the code.

stale[bot] commented 4 years ago

This support-issue has been automatically marked as stale because it has not had recent activity. If no further activity occurs, the issue will be automatically closed as we'll assume your problem to be fixed.

streaps commented 4 years ago

I don't understand this as a support issue, but as a pledge to add proper capability negotiation. Doing this based on versions seems wrong. Mumble uses protobuffer. There should be a better way to do it.