[vlc-devel] commit: Qt4: store input_item_t* in plitem and handle metadata in model ( Ilkka Ollakka )

[Jakob Leben]

I am very sorry, but don't you see the simple problem, that there is time
when the qt view is walking around with a pointer to a playlist item and the
global playlist lock CAN NOT BE HELD. Because you must not lock in a
function without unlocking and then expect that sooner or later some other
function will be called which will unlock, it's insane.

So, there is this NECESSARY function, that must be implemented in playlist
model and it goes like this:

QModelIndex *PLModel::index* ( int row, int col, QModelIndex parent )
{
    *PL_LOCK;*
    playlist_item_t * item;
    item = (...)
    *PL_UNLOCK;*
    return ( createIndex ( row, col, *playlist_item_t *item* ) );
}

So IF we are going to store the playlist_item_t * in the QModelIndex, and
this discussion has always been about that if, then WHATEVER could happen
until the view or whoever obtained that QModelIndex calls our thread back,
calling let's say "QModelIndex PLModel::parent ( QModelIndex index )". So it
might happen that that function goes like this:

QModelIndex *PLModel::parent* ( QModelIndex index )
{
   *PL_LOCK;*
   playlist_item_t *p_item = static_cast<playlist_item_t*>(
index.internalPointer( ) );
   playlist_item_t *p_parent = p_item->p_parent; // OOOOOOPS, the
p_item->p_parent pointer is not valid any more!!!
   int row = (....); int col = (....);
   *PL_UNLOCK;*
   return ( createIndex ( row, col, p_parent ) );
}

You see, the locks don't help as a frak, because there were changes that
made invalid that p_parent field of the playlist_item_t* stored in
QModelIndex, and those changes happened while we COULDN'T have held the
lock! There is this gap from the qt view's point of view:

....
QAbstractItemModel * model = new PLModel (.... );
QModelIndex an_index = *model->index* ( 1, 0, whatever_index );
*/* gaaaaa a a a a a a a a aaaaaaaaaaaaap *in which there is this
playlist_item_t * hidden in the QModelIndex and I don't have the playlist
lock, because I am just the qt view and I didn't even know you had that rule
about locking... */
QModelIndex a_parent = *model->parent ( an_index )*; // let's see if this
index has a parent.
...

EVERYTHING might happen between qt view calling PLModel::index() and
PLModel::parent (). We might as well not even go into details. It's easy to
imagine what all could happen.

So, I just wanted to point to something that might not be so obvious: IF we
have reference counting AND we free items as soon as refrence count goes to
0 THEN we must still pay attention that even if an item has been REMOVED
from the playlist tree (it doesn't meen freed yet, it means there is no
reference to it from any other item in the tree), we must still correct it's
links (playlist_item_t::p_parent, playlist_item_t::pp_children) and set them
to zero, because someone else might still use them and the objects they
pointed to might have been freed!
It is often the practice that if you are gonna throw away an object (a
struct in C or whatever), why making any changes to it's fields? I won't use
it anyway.

So that was just a suggestion. We are still far from there anyway (from
implementing all this stuff).

I have another suggestion, if it happens to make sense to you:

We could just as well not care about the fields of an item that has been
REMOVED, instead whenever a third party like a qt view returns back with
that goddamn QModelIndex and wants something from us we (lock the playlist,
yeees, and) go and check if that playlist_item_t referenced inside the
QModelIndex still exists in the playlist, we go nice and slow from the root
item down (which is always present) and if we don't find that item, than we
just return 0 or NULL or QModelIndex() or QVariant() or QString() or
whatever relevant neutral, void data back to the view.

Makes sense?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20090814/c53eebd1/attachment.html>