MusicPlayerDaemon / MPD

Music Player Daemon
https://www.musicpd.org/
GNU General Public License v2.0
2.19k stars 350 forks source link

New commands outputlist and output #2147

Closed jcorporation closed 2 weeks ago

jcorporation commented 2 weeks ago
MaxKellermann commented 2 weeks ago

What is the purpose of these two new commands?

jcorporation commented 2 weeks ago

Clients usually prints a list of outputs and after the user selects an output, it shows the details. This new commands reduces the protocol overhead and simplifies client implementation of handling outputs.

MaxKellermann commented 2 weeks ago

Does it really reduce protocol overhead? For your workflow, you need two commands instead of one, i.e. doubles the number of round trips, for something that usually fits into a single TCP PDU (unless you mean monster MPD installations with dozens or hundreds of outputs). Now you have two PDUs instead of one; smaller PDUs, but PDU sizes don't matter a lot. Twice the number of PDUs and twice latency is what matters much more than PDU size.

jcorporation commented 2 weeks ago

unless you mean monster MPD installations with dozens or hundreds of outputs

I have users with dozens of mpd outputs

For your workflow, you need two commands instead of one,

Only if the user really want to show the details of an output.

The usual usage is to list the outputs with its type and enabled status. More details are only required if the user want to see the attributes or want to modify an attribute. This is in my workflow a second step.

With current implementation:

  1. I must request all outputs with all details to print a list of outputs.
  2. User selects a output to show its details
  3. I must request all outputs again, filter the list by its id and present it to the user.

With my implementation:

  1. I must request all outputs with no details to print a list of outputs.
  2. User selects a output to show its details
  3. I must request only one output with it details
MaxKellermann commented 2 weeks ago

Step 3 is superfluous because your client has the data already. You only need to discard that response and repeat the command if you receive an "output" idle event. Adding new protocol commands increases the complexity which adds a maintenance burden on MPD, libraries and clients; that must be justified by some real advantage. I'm always for optimizing the protocol to make it lighter and transfer only what is needed, but I'm not convinced of this one. The outputs command does not seem to need any such optimizations. On my computer with one ALSA output, the outputs response is:

OK MPD 0.24.0
outputid: 0
outputname: alsa
plugin: alsa
outputenabled: 1
attribute: allowed_formats=
attribute: dop=0
OK

That's 104 bytes. Let's say you have a dozen ALSA outputs, each with the overhead for those two dummy attributes (which we could/should omit if there's nothing useful), we're at 1040 bytes. That's well below the MTU. Since we're comparing with your approach of using two PDUs, we have a budget of more than 2800 bytes. You'd need 27 outputs to exceed the size of two PDUs; i.e. only users with 27 outputs or more would benefit from your optimization, at the cost of degrading performance for the 99.9% of all other MPD users (those who have less than 14 outputs).