[vlc-devel] Playlists, locking, and callbacks
polar.humenn at gmail.com
Mon Nov 17 17:12:01 CET 2008
On Sun, Nov 16, 2008 at 10:28 AM, Rémi Denis-Courmont <rem at videolan.org> wrote:
> 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.
Given the "style" already deployed, that would probably be the more
>> 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.
Yeah, it depends on what the call backs are "published" (or
documented) to do before some programmers take "advantage" of
undocumented semantics. Again, a documented callback API for playlists
should probably be the better approach.
>> 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).
I agree, and that's what I have implemented.
>> 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.
I'll clean them up as per notes above, separate them out from other
work I've been doing, and post the patches for each.
Thanks for commenting,
> Rémi Denis-Courmont
More information about the vlc-devel