Max Kellermann <[EMAIL PROTECTED]> wrote: > On 2008/09/27 07:24, Eric Wong <[EMAIL PROTECTED]> wrote: > > Neither notify nor condition are optimal for the one thread per audio > > output stuff. > > > > Instead, use one global condition variable and multiple mutexes (one mutex > > per output) and pthread_cond_broadcast(). > [...] > > playAudio()/flushAudioBuffer() will then call > > pthread_cond_broadcast(&global_ao_cond). I would do this after > > verifying that at least of the ao->mutex-es is unlocked; indicating that > > there's at least one playable output. > > I considered using pthread_cond_broadcast(), but postponed that > decision for now, since I am quite comfortable with having full > control over which audio outputs I am going to wake up. Looping with > pthread_cond_signal() isn't worse than looping to check if mutexes are > locked...
But you wouldn't know if a notify_signal() even succeeded in delivering a signal or not (I haven't fully read the rest of the output rewrite). And the eventual response could be a wakeup from much earlier notify_signal() call, couldn't it? Perhaps notify_signal() should return EBUSY if pending is already set? > > Currently, it seems that playAudio() still waits until all the outputs > > have played. This seems wrong to me. If some outputs are blocking > > others from completing, it should be skipped over (and drop audio) to > > not affect other outputs. > > That's right. That's were my patches are not feature complete yet. > I'm implementing one thing at a time, starting with sending all > outputs to a separate thread - further refinements will come, or > somebody else will implement them if I don't have the time soon > enough. At the current patch level, the infrastructure for threaded > audio outputs is there, but no "real" advantages yet. Adding a user-configurable async flag to each audio_output (defaulting to off for backwards compatibility) for playback could be useful. This way users can choose which outputs they value over others. enable/disable/close/open should always be async, though; just playback should be (optionally) synchronous. If a user only has one output enabled, then it should of course be synchronous. > > A note about notify: > > > > The pending flag in notify has always bugged me as it's totally > > unnecessary. pthread_cond_wait() atomically unlocks the mutex when it > > is called and starts waiting. This is for allowing a signalling > > thread to check to see if the waiter is signal-able. So attempting > > to lock the mutex that is passed to pthread_cond_wait is already > > enough to know if there's a thread waiting on that condition variable. > > That is correct if you rely on synchronous notification. I am not > sure yet if that is a good thing for MPD. I think my current approach > (asynchronous notifications) is better: send a signal to a thread, > which it will work on whenever it's ready to do so, and meanwhile the > calling thread can move on, do something else, or notify other > threads. You cannot do that without a "pending" flag, since the > pthread_cond_t itself is stateless, it forgets the signal if nobody > was listening. synchronous notification is much easier in some cases. It is much nicer for some "fast" things such as dropping the output buffer when skipping tracks; but I agree that async is much better in many cases, too... condition.c doesn't actually enforce synchronous or asynchronous operation: The 'pending' flag could just be the enum foo_action tied to a particular structure (condition.c being much more freeform does allow more places to screw it up, however :) > Just think about MPD sending data to 10 outputs, and one of them is > blocking, having its notify.mutex still locked, and the player thread > waits for that mutex: MPD will stall, it cannot send audio data to the > other 9 outputs - deadlock. With my asynchronous approach, MPD can > send the signal to all 10 threads, and see which one responds quickly, > disabling those who don't. I would just use pthread_mutex_trylock() and then broadcast if I get locks from the required set of outputs... I don't think automatically disabling outputs on timeouts/errors is good policy, though. We should make a best effort to send to an output unless a user disables it manually; including retrying the open/initialize (unless it's the only output enabled by the user). > You were right that my notify.c library contained a race condition, > which I was able to trigger during debugging. I am now (since patch > 58554e14) using notify's mutex solely to protect the "pending" flag. > Please review if you find another race in there.. No races, but one bug and the aforementioned issue: A definite bug: pthread_cond_wait is subject to spurious wakeups so you need to restart pthread_cond_wait if pending is not set. assert(notify->pending) is wrong after pthread_cond_wait. Again, there should be a way of preventing notify_signal() from clobbering/merging its own signals; so EBUSY or just assert(!pending)... -- Eric Wong ------------------------------------------------------------------------- This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100&url=/ _______________________________________________ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
