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

[Pierre d'Herbemont]

On Aug 14, 2009, at 12:19 PM, Jakob Leben wrote:

> On Fri, Aug 14, 2009 at 11:11 AM, Pierre d'Herbemont <pdherbemont at gmail.com 
> > wrote:
> But imagine that in the time when we don't hold the lock the other  
> thread adds a child to the playlist item, so changes it's  
> pp_children field, but then unreferences both the item and its new  
> child, before we could add a reference to the child! The former item  
> will still exist as we had a reference to it, but with wrong  
> reference to a child that got already deleted, as we didn't add a  
> reference for it.
>
> That's one of the weakness of the playlist. However, this won't  
> happen as playlist_item are only freed when the playlist is freed.  
> See src/playlist/item.c
>
> int playlist_ItemRelease( playlist_item_t *p_item )
> {
>        /* For the assert */
>        playlist_t *p_playlist = p_item->p_playlist;
>        PL_ASSERT_LOCKED;
>
>        /* Surprise, we can't actually do more because we
>        * don't do refcounting, or eauivalent.
>        * Because item are not only accessed by their id
>        * using playlist_item outside the PL_LOCK isn't safe.
>        * modules do that.
>        *
>        * Who wants to add proper memory management? */
>        uninstall_input_item_observer( p_item );
>        ARRAY_APPEND( pl_priv(p_playlist)->items_to_delete, p_item);
>        return VLC_SUCCESS;
> }
>
> As the comment in the above code suggests, the point of adding  
> reference counting to playlist items is exactly that they can be  
> freed sooner then when the playlist is freed. And I guess it is  
> desirable so, instead of just hanging on to the current behavior.

It is the point of my mail, right.

> So then we *will* have the problem I was writing about.

For sure, there will be a synchronization issue between the playlist  
data and the view.

> However, now I think of a possible solution: even if both parent and  
> it's children are being removed, they shouldn't be just freed, but  
> their internal pointers (parent -> children, child -> parent) should  
> still be adjusted.

What is to be freed? The playlist_item? If there is someone holding a  
reference to it, it won't happen with refcounting. With current  
implementation those pointers will also always be valid

> This way when control in a qt interface returns from a view to our  
> implementation of a model's function and it uses a QModelIndex with  
> a pointer to the parent item, both that pointer and it's internal  
> pointer to children are valid (only the pointer to children is NULL).

If you use refcounting they will *alway* be valid. Again, with current  
implementation those pointer will also always be valid.

> This way we can still serve the view's request for data about the  
> parent, even if it changed since it obtained the QModelIndex. I  
> guess qt views should be (cosmetically) protected against this  
> change of data, since the documentation states that QModelIndex is  
> only temporary. The main concern is just to prevent segmentation  
> faults.

This is addressed by refcounting, and proper ownership. You are not  
trying to address segmentation fault but synchronization, unless I am  
mistaken.

Pierre.



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