[vlc-devel] Playlists, locking, and callbacks

[Polar Humenn]

Hi
I'm new to VLC, and have been doing some developement for a module for
a small personal project I am working on.

I have found two things that I think are problems with locking of the playlists.

The first problem is that there are calls made to "var_Set" that
trigger callbacks, on playlists. This is problematic if the playlist
is locked during these call backs.  And is quite readily seen in the
DBUS implementation when you --enable-debug, which enables ERRORCHECK
on the vlc_mutexes. Dbus initialization fails with a mutex error
because a function sets a variable while the playlist is locked and
the callback tries to lock the playlist through some other deeper
functions.

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

Are there any comments on/caveats with this 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.

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.

I've taken care of this problem by copying the callback list to the
stack and iterating through that, and replacing the calls to GetUnused
with the simple call to Lookup in var_AddCallback and var_DelCallback.
Works well, and I don't think it alters the semantics because you
couldn't manipulate the callback list while in a callback for that
variable. Now you can manipulate the callback lists from within a
callback, but you have to understand that any changes made to the
callback list will not be noticed until after the current execution of
the callback list is complete. Which leads me to another question.

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"?

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

Cheers,
-Dr. Polar