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

[Rémi Denis-Courmont]

On Fri, 14 Aug 2009 16:48:03 +0200, Jakob Leben <jakob.leben at gmail.com>
wrote:
> 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.

Walking around with an item is OK provided that:
 - you have a reference to the item,
 - you have the playlist lock whenever you read (or write)
   the parent or child or whatever structural link of that item.

The playlist lock is not needed at all times. And if that's 

> 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* ) );
> }

That's where you need reference counting. You acquire a reference while you
have the lock. Then you know that the pointer will remain valid until you
release the reference. Even if you release the playlist lock earlier.

And yeah, it is probably impossible to get rid of playlist item IDs without
first implementing a reference counting mechanism.

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

p_parent becomes invalid when its reference count dropped from one down to
zero. In other words, there was only one reference to p_parent. Since
p_item has a reference to p_parent, we deduce it is _that_ sole reference.
Whoever dropped that reference is expected to do it properly, which means
replace p_parent with NULL or another valid item with an extra reference.

Hence, I don't how a problem could happen.

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

You need the lock when you read or write the structure.
You need a reference when you have a pointer to the object.


This is exactly like the VLC objects tree, with vlc_object_hold(),
vlc_object_release() and the LibVLC structure lock.

-- 
Rémi Denis-Courmont