Playqueue IDs are wrong

[wolfmanx]

After clearing the playqueue, IDs are starting always at 1. plchangesposid does not report the actual changes to previous versions.

For clients that evaluate this information to make incremental changes, this leads to strange display malfunctions.

[ejurgensen]

Err, is this an mpd issue? What clients are you referring to?

[chme]

Yes, this is a mpd client specific issue.

After clearing the playqueue, IDs are starting always at 1.

@wolfmanx To prevent reused queue item ids, you can define the id column of the queue table as an autoincrement (https://sqlite.org/autoinc.html)

In db_init.c replace

#define T_QUEUE                             \
  "CREATE TABLE IF NOT EXISTS queue ("                  \
  "   id                  INTEGER PRIMARY KEY NOT NULL,"        \

with

#define T_QUEUE                             \
  "CREATE TABLE IF NOT EXISTS queue ("                  \
  "   id                  INTEGER PRIMARY KEY AUTOINCREMENT,"       \

The song3.db file needs to be deleted to get the tables recreated (alternatively you can manually drop and recreate the queue table in sqlite3).

Does this solve the "display malfunctions"?

[wolfmanx]

@chme It sure does work. I specifically tested it by resetting the changes I made to cantata. So, why isn't this in the code? (As it is in fact an mpd server issue not a client issue.)

The manual drop/create is not a good workaround for genpop, since "COLLATE DAAP" is unknown outside forked-daapd.

Granted that the specification is pretty vague regarding queue IDs (i.e. no sufficient specification is given at all), but really none of the queue commands using IDs make any sense with reusable IDs. A client with outdated playlist information using an outdated song queue ID should get an ACK error instead of playing a totally different song than the one displayed to the user. Since the race condition of another client changing the playlist between one client obtaining the song IDs and then using the song ID in a command cannot be avoided, queue IDs cannot be reusable.

Oops, I'm ranting, sorry :) It's just that it took me a couple of hours to find this bug.

[chme]

Yes, this is a mpd client specific issue.

This was meant as short form for "It is an issue in forked-daapd, that only affects mpd clients" :-)

So, why isn't this in the code?

The clients i tested, worked fine without unique ids and no one until now reported that there is an issue.

Reading the sqlite3 doc use of AUTOINCREMENT gives a small performance hit and the possible SQLITE_FULL error should be addressed. I'll prepare a fix, i have also an idea how to report only the changed items.

[wolfmanx]

This was meant as short form for "It is an issue in forked-daapd, that only affects mpd clients" :-)

Yeah, I thought so (after a short moment) ;-)

Reporting the entire state of the queue as changed is not wrong. The client code I saw just ignores IDs that are already in the queue, so the command response serves its purpose as it is. The limit cantata imposes on the number of changes is 1000. In that case the entire playlist is requested.

So it does not seem to be a grave limitation, but I don't know the average queue length ...