bug(core/playback/mpv): jukebox mode under windows - #2767 #2774
Conversation
Use named pipe for socket path under windows during mpv playback, change function name, unexport function Signed-off-by: apkatsikas <apkatsikas@gmail.com>
|
Download the artifacts for this pull request: |
Fix typo Signed-off-by: apkatsikas <apkatsikas@gmail.com>
|
I noticed two errors being emitted when a track ends and goes to next queued during Jukebox play on Windows, that do not seem to affect playback experience, but are probably specific to how Named Pipes are treated in Windows:
Neither of these errors is affecting the user experience (to my knowledge) but it's not great to throw these errors on Windows due to its special behavior with Named Pipes. The question is - do we want to "fix" these issues in this PR or handle them later? |
|
Hey, thanks! I'll take a look and get back to you as soon as I can! |
|
The PR looks totally sensible to me, although i haven't compiled it under Windows. @apkatsikas if you tell/point me on how to build on windows I'll doublecheck for you. Windows is not my Hometurf ... Regarding the Socket close issue my cut is to handle/fix this within this bug since it's so related. |
makes sense to me. i can rebase and fix all instances outlined soon. in the mean time, if you want to try the build, you can download the binaries.zip created by the GitHub Actions bot above
https://nightly.link/navidrome/navidrome/actions/artifacts/1152903166.zip |
Early return for Close on Windows Signed-off-by: apkatsikas <apkatsikas@gmail.com>
Merge master and move logic into utils function Signed-off-by: apkatsikas <apkatsikas@gmail.com>
Update import and run prettier Signed-off-by: apkatsikas <apkatsikas@gmail.com>
| `https://www.last.fm/api/auth/?api_key=${localStorage.getItem('lastfm-apikey')}&cb=${callbackUrl}`, | ||
| `https://www.last.fm/api/auth/?api_key=${localStorage.getItem( | ||
| 'lastfm-apikey', | ||
| )}&cb=${callbackUrl}`, |
There was a problem hiding this comment.
Was required to run prettier before I could push due to git hooks.
Update function name Signed-off-by: apkatsikas <apkatsikas@gmail.com>
core/playback/mpv/track.go
Outdated
| @@ -104,6 +105,11 @@ func (t *MpvTrack) Pause() { | |||
| } | |||
|
|
|||
| func (t *MpvTrack) Close() { | |||
| // Windows automatically handles closing | |||
| // and cleaning up named pipe | |||
| if runtime.GOOS == "windows" { | |||
There was a problem hiding this comment.
It is not a good practice to use runtime.GOOS in your files (unless it is a really small check). Usually we create one file for each variation and use build tags to control which version to use.
For an example, see scanner/metadata/taglib/get_filename.go and scanner/metadata/taglib/get_filename_win.go
In your case I'd have two Close() methods in different files, one for windows (do nothing) and the other for all other platforms with the actual code.
Let me know if it makes sense to you or if you want me to do that.
There was a problem hiding this comment.
yeah was wondering about this and hoping to get feedback on what best practice is for this project. makes sense and thank you for the examples - will give it a shot and report back
utils/files.go
Outdated
|
|
||
| "github.com/google/uuid" | ||
| ) | ||
|
|
||
| func TempFileName(prefix, suffix string) string { | ||
| return filepath.Join(os.TempDir(), prefix+uuid.NewString()+suffix) | ||
| func RandomSocketOrFileName(prefix, suffix string) string { |
There was a problem hiding this comment.
I rather leave this function (TempFileName) alone and create a SocketName in the core/playback/mpv package, also selectable by build tags
Create track_close files for both platforms and move MpvTrack Close into new file Signed-off-by: apkatsikas <apkatsikas@gmail.com>
Create SocketName function for both platforms, restore name of TempFileName Signed-off-by: apkatsikas <apkatsikas@gmail.com>
Add missing params to SocketName on windows Signed-off-by: apkatsikas <apkatsikas@gmail.com>
Closes
#2767
Description
Fixes Jukebox mode when running under Windows.
Windows needs to use a named pipe instead of a file in the temp folder for the socket with mpv. See https://mpv.io/manual/master#using-mpv-from-other-programs-or-scripts which states regarding
--input-ipc-serverflag for script controlThis gives you access to the JSON IPC over unix domain sockets (or named pipes on Windows).See Windows docs on Named Pipes - https://learn.microsoft.com/en-us/windows/win32/ipc/pipe-names
This is also expounded on here - https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createnamedpipea
Changes
Tested regular playback and Jukebox mode via
rest/jukeboxControlendpoint, on Windows and Linux.