GNOME Bugzilla – Bug 607226
Disallow setting the playbin uri property in state >= PAUSED
Last modified: 2010-02-05 01:33:17 UTC
Created attachment 151607 [details] [review] proposed patch see subject, patch attached
What is the non-deterministic behaviour you're talking about? Setting the URI in >= PAUSED will make playbin2 switch to the newly set URI after the currently playing one has finished. Everything else is a bug. Allowing to set a new URI in >= PAUSED is part of the gapless track switches support in playbin2.
I thought it was only supported to do this in playing state when done from within the about-to-finish callback. Seems a bit weird to me API-wise to allow it generally.
Well, there are no checks if it's called from a a-t-f callback :) But you're right, it's a bit weird API-wise the way it is now, would make more sense to only allow it in < PAUSED and inside about-to-finish.
I'll prepare a patch for that later today.
commit 7335ce5d3e03c126a417a721571cb6f3af136ecf Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Mon Jan 18 09:30:18 2010 +0100 playbin2: Only allow to set the URIs in states <= READY or from an about-to- Changing the URIs in a state > READY results in unexpected behaviour, i.e. the new URIs are only used after the current track has finished. Fixes bug #607226.
This breaks applications (apparently including some patches to make banshee do gapless). We could warn if you change the uri while about-to-finish isn't being processed, but not disallow it. We should NOT use a thread-private variable for this - it should be perfectly ok to (as banshee does) block the thread processing about-to-finish until the main thread sets the next URI. But really, it's ok to change the URI at any time - it has had clear semantics in the past (sets the next URI to play), and there's no need to change those. This needs reverting before the next release. Marking as blocker.
> This breaks applications Which applications in which versions? Please be specific. > (apparently including some patches to make banshee do > gapless). "patches"? Well, given that playbin2's API is still declared unstable I'm not so bothered about "patches" being broken here tbh, but if it would seriously break existing apps as distributed, that wouldn't be so nice I guess. > We could warn if you change the uri while about-to-finish isn't being > processed, but not disallow it. We should decide what API we want to support, and then implement that while the API can still be changed. We should only warn if we don't want to allow the property being used like this. > We should NOT use a thread-private variable for this - it should be perfectly > ok to (as banshee does) block the thread processing about-to-finish until the > main thread sets the next URI. I agree. Why does it do things like that btw? > But really, it's ok to change the URI at any time - it has had clear > semantics in the past (sets the next URI to play), and > there's no need to change those. It has worked in the past like that (minus the issues Sjoerd reported, at least), that doesn't necessarily mean there were 'clear semantics'. I don't think this was ever documented to work like that. I think using GObject properties like this is extremely screwy API. However, alternatives don't seem to be much nicer either, and we don't have a playlist API, so I guess the best course of action is to allow this for now and revert the patch.
Ok, reverted this again, since consensus seems to be that this is the least bad thing to do. Sjoerd: it would still be nice to fix those issues you ran into though - could you provide more details on how/when/why that happened? commit 729b6da76a7985ed48594cde78ba1c675ae47472 Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Thu Feb 4 18:32:48 2010 +0000 Revert "playbin2: Only allow to set the URIs in states <= READY or from an about-to-finish signal handler" This reverts commit 7335ce5d3e03c126a417a721571cb6f3af136ecf. Support abusing the uri property to configure the next uri to play outside of the about-to-finish handler for the time being after all. We also shouldn't use thread private structures for this, since it should be possible to block the thread that emitted about-to-finish while the main thread sets the uri property. See #607226.
> > We should NOT use a thread-private variable for this - it should be perfectly > > ok to (as banshee does) block the thread processing about-to-finish until the > > main thread sets the next URI. > > I agree. Why does it do things like that btw? As the developer of the Banshee gapless branch, this is all that I actually care about. I don't mind not being able to set uri with STATE > READY at arbitrary times, but I do care about needing to set it in the same thread as the about-to-finish callback.
> (...) but I do care about needing to set it in the same thread as > the about-to-finish callback. Hrm, that should have worked even with the patch applied. Did it not? Or did you mean that you care about setting it from a *different* thread than the about-to-finish callback (which is what Mike was talking about, I think)? Setting it from the about-to-finish callback directly, so from the same thread, is/was always supposed to work as far as I know.
Ok, I've been unclear. I meant “I don't care, as long as I'm able to set uri from a different thread while the about-to-finish handler is blocked”. In Banshee it's much easier to be able to block the about-to-finish handler and then set the uri from the main thread.