Manage client-side channels locally, own channel always 0 #3426
Conversation
On initial connect, server sends an empty channel list (jamulussoftware#1010) before the real one. Skip client channel management if list is empty, so that we don't lose client channel 0 for our own server channel.
Instead of using separate oldGain and newGain arrays. We also don't need to reset the oldGain and newGain values when freeing a channel, as they are always initialised when allocating a channel.
|
I have left the chain of commits separate, so that reviewers can see the sequence of development if interested. |
pljones
added
refactoring
|
NOTE: I'm happy with the scope of this PR. The following outlines my thoughts - I'll raise a feature request shortly after this gets merged. OK, just thinking about the wider implications...
So we will have inconsistent client behaviour based on server version, with nothing explanatory for the user to understand why. I don't like that. I'd rather we made the default act as if we had not received the "own server channel" message and only enable remapping based on receipt of the message if:
This way we can make the GUI option disabled when the server does not supply "own server channel" and document the effect of the new CLI option as being server-dependent. The new CLI option should additionally support one of
Of course, this pulls the channel sorting logic into the client itself, so that the GUI simply displays the channels in the order held by the client, which further reduces logic in the GUI code. (Yes, there should also be a JSON-RPC interface to set the sort order.) |
I don't think that's a problem. There are already differences in client behaviour depending whether it receives the CLIENT_ID message. When talking to a server < 3.5.5, I think the following features will work differently or not at all:
I don't see what problem that is trying to solve, beyond the notion of consistency with archaic server versions, and I don't think it gives any advantage to the user. Version 3.5.5 was released on 26 May 2020. Looking in explorer at the moment, under Any Genre 1, 7 out of 108 servers are older than 3.5.5, and only 12 are older than 3.6.2. The oldest on Any Genre 2 is 3.6.1, of which there are 2 out of 51; the rest are 3.6.2 or newer. All the other directories only accept 3.6.2 or newer servers anyway. Anyone running a private server but using a recent version of client is extremely unlikely to be using a really old version of server. So changing the default as suggested would just be degrading the operation of the client for consistency with a minuscule set of situations. The user doesn't get to choose the channels assigned by the server, so they also don't need to choose whether their own channel gets client channel 0 or not. It's no disadvantage to the user to consistently get channel 0 for themselves by default in the overwhelming majority of cases. I would much rather it became a non-issue by having all the public directories only accept registrations from servers of 3.6.2 or newer. Also, if we incorporate #3416, then the user would see the server versions in the Connect dialog, and could decide not to connect to an archaic version of server. |
I think the client layer is the wrong place to do any sorting. We don't want to be changing channel assignments on the fly. Sorting is a presentational function, and so the mixer board is the right place to do it, as at present.
The JSON-RPC interface will/should receive all the same channel info that the mixer board does. A user interface that gets its info via JSON-RPC can therefore do its own sorting for presentation to the user, based on that information and the user's settings (which don't need to be sent to the Jamulus client). |
Yes, but it's not an UI only feature either.
I think it is confusing if JSON-RPC and GUI show different ordering. |
I agree - this time. It's not a huge problem. |
| clientChannelIDs[i] = INVALID_INDEX; | ||
| } | ||
|
|
||
| // qInfo() << "> Client channel list cleared"; |
There was a problem hiding this comment.
Probably worth getting rid of this debug output (this doesn't add anything and can be easily re-added during debugging if necessary)
There was a problem hiding this comment.
I thought it would be useful to retain the qInfo() messages I used while debugging, just commented out, then someone wanting to re-enable them only needs to uncomment, not compose afresh or search earlier commits for them.
| // any unexpected temporary mismatch in size due to potential out-of-order message delivery. | ||
| // | ||
| // This function returns true if the list has been processed and should be passed on, | ||
| // or false if it was the wrong size and should be discarded. |
There was a problem hiding this comment.
Not part of this PR, but I'd like to have a standard format like in the recorder of JSON-RPC server for documentation like this
Co-authored-by: ann0see <[email protected]>
Well let's have this conversation in any follow-up issue, as it's not relevant to this PR. |
|
I don't think there is any reason to delay this PR until 4.0.0. It can very usefully be merged now. |
Short description of changes
Manages the allocation of client-side channels independently of server-side channels, and maps between
the two at the boundary between CClient and the lower-level CChannel and CProtocol.
All layers above (e.g. CClientDlg and the audio mixer board) will use client-side channels transparently.
When talking to a server version 3.5.5 or newer, the client is told its own server channel, and will always map
this to client channel 0. This allows the user of a MIDI device or JSON-RPC client to know that channel 0 will always control
the user's own audio channel.
In addition, when connecting to a server that only has a small number of high-numbered channels remaining
after many users have left, those channels will automatically be mapped to lower client channel numbers and
so will be controllable using local MIDI faders.
CHANGELOG: Client: allocate channel numbers locally and always give user own channel of 0
Context: Fixes an issue?
Resolves #3423 and supersedes both #3419 and #3394
Does this change need documentation? What needs to be documented and how?
Most operation is unchanged from before, but it is worth mentioning that the user's own channel will always be
number 0 unless connected to a server older than 3.5.5
Status of this Pull Request
Tested and working, ready for review.
What is missing until this pull request can be merged?
Testing on multiple platforms.
Checklist