-
Notifications
You must be signed in to change notification settings - Fork 37
Add metadata to fifo pipe output #203
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.
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
base: master
Are you sure you want to change the base?
Conversation
|
This is the config.yml I have been using: |
devgianlu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First review, I did not test it and generally glazed over some things.
Thank you for your effort!
cmd/daemon/controls.go
Outdated
| "google.golang.org/protobuf/proto" | ||
| ) | ||
|
|
||
| // Update extractMetadataFromStream method signature: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tautological comment
cmd/daemon/controls.go
Outdated
| } | ||
| } | ||
|
|
||
| return title, artist, album, trackID, duration, artworkURL, artworkData |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not be returning that many values
cmd/daemon/controls.go
Outdated
| /* | ||
| if p.primaryStream != nil { | ||
| title, artist, album, trackID, duration, artworkURL := p.extractMetadataFromStream(p.primaryStream) | ||
| p.app.log.Debugf("Sending metadata: %s by %s", title, artist) | ||
| p.UpdateTrack(title, artist, album, trackID, duration, true, artworkURL) // true = playing | ||
| } | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be definitely removed?
|
|
||
| p.UpdatePlayingState(true) | ||
|
|
||
| // Add this line to update position on resume |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tautological comment
cmd/daemon/controls.go
Outdated
|
|
||
| p.sess.Events().PostPrimaryStreamLoad(p.primaryStream, paused) | ||
|
|
||
| // In loadCurrentTrack method: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tautological comment
| drop bool | ||
| } | ||
|
|
||
| // Update MetadataCallback interface: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tautological comment
.gitignore
Outdated
| /daemon | ||
| /go-librespot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess these are because you are cross-compiling, but they are a bit misleading
metadata/fifo.go
Outdated
|
|
||
| _, err := fm.pipe.Write(data) | ||
| if err != nil { | ||
| // Close and attempt to reopen on error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem reconnection happens on error
| switch fm.format { | ||
| // case "json": | ||
| // data = metadata.ToJSONFormat() | ||
| case "xml": // ADD THIS CASE | ||
| data = metadata.ToXMLFormat() | ||
| //default: // "dacp" | ||
| // data = metadata.ToDACPFormat() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be supported right?
metadata/player_wrapper.go
Outdated
| } | ||
|
|
||
| // Update UpdateTrack method signature: | ||
| func (pm *PlayerMetadata) UpdateTrack(title, artist, album, trackID string, duration time.Duration, playing bool, artworkURL string, artworkData []byte) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we can use a struct here
4079b8f to
f4c58a3
Compare
- Remove tautological comments throughout codebase - Refactor UpdateTrack to use TrackUpdateInfo struct instead of 8 parameters - Refactor extractMetadataFromStream to return struct instead of 7 values - Support all metadata format cases (JSON, XML, DACP) in FIFO manager - Fix misleading reconnection comment - Clean up merge conflicts and duplicate function definitions
- Add variable declarations in extractMetadataFromStream - Update AppPlayer.UpdateTrack to accept TrackUpdateInfo struct - Remove duplicate timer initialization in main.go - Change FIFO startup error to warning - Handle EEXIST gracefully in FIFO creation - Add binaries to .gitignore
|
Hi @devgianlu, I've addressed all the review feedback: ✅ Removed tautological comments throughout the codebase The code now compiles successfully and I've tested it in my Home Assistant addon with OwnTone - metadata (including track info, artwork, and position updates) is flowing correctly. Let me know if there's anything else you'd like me to change! |
Add metadata to fifo pipe output as requested in: Add metadata output to a pipe as it is in librespot-java #157
Functionally this works very well showing track info, duration and timing, and artwork when piped into owntone and the Home Assistant dashboard when running when running in an addon.
I used AI to help write it and don't see any issues at this point. This is my first pull request so I expect there will be nits and other issues, but I will do will what I can to help push this forward.