[vlc-devel] Playlists, locking, and callbacks

[Rémi Denis-Courmont]

On Friday 14 November 2008 19:37:52 Polar Humenn, you wrote:
> It would seem that the playlist should be forcedly unlocked before
> going into a "var_Set" call. I have done this all around the playlist
> implementation with great success.

Might very well be. Variable callbacks are a big mess at the moment.

We should check if it makes sense for this variable to be set with the 
playlist (un)locked. It might also be worse checking if a dedicated API would 
not make more sense than var_Set().

> There is also one other function I had to create, which was
> playlist_GetCurrentInputLocked(). This is because playlist_GetCurrentInput
> was being called in in places where the playlist was already locked, such as
> in variable callbacks, causing errors.
> There might be a more elegant way to do this, perhaps 
> with a boolean value as with some of the other playlist functions,
> stipulating whether the playlist is already locked.

Not elegant, but looks valid.

> Are there any comments on/caveats with this approach?

A boolean flag would be an alternative. Pierre d'H. would be a more 
knowledgeable person than I.

> It looks like people have been burned by this problem before as some
> "var_Set" calls have been wrapped with a PL_UNLOCK and PL_LOCK, but
> obviously not all of them.

Similarly, it would have been worth checking that it makes sense to unlock - 
does this not introduce some race condition/consistency problems.

> Another point, which is more systemic, is that in the variables.c
> implementation. The callback list is iterated directly. This approach
> forces the need (and correctly so) to lock the variable to prevent
> changes to the callback list through the use of GetUnused in
> var_AddCallback and var_DelCallback functions. This prevents such
> simple dynamic behavior has a a callback removing itself.

The variable callback stuff looks quite messy to me, not using locks and 
condition variables properly (e.g. waiting for pending callbacks to complete 
before removing callbacks).

> Why are the callbacks iterated through from last to first? Is that a
> design decision? Or just a desire to not use two variables, and a
> comparison of two numbers, in contrast to  just a comparison against
> zero? Is the "public" documented behavior supposed to be "undefined"?

Probably the only ones who know is Sam, and he hasn't been very active lately.

> Anyway, I can supply patches or, if allowed, can git them through.
> Just looking to give back to the project.

It won't hurt to post them here.

-- 
Rémi Denis-Courmont