[vlc-devel] Playlists, locking, and callbacks

[Polar Humenn]

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().

I agree.

>> 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
consistent  approach.

>> 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,
Cheers,
-Polar

> --
> Rémi Denis-Courmont
>