MusicPlayerDaemon / MPD

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

Add commands to protocol to align with Mpc's Capabilities #2030

Closed roizcorp closed 2 months ago

roizcorp commented 2 months ago

Feature request

Hi,

I am missing few commands from the protocol to match mpc's capabilities

setvol command with the ability to define the marginal volume i.e. +5 or -5

queue command, in order to get the string for the next song in the format of artist - title (can be the same as currentsong format).

jcorporation commented 2 months ago

mpc uses only the mpd protocol, therefor all protocol commands are already existing.

For reference: https://mpd.readthedocs.io/en/latest/protocol.html

roizcorp commented 2 months ago

I'm familiar with the documentation, and if you will pay attention there is no command that retrieves queued song, the status command provide the nextsong by id, in which I need to go over the playlist to find in order to get the desired artist - title format.

The same goes for the setvol commands, they are absolute and cannot add an increment to the current volume, you can only set the exact volume level

MaxKellermann commented 2 months ago

I'm familiar with the documentation, and if you will pay attention there is no command that retrieves queued song, the status command provide the nextsong by id, in which I need to go over the playlist to find in order to get the desired artist - title format.

That would be a very clumsy way to do it. The right way is the playlistid command.

@jcorporation has made a good point. You want to be able to do the same things mpc can do, but if mpc can already do them, you can do just as well. There is nothing special about mpc, it doesn't have capabilities that exceed the MPD protocol.

I consider this feature request pointless because its premise is flawed.

kingosticks commented 2 months ago

if you will pay attention there is no command that retrieves queued song

Did you pay enough attention to https://mpd.readthedocs.io/en/latest/protocol.html#command-playlistid ?

It doesn't provide the format you desire but not everyone wants your format. Fortunately, it's trivial for you to convert the response into whatever format you desire.

MaxKellermann commented 2 months ago

It doesn't provide the format you desire

Formatting is a pure client-side thing anyway. The MPD protocol is about obtaining raw information; how to present it to the user is out of the protocol's scope.

roizcorp commented 2 months ago

Thanks for the comments, perhaps I am mistaking, but as far as I understand, in order to use playlistid I need to know first the id of the song, so I need to execute the status command, grab the id from nextsong attribute and then call the playlistid command with the value. Correct?

If so, my request is valid (might be low prioritized though) in a single command be able to get the next song name (in whatever format...it's really less important).

The same goes for the volume, I have to know the current level first before I could ever know how much will it be +5%, and that will require 2 commands and additional code on client side to parse result, add 5 and send it in the second command.

What I am missing that you clearly see?

jcorporation commented 2 months ago

The same goes for the volume, I have to know the current level first before I could ever know how much will it be +5%, and that will require 2 commands and additional code on client side to parse result, add 5 and send it in the second command.

There is a volume command that does exactly this, but it is marked as deprecated. I personally do not know why Max marked it as deprecated.

Thanks for the comments, perhaps I am mistaking, but as far as I understand, in order to use playlistid I need to know first the id of the song, so I need to execute the status command, grab the id from nextsong attribute and then call the playlistid command with the value. Correct?

That is correct. This is easy to implement on client side. I see no demand for a new protocol command.

MaxKellermann commented 2 months ago

There is a volume command that does exactly this, but it is marked as deprecated. I personally do not know why Max marked it as deprecated.

I didn't. It was deprecated 4 years before I got involved with MPD: 636f83dc7bc8909032cbfe9e92cf24542b45cad7 (oh yeah, that's 20 years ago already! - but look how MPD takes backwards compatibility seriously; a command that was deprecated 20 years ago still works today!)

But I have often been thinking about bringing it back officially. The command makes a lot of sense, it can reduce the latency of simple clients by saving a roundtrip, and it is used a lot.

Thanks for the comments, perhaps I am mistaking, but as far as I understand, in order to use playlistid I need to know first the id of the song, so I need to execute the status command, grab the id from nextsong attribute and then call the playlistid command with the value. Correct?

That is correct. This is easy to implement on client side. I see no demand for a new protocol command.

A dedicated command for obtaining detailed information about the queued command can save a roundtrip just like volume vs setvol, but I'm not convinced that this exotic feature deserves a dedicated command.

jcorporation commented 2 months ago

I didn't. It was deprecated 4 years before I got involved with MPD: https://github.com/MusicPlayerDaemon/MPD/commit/636f83dc7bc8909032cbfe9e92cf24542b45cad7 (oh yeah, that's 20 years ago already! - but look how MPD takes backwards compatibility seriously; a command that was deprecated 20 years ago still works today!)

But I have often been thinking about bringing it back officially. The command makes a lot of sense, it can reduce the latency of simple clients by saving a roundtrip, and it is used a lot.

Then let us bring it officially back. An other idea can be the extension of the setvol command to accept absolute or relative values and leave changevol only for backward compatibility.

MaxKellermann commented 2 months ago

Since there's already volume, let's not add the same feature to setvol. The latter might be more consistent with how we've been extending the MPD protocol in the last 10 years, but adding a redundant feature would just add more cruft and wouldn't really solve anything.

roizcorp commented 2 months ago

If I can add another thing to the mix, pause command by the documentation should work the same as Mpc's toggle. But when MPD is in stop state, only toggle starts the music while pause does nothing

MaxKellermann commented 2 months ago

If I can add another thing to the mix, pause command by the documentation should work the same as Mpc's toggle. But when MPD is in stop state, only toggle starts the music while pause does nothing

Please do not add more things to the mix. This issue is too confusing already, partly because you were already mixing two unrelated feature requests in one issue, and partly because you were misunderstanding the protocol.

If you do write a separate feature request, there should be a proper explanation why something "should work the same as ...", and not just state that. I don't agree, but you didn't even try to convince anybody, and maybe somebody agrees once you explain it.

jcorporation commented 2 months ago

pause {STATE} Pause or resume playback. Pass 1 to pause playback or 0 to resume playback. Without the parameter, the pause state is toggled.

This is from the mpd documentation, what is the issue? How mpc handle this commands is another topic. Mpc commands do not necessarily map 1:1 to mpd protocol. A client has always the freedom to abstract some mpd protocol commands.

roizcorp commented 2 months ago

@MaxKellermann Ok, no need to be hostile though, I'm doing this due to my love to MPD.

@jcorporation I hear mixed messages, which I'm fine with either but it would be easier if this could be consistent.

  1. MPC is a very simple instrumentation of the MPD protocol, provides a good way (I think) to control and manage MPD.
  2. MPC is a client with its own logic which can incorporate multiple protocol commands.

My 2 cents are that if they would be aligned (or at least inspired to be) a lot of the roundtrips could be reduced with the examples I provided in this issue.

MaxKellermann commented 2 months ago

@MaxKellermann Ok, no need to be hostile though, I'm doing this due to my love to MPD.

I don't mean to be hostile. I only tried to point out where the discussion went in the wrong direction - explaining where your feature request was confusing and not convincing.

MaxKellermann commented 2 months ago

My 2 cents are that if they would be aligned (or at least inspired to be)

What does "aligned" mean and why is it a desirable trait? Your posts are still vague or unspecific.

a lot of the roundtrips could be reduced with the examples I provided in this issue.

Saving roundtrips is desirable, but keeping the MPD protocol simple is also desirable. These two contradicting goals have to be balanced - it's a tradeoff, there's no ultimate truth, only opinions, and we have to draw a line somewhere. You want to move that line, but it's up to you to properly explain and convince others that your line is better than the current line, and that the new commands and worth the added complexity. So far, you have not even started explaining.

roizcorp commented 2 months ago

Thanks, I was not aware there was a line. I saw in the recent releases how Mpc starts to get more outputs about audio quality and priority to I figured there is an effort to align between the 2.

If anything I would go into pub-sub model, rather than request-response but this is not the discussion.

I'm not a developer, just a linux enthusiast.

I'm migrating from MPC into protocol (echo -e "command1\ncommand2" | nc mpdhost port) as it is not that easy to get Mpc to android phones. The market has few good MPD clients but few, and while I implement the MPD solution in various environments I try to answer some user experience and automation requirements.

Migrating into protocol has seriously been good to me as I could get all the information I need with a single bash command (and manually parsing the output into json format (I posted a feature request for json output, but you rejected it), and handle all the attributes when needed locally on the client side.

I appreciate the great work you are doing and can't wait to use MPD 0.24 and try the new Android app you are preparing (following the commits daily!)

MaxKellermann commented 2 months ago

Your latest post still doesn't try to explain anything. It's just a lot of text, but it leads nowhere. Since you didn't even answer my questions, let's just stop here.

roizcorp commented 2 months ago

(yes I thought this is where it is going, so I wrapped up with background and gratitude)