fix(mpv.go): Jukebox mode doesn't include MusicFold… #4067
Conversation
…er in mpv command - navidrome#4066 The call to createMPVCommand is not including the MusicFolder path in mpv command causing it to fail with file not found errors. Updated default command template and createMPVCommand to use additional substitution with conf.server.MusicFolder Signed-off-by: Pat <patso.oshea@gmail.com>
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 0609de3 in 47 seconds. Click for details.
- Reviewed
25lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. conf/configuration.go:458
- Draft comment:
The default mpv command template now uses a '%p/%f' pattern. Note that hardcoding '/' may cause issues on Windows or if MusicFolder already ends with a slash. Consider using a platform‐independent join (e.g. filepath.Join) or ensure proper formatting. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. core/playback/mpv/mpv.go:77
- Draft comment:
Addition of '%p' replacement in createMPVCommand correctly inserts MusicFolder. Be aware that simple string replacement may break if the folder or filename contains spaces. Consider using a more robust method (e.g. filepath.Join) for path construction. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_5jEcOfYghDHkuV9D
You can customize 
|
Apologies, first time attempting to contribute and I hadn't considered cross platform paths. Will try to push an update soon |
Updated |
…-mpv-command/4066
There was a problem hiding this comment.
Thanks for the contribution, check my comments.
By the way, I'm curious why the few folks out there that use the current Jukebox implementation didn't complain before.
@tcurdt sorry for the ping, but as you are the most vocal Jukebox user I know, do you get this error as well?
core/playback/mpv/mpv.go
Outdated
| @@ -74,7 +75,7 @@ func createMPVCommand(deviceName string, filename string, socketName string) []s | |||
| split := strings.Split(fixCmd(conf.Server.MPVCmdTemplate), " ") | |||
| for i, s := range split { | |||
| s = strings.ReplaceAll(s, "%d", deviceName) | |||
| s = strings.ReplaceAll(s, "%f", filename) | |||
| s = strings.ReplaceAll(s, "%f", filepath.Join(conf.Server.MusicFolder, filename)) | |||
There was a problem hiding this comment.
The correct way to do this is to call createMPVCommand with mf.AbsolutePath():
navidrome/core/playback/mpv/track.go
Line 37 in de0a089
There was a problem hiding this comment.
Thanks, first time looking at the the code of course. This makes more sense for the caller to be corrected. Will push an update
There was a problem hiding this comment.
@deluan, just pushed the update to use AbsolutePath() in the caller.
I did see in another issue that people noticed this problem and had solved it by prepending their MusicFolder path in the mpv command template itself as a workaround.
But I think @tcurdt is right about the problem using spaces to tokenize the command. That go shellwords could be the way to go, I might play around with that. But figured getting this change in for people might be good in the interim
| @@ -477,7 +477,7 @@ func setViperDefaults() { | |||
| viper.SetDefault("ignoredarticles", "The El La Los Las Le Les Os As O A") | |||
| viper.SetDefault("indexgroups", "A B C D E F G H I J K L M N O P Q R S T U V W X-Z(XYZ) [Unknown]([)") | |||
| viper.SetDefault("ffmpegpath", "") | |||
| viper.SetDefault("mpvcmdtemplate", "mpv --audio-device=%d --no-audio-display --pause %f --input-ipc-server=%s") | |||
| viper.SetDefault("mpvcmdtemplate", "mpv --audio-device=%d --no-audio-display %f --input-ipc-server=%s") | |||
There was a problem hiding this comment.
I don't know why the pause was there in the first place.
It was causing every song I started in jukebox mode to go straight into a paused state which didn't make sense. That's why I removed it
There was a problem hiding this comment.
What client are you using @patso23 ?
There was a problem hiding this comment.
@tcurdt DSub currently
No worries. I am fine to be pinged :) I had it working without this change (see the other discussion/ticket) but since I didn't get any traction I abandoned that route. Traction as in to properly rewrite it in Navidrome and to fix client support. This change might go in the right direction but it feels like putting lipstick on a (dead) pig. I am now using music assistant on top of navidrome which does the snapcast integration. I am still happy to help on fixing this, but this needs more work. Both on the server and the client side. |
|
@deluan I can confirm that these fixes work when applied directly via config. I put Navidrome jukebox away for awhile and relied on Mopidy3 if I really wanted local playback. With these changes though, it's at least good enough to kick off an album or playlist and move on. Jukebox.Devices = [
[ "snapcast", "snapcast" ],
]
Jukebox.Default = "snapcast"
Jukebox.Enabled = true
MPVPath = ""
MPVCmdTemplate = "mpv --no-audio-display /music/%f --input-ipc-server=%s --audio-channels=stereo --audio-samplerate=48000 --audio-format=s16 --ao=pcm --ao-pcm-file=/tmp/host/navidrome"Above is my config, which has the following major changes
To test my server, I'm using Jamstash on my computer and iSub/AVSub on my iPhone. For all of these apps the jukebox mode controls are not working as expected. Biggest flaws are not being able to track the playback state properly, or get a queue of what's playing. The error from the linked issue How hard could it be for me to add them myself, right? 😇 |
…usicfolder-in-mpv-command/4066
Jukebox mode doesn't include MusicFolder in mpv command in mpv command - #4066
The call to createMPVCommand is not including the MusicFolder path in mpv command causing it to fail with file not found errors.
Updated createMPVCommand to use filepath.Join with conf.server.MusicFolder