Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Some fixes and enhancements about cue files in satellite setup #1780

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Some fixes and enhancements about cue files in satellite setup #1780

wants to merge 5 commits into from

Conversation

datasone
Copy link
Contributor

@datasone datasone commented Apr 11, 2023 •
edited
Loading

This pr contains a series of commits which fixes issues around cue files in satellite setup, and some enhancements related to cue files. As ProxyDatabasePlugin uses libmpdclient for retrieving information from server, a pull request has also been created in libmpdclient repository.

Fixes

  • SongPrint: include real_uri in song_print_info
  • db/ProxyDatabasePlugin: read real_uri from server's song info
  • (libmpdclient) Add real_uri into mpd_song

These commits add real_uri info into printed song info, and add the ability to read it in libmpdclient and ProxyDatabasePlugin. CUE virtual playlists is currently implemented by accessing real_uri file with range. But the real_uri is not included in the protocol now, thus ProxyDatabasePlugin is not able to retreive this information, and cannot access the corresponding file. This should fix #907.

  • input/CurlInputPlugin: fix malformed value for HTTP Range header

CURLOPT_RANGE value requires the format "X-Y", where X or Y can be omitted. The current value provided to libcurl is only offset without 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

  • db/ProxyDatabasePlugin: read millisecond values for range start and end
  • (libmpdclient) Add range start and end in milliseconds into mpd_song

Range start and end values in mpd_song is stored in seconds only, which makes range times in ProxySong truncated to seconds. This leads to precision loss on cue tracks. These commits adds start_ms and end_ms, which represents range start and end in milliseconds, into libmpdclient parsing procedure.

  • player/Thread: merge tag for songs have range instead of replacing with chunk->tag

On current code base (with patches above), while playing the first track of the cue virtual playlist on the satellite client, the currentsong status will lose cue metadata. It's due to tag being replaced by tag of MusicChunk in PlayerControl::PlayChunk, which is produced in DecoderBridge::SubmitAudio and DecoderBridge::SubmitTag.

In local cue files, song_tag is used to form the stream_tag in DecoderBridge::UpdateStreamTag, and then merged into chunk->tag. However, song_tag only 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->tag when 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 the rangeid command. So this change will only affect behaviour when the user uses rangeid command 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.

@datasone datasone mentioned this pull request Apr 11, 2023
Open
@datasone datasone changed the title SongPrint: include real_uri in song_print_info Some fixes and enhancements about cue files in satellite setup Apr 11, 2023
@MaxKellermann
Copy link
Member

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 datasone force-pushed the satellite_cue_fixes branch from 95c7f63 to 9a597b8 Compare April 12, 2023 14:33
@datasone
Copy link
Contributor Author

The commit messages have been reworded for review.

@MaxKellermann
Copy link
Member

Did you see that the CI build failed?

@datasone
Copy link
Contributor Author

datasone commented Jun 1, 2023

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 real_uri field and start/end time fields in milliseconds), and utilizes them to achieve working cue files and more precise cue tracks in satellite setup.

MPD uses libmpdclient for retrieving info from server in ProxyDatabase, and three functions: mpd_song_get_real_uri, mpd_song_get_start_ms, mpd_song_get_end_ms are added into libmpdclient (with the separate PR) for getting corresponding information.

With the log, I think the CI uses libmpdclient release directly. So the build will fail due to missing function definitions.

@MaxKellermann
Copy link
Member

But the MPD build must not fail if you have an older libmpdclient version. You need to add compile-time version checks.

@datasone
Copy link
Contributor Author

datasone commented Jun 2, 2023

Thanks for the remind for code requirements, I will add necessary fixes soon.

@datasone
SongPrint: include real_uri in song_print_info
48bdcd1 
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`.
@datasone
db/ProxyDatabasePlugin: read real_uri from server's song info
32fbdb7 
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.
@datasone
input/CurlInputPlugin: fix malformed value for HTTP Range header
495f453 
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.
@datasone
db/ProxyDatabasePlugin: read millisecond values for range start and end
0f7ee06 
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`.
@datasone
player/Thread: merge tag for songs have range instead of replacing wi…
630d68d 
…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 datasone force-pushed the satellite_cue_fixes branch from 9a597b8 to 630d68d Compare August 16, 2023 13:55
@datasone
Copy link
Contributor Author

datasone commented Aug 16, 2023 •
edited
Loading

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 LIBMPDCLIENT_CHECK_VERSION macro requires a version number for checking. And code in this pr requires some additional features in libmpdclient to work, which is introduced in a pr on that repository.

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 2.21.0 libmpdclient version number to pass the MPD CI build, and get those two prs into the review process?

@jcorporation
Copy link
Member

So is it necessary for me to get the MusicPlayerDaemon/libmpdclient#100 to be merged for this pr to continue being reviewed, or can I just assume that it can be merged, use the unreleased 2.21.0 libmpdclient version number to pass the MPD CI build, and get those two prs into the review process?

You can use the unreleased 2.23.0 for now. If this pull request is merged, I will timely merge the patch for libmpdclient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@datasone @MaxKellermann @jcorporation