search

jishi / node-sonos-discovery

Simplified framework for Sonos built on node.js
MIT License
146 stars 75 forks source link

fix parse meta data, add high res covers (if available) to current #46

Closed neophob closed 8 years ago

neophob commented 8 years ago

and next track

jishi commented 8 years ago

Hi, but this seems to reintroduce some of the code that I actually removed? The code in parseMetaDataXML is unecessary since it will add that if the fetching of high res urls fails for some reason, as a fallback. It may help if you explain the problem that you are seeing, I'm feeling a bit dense right now, to be honest :)

neophob commented 8 years ago

Ahh I missed the commit with message "remove console.log" - where you actually removed the part I re added. I would like to add high res cover support for the next song and for the playlist entries. I'll try to add support for the music services in the parseMetaDataXML

jishi commented 8 years ago

Next song is probably fine to do it for, but if you start doing request for each entry in a playlist, you will probably end up triggering the request throttling that most services apply and it will break. This would probably need to be lazily loaded, and one way to achieve that would be to have the album art resolution to an extra api endpoint for you to utilize (send in track uri, receive an album art uri back).

neophob commented 8 years ago

Good point yes (throttling). So for the playlist we could still query sonos itself using the lowres image - which is in this pull request.

jishi commented 8 years ago

Hi again, I did an alternative fix for this. I did a small optimization for when you fetch cover art from the players for a queue (although I'm not sure how you retrieved the queue previously), and it tries to utilize all players on your network. This makes a huge difference if you have more than 2-3 players in my experience (apparently, one player can only use 4 threads for requests, hence, only 4 parallell request can wait on IO in best case).

Try it out and let me know if this works. Closing this pull request.

neophob commented 8 years ago

thanks - works like a charm!