Make Sink work - Githubissues

devgianlu / librespot-android

A demo app that runs librespot-java on Android

33 stars 13 forks source link

Make Sink work #2

Closed mitschwimmer closed 3 years ago

mitschwimmer commented 3 years ago

Hey @devgianlu, I finally found some time to make this work. I have a couple of questions and would like your review which is why I am not just merging this. I will place my Implementation questions/remarks as comments on the PR. Regarding testing, I want to transform the MainActivity of the app-module into a couple of emulator/device Integration Tests which will be expecting credentials as environment variables. Would you like to keep the app-module as an implementation example?

mitschwimmer commented 3 years ago

track.stop() in AdnroidSinkOutput does not immediately stop but will play the remaining buffer first. If we want it to stop right away we need to implement .stop() by calling .pause() and .flush() in the method. What would you expect .stop() to do? While we are on the topic of expactations, what would you expoect in .close() over .release()?

devgianlu commented 3 years ago

track.stop() in AdnroidSinkOutput does not immediately stop but will play the remaining buffer first. If we want it to stop right away we need to implement .stop() by calling .pause() and .flush() in the method. What would you expect .stop() to do? While we are on the topic of expactations, what would you expoect in .close() over .release()?

These methods have the same functionality as the ones in javax.sound.sampled.DataLine. Precisely:

stop() is called when the playback pauses so it should stop immediately, but not discard the buffer data
close() should stop the playback, free the line and associated resources
release() could be a no-op in this case because it is called 20 seconds after the line has been paused to allow other programs to use it on systems that support one line only. I have just realized that in the MixerOutput implementation it has the same function as close(), but it may be worth keeping

devgianlu commented 3 years ago

Regarding testing, I want to transform the MainActivity of the app-module into a couple of emulator/device Integration Tests which will be expecting credentials as environment variables. Would you like to keep the app-module as an implementation example?

I think having an example activity that one can just run on its device would be a nice proof of concept, possibly adding some UI to play/pause (I can do that). Having tests would be an additional bonus since it is easy to break this if non-Android classes are used in the librespot-java source code.

devgianlu commented 3 years ago

@mitschwimmer What's your opinion on #1? I think it could be really useful, I am in the process of merging it

I have made it the default one on master (hence the merge conflicts).

mitschwimmer commented 3 years ago

@mitschwimmer What's your opinion on #1? I think it could be really useful, I am in the process of merging it

I have made it the default one on master (hence the merge conflicts).

I am full of respect for someone using the NDK :-) However, I can't say much regarding the actual implementation, more so on a meta level. Sparing cpu cycles is great, therefore I am all in on a more efficient decoder implementation. I was just wondering; can't we even use one of Android's own decoder facilities?

devgianlu commented 3 years ago

can't we even use one of Android's own decoder facilities?

Something like https://developer.android.com/reference/android/media/MediaCodec ?

mitschwimmer commented 3 years ago

Yes, that should decode Ogg Vorbis (among others) and it offers buffers to work with.

devgianlu commented 3 years ago

We can definitely have another module for that one too. Giving multiple options won't hurt. We can eventually make some benchmarks (?).

Let me know if you want to take care of it or I'll do it.

mitschwimmer commented 3 years ago

Please go ahead :grin: Are you fine with me opening issues for (even small) todos? It will make discussions and coordination easier.

devgianlu commented 3 years ago

Absolutely, I love organization.

funtax commented 3 years ago

Hey there, I am looking forward to your ideas & implementations. I darkly remember that I had implemented (or at least tried to) Vorbis-decoding via Android's native MediaCodec years ago (~2016). But either it didn't work, the minimum compile level didn't match at this time or something else. Would be interesting if this could be used now down to this project's current minimum API-level.