Skip to content
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

Remove time tracking code for Spotify playback #191

Closed
adamcik opened this issue Sep 11, 2012 · 6 comments · Fixed by #289
Closed

Remove time tracking code for Spotify playback #191

adamcik opened this issue Sep 11, 2012 · 6 comments · Fixed by #289
Assignees
Labels
A-audio Area: Audio layer
Milestone

Comments

@adamcik
Copy link
Member

adamcik commented Sep 11, 2012

See if we can switch to just plain old GStreamer timers for this.

@adamcik
Copy link
Member Author

adamcik commented Sep 30, 2012

I've been playing with figuring out how to make this work and so far figured out the following:

  • Our current get_position() code blocks using get_state(), this seems to cause a deadlock in the pipeline state changes so we end up blocking forever (only for spotify). The simple workaround is to have a timeout for get state. Also worth testing, is to see if this is really caused by lack of data flow.
  • Our appsrc needs to have format=time for it to work. Once this is place basic playback and time tracking works.
  • In order to get seeking to work we need stream-type=seekable - this implies that we start performing proper seeks on the pipeline, these trigger a callback which can instruct spotify to seek. (Note that blocking after seek with get_state() will only unblock once there is data flow.)
  • In order to setup format, stream-type and connect the seek-data callback our playback provider needs access to the newly created source via notify::source or we need to add some special handling to the audio module for appsrc instead of using just set_uri(). The second option would be something along the lines of calling a set_appsrc which in turn sets up the properties and callbacks as needed.

@adamcik
Copy link
Member Author

adamcik commented Sep 30, 2012

Most likely the set_appsrc variant is better in the sense that playback providers can stay clear of gstreamer internals and the likes. I would suggest that set_appsrc add a one off notify::source listener (i.e. returns False), this listener then sets up the format and stream-type and attaches a callback to the seek-data event. When the seek callback is called we need to call a method on the playback provider to make it seek, this needs to be seperate from the normal callback. An other option is to pass in the callback when set_appsrc is called.

Within this solution we might also want to look at storing the source when we get notified instead of looking it up each time and hoping it is still an appsrc. If we store i properly we can set it to None when appropriate and set it to a proper source only when we have an active appsrc. However, this is likely a separate bug.

@jodal
Copy link
Member

jodal commented Dec 21, 2012

I'm considering looking at this.

The above comments are still valid, and you don't have any partial code for this laying around?

@adamcik
Copy link
Member Author

adamcik commented Dec 21, 2012

https://gist.github.com/5e1451c53b05d4a5f16f has the demo app I created to play with this and figure out what is documented above, it also contains the latest work I did which was really just wiring in the seekable callback. Problem I ran into was really just deadlocks caused by get_state blocking for state change completion, when the state change can't complete without data flow. So most likely missing bit is really just figuring out where we shouldn't block, or block with a timeout and this will be done. But don't forget to test with both local and spotify :-)

@ghost ghost assigned jodal Dec 22, 2012
@jodal
Copy link
Member

jodal commented Dec 22, 2012

Thanks! I got GStreamer time tracking and GStreamer-based pause/resume working. Got some issues with seek. Will continue tomorrow.

@jodal
Copy link
Member

jodal commented Dec 24, 2012

This was fixed with the merge of #289.

@jodal jodal closed this as completed Dec 24, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-audio Area: Audio layer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants