Some fixes and enhancements about cue files in satellite setup #1780
Conversation
datasone
changed the title
|
I havn't yet read the actual code changes, but what I see from a first glance that all commit messages are bad, just like with your other PR #1779 |
datasone
force-pushed
the
satellite_cue_fixes
branch
from
95c7f63
to
9a597b8
Compare
|
The commit messages have been reworded for review. |
|
Did you see that the CI build failed? |
|
Sorry for didn't make that clear earlier, the CI build fails because of the requirement for newly added functions in libmpdclient PR isn't met. The pr in MPD repo adds more info into the MPD protocol (the MPD uses libmpdclient for retrieving info from server in With the log, I think the CI uses libmpdclient release directly. So the build will fail due to missing function definitions. |
|
But the MPD build must not fail if you have an older libmpdclient version. You need to add compile-time version checks. |
|
Thanks for the remind for code requirements, I will add necessary fixes soon. |
CUE virtual playlists is currently implemented by accessing ranges in file referenced by `real_uri` locally. But the `real_uri` is not included in the song info protocol, thus `ProxyDatabasePlugin` is not able to retreive this information and access the corresponding file, leads to not working cue tracks in satellite setup. This commit adds the `real_uri` field into server-printed song info, with the key name `RealUri`.
This commit reads `real_uri` property from `mpd_song` in libmpdclient, and set ProxySong's `real_uri` base on it. The commit solves the issue that cue virtual playlists not working in satellite setup.
CURLOPT_RANGE value requires the format "X-Y", where X or Y can be
omitted. The current value provided to libcurl in `CurlInputPlugin` is
only offset without the hyphen character and will cause HTTP error while
accessing song files with range. This commit fixes it by adding back the
hyphen and feed value formatted by `{offset}-` to libcurl.
Range `start` and `end` values in `mpd_song` (libmpdclient) is stored in seconds only, which makes range times in `ProxySong` truncated to seconds and leads to precision loss on satellite cue track range times. This commit reads the added `start_ms` and `end_ms` value from libmpdclient, which represent range start and end time in milliseconds. And uses them to set `start_time` and `end_time` of `ProxySong`.
…th `chunk->tag` While playing first track of cue file on satellite setup, the tag of currentsong will loss. This is due to `chunk->tag` overwriting song tag in `PlayerControl::PlayChunk`. In local cue files, `song_tag` is used to form `stream_tag` and merged into `chunk->tag`, but `song_tag` only works for local files. So the `chunk->tag` is decided by decoded tag from audio files, which in cue's case is the whole album file and doesn't contain track metadata. This commit fixes the issue by merging song's tag with `chunk->tag` (with `chunk->tag` prioritized) in `PlayerControl::LockUpdateSongTag`, only when the song's `end_time` is not zero. As the `end_time` will only be set on cue virtual songs or with `rangeid` command, i.e. songs represented by portion of a whole file, where metadata of the portion and whole file may differ. The impact to current behaviour is minimized: changes only happen when users use `rangeid` command on a remote song, and they expect that song tags in database should be completely overwritten with tags provided by remote stream instead of merged.
datasone
force-pushed
the
satellite_cue_fixes
branch
from
9a597b8
to
630d68d
Compare
|
Sorry for delaying till now about this pr, I haven't been able to get time on working on this. I understand that libmpdclient version detection must be present for preventing build failures, but the So is it necessary for me to get the libmpdclient pull request to be merged for this pr to continue being reviewed, or can I just assume that it can be merged, use the unreleased |
You can use the unreleased 2.23.0 for now. If this pull request is merged, I will timely merge the patch for libmpdclient. |
This pr contains a series of commits which fixes issues around cue files in satellite setup, and some enhancements related to cue files. As
ProxyDatabasePluginuseslibmpdclientfor retrieving information from server, a pull request has also been created inlibmpdclientrepository.Fixes
These commits add
real_uriinfo into printed song info, and add the ability to read it inlibmpdclientandProxyDatabasePlugin. CUE virtual playlists is currently implemented by accessingreal_urifile with range. But thereal_uriis not included in the protocol now, thusProxyDatabasePluginis not able to retreive this information, and cannot access the corresponding file. This should fix #907.CURLOPT_RANGEvalue requires the format "X-Y", where X or Y can be omitted. The current value provided to libcurl is onlyoffsetwithout the hyphen character and will cause HTTP error while accessing song files with range, the format string went wrong in 415de49, and this commit fixes it.Enhancements
Range
startandendvalues inmpd_songis stored in seconds only, which makes range times inProxySongtruncated to seconds. This leads to precision loss on cue tracks. These commits addsstart_msandend_ms, which represents range start and end in milliseconds, into libmpdclient parsing procedure.chunk->tagOn current code base (with patches above), while playing the first track of the cue virtual playlist on the satellite client, the
currentsongstatus will lose cue metadata. It's due to tag being replaced bytagofMusicChunkinPlayerControl::PlayChunk, which is produced inDecoderBridge::SubmitAudioandDecoderBridge::SubmitTag.In local cue files,
song_tagis used to form thestream_taginDecoderBridge::UpdateStreamTag, and then merged intochunk->tag. However,song_tagonly works for local files. While playing cue tracks in satellite client, the tags are replaced by decoded tags in the whole album audio file.This commit fixes this problem by merging song's tag with
chunk->tagwhen the song's end_time is not zero. In the code base, end_time is only set while copying from other song objects, loading from database, parsing cue and therangeidcommand. So this change will only affect behaviour when the user usesrangeidcommand to set range for a song in remote stream, and the user needs the tags from remote stream to replace tags in database instead of merging them. I think this is a much more rare scenario compared to the issue to be fixed.