New Stuffs!

[IainCole]

I made some progress in some areas you might be interested in. I had to do some fun merging jiggery pokery to avoid pushing the changes you don't want (travis settings etc)

It has basic playlist functionality as well as some improvements to the audio playback system

[Floby]

wow! thank you very much! that's a lot of code to read, I'll do that tomorrow morning and see if it works as expected for me too.

Have you written addtionnal tests for your playlist object? If so, please do add them to the test suite if not, I'll try and write some.

Also can you explain a little what you changed in the audio playback processing? I ripped most of that code from the spotify examples and I'm not sure I can understand your changes.

Thanks again, I'll make sure to add your name as a major contributor when I merge this.

[IainCole]

The audio playback changes were two things really.

Firstly the data was coming out of libspotify too fast. node::Buffer objects were being created faster than they were being consumed by the Speaker, node was handling the backlog but it meant that there would be an ever increasing amount of audio data resulting in going out of memory eventually. Also it meant that "track-end" messages from libspotify were being emitted much earlier than expected (this is actually what got me looking at it in the first place).

To fix it I made Player.js inherit from stream.Readable so that it could deal with the backpressure from whatever you are piping to and sending that information back down to the audio handle loop (that's basically what pause_delivery is for) which stops taking stuff off the queue until Session_Player_Stream_Resume is called.

Once that was sorted I noticed that the memory usage just kept rising, so after a few hours of messing around (because I don't really know what I'm doing in v8 land) I ended up with a HandleScope around the code that calls the music delivery JS method which resulted in the free_music_data function actually being called and memory usage being kept stable.

I haven't done much in the way of testing yet as I'm still working out how everything works.

I'm also pretty new to git so I'm hoping that future commits / pull requests will be slightly more organised :P

[Floby]

it's ok to be new to git, I'll sort the commits by theme and see what works and what needs more testing, it's ok to to be new to v8, Scopes are hard to get right ^^ it's ok to be new to nodeunit, but it's one of the easiest testing frameworks out there, but I concede that testing libspotify was not the most straightofrward lib I had to test.

On 14 April 2013 00:08, Iain Cole notifications@github.com wrote:

The audio playback changes were two things really.

Firstly the data was coming out of libspotify too fast. node::Buffer objects were being created faster than they were being consumed by the Speaker, node was handling the backlog but it meant that there would be an ever increasing amount of audio data resulting in going out of memory eventually. Also it meant that "track-end" messages from libspotify were being emitted much earlier than expected (this is actually what got me looking at it in the first place).

To fix it I made Player.js inherit from stream.Readable so that it could deal with the backpressure from whatever you are piping to and sending that information back down to the audio handle loop (that's basically what pause_delivery is for) which stops taking stuff off the queue until Session_Player_Stream_Resume is called.

Once that was sorted I noticed that the memory usage just kept rising, so after a few hours of messing around (because I don't really know what I'm doing in v8 land) I ended up with a HandleScope around the code that calls the music delivery JS method which resulted in the free_music_data function actually being called and memory usage being kept stable.

I haven't done much in the way of testing yet as I'm still working out how everything works.

I'm also pretty new to git so I'm hoping that future commits / pull requests will be slightly more organised :P

— Reply to this email directly or view it on GitHubhttps://github.com/Floby/node-libspotify/pull/7#issuecomment-16341837 .

[19h]

Any progress on this PR?

[Floby]

Hey, Sorry I started a new job and couldn't quite make time to look at this. I promise I'll get too it soon.

On 20 May 2013 12:42, Kenan Sulayman notifications@github.com wrote:

Any progress on this PR?

— Reply to this email directly or view it on GitHubhttps://github.com/Floby/node-libspotify/pull/7#issuecomment-18141448 .

[19h]

Take your time. I have published a libspotify-dev package on npm merging Lains PR.

[Floby]

hey I've got some time today, I'll fix de broken tests and try and write some for the playlist API.

[ronaldevers]

Hey @KenanSulayman , could you please add a big note (red, blinking, marquee?) to your libspotify-dev package on npm stating what the relation to the libspotify package is? The current situation with two packages is very confusing. I had to find this thread to figure out what was going on. Or at least fork so you can list another git repo as source. Then people can figure out what's going on through git.

[Floby]

Hopefully this should need to get unpublished soon ;)

On 28 May 2013 22:46, Ronald Evers notifications@github.com wrote:

Hey @KenanSulayman https://github.com/KenanSulayman , could you please add a big note (red, blinking, marquee?) to your libspotify-dev package on npm stating what the relation to the libspotify package is? The current situation with two packages is very confusing. I had to find this thread to figure out what was going on. Or at least fork so you can list another git repo as source. Then people can figure out what's going on through git.

— Reply to this email directly or view it on GitHubhttps://github.com/Floby/node-libspotify/pull/7#issuecomment-18578900 .

[19h]

Sorry, I was quite impatient. Description now includes: "[INFO: THIS MERGES PLAYLIST SUPPORT AND IMPROVED AUDIO PROCESSING BY IainCole]". Under specific circumstances this is quite useful.

@Floby Exceptions should have taken a look at; currently, node dies [from C++ land] when a function of specific type (Album, Artist, Playlist et al.) is used to query a url of different type. (e.g. the album url of Random Access Memories by Daft Punk is used as parameter to query the Artist [Album->Artist]). We could encounter this issue by obtaining the type of the given url and query its information, when possible. (Album->Artist should be possible, Artist->Album should throw an exception, etc.)

[Floby]

This can be discussed in another issue or PR.

On 28 May 2013 23:42, Kenan Sulayman notifications@github.com wrote:

Sorry, I was quite impatient. Description now includes: "[INFO: THIS MERGES PLAYLIST SUPPORT AND IMPROVED AUDIO PROCESSING BY IainCole]". Under specific circumstances this is quite useful.

@Floby https://github.com/Floby Exceptions should have taken a look at; currently, node dies [from C++ land] when a function of specific type (Album, Artist, Playlist et al.) is used to query a url of different type. (e.g. the album url of Random Access Memories by Daft Punk is used as parameter to query the Artist [Album->Artist]). We could encounter this issue by obtaining the type of the given url and query its information, when possible. (Album->Artist should be possible, Artist->Album should throw an exception, etc.)

— Reply to this email directly or view it on GitHubhttps://github.com/Floby/node-libspotify/pull/7#issuecomment-18582295 .

[Floby]

This pull request has been merged with minimal API differences. I plan to make some changes here and there and add some other features from the playlist subsystem.

It's published as 0.2.1

[19h]

Okay, libspotify-dev is unpublished. I'll open another issue for now (can be joined later with an according PR).

[Floby]

thanks :)

On 28 May 2013 23:56, Kenan Sulayman notifications@github.com wrote:

Okay, libspotify-dev is unpublished. I'll open another issue for now (can be joined later with an according PR).

— Reply to this email directly or view it on GitHubhttps://github.com/Floby/node-libspotify/pull/7#issuecomment-18583040 .