<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Aug 14, 2009, at 12:19 PM, Jakob Leben wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div class="gmail_quote">On Fri, Aug 14, 2009 at 11:11 AM, Pierre d'Herbemont <span dir="ltr"><<a href="mailto:pdherbemont@gmail.com">pdherbemont@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> <div class="im"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">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.<br> </blockquote> <br></div> 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<br> <br> int playlist_ItemRelease( playlist_item_t *p_item )<br> {<br> /* For the assert */<br> playlist_t *p_playlist = p_item->p_playlist;<br> PL_ASSERT_LOCKED;<br> <br> /* Surprise, we can't actually do more because we<br> * don't do refcounting, or eauivalent.<br> * Because item are not only accessed by their id<br> * using playlist_item outside the PL_LOCK isn't safe.<br> * modules do that.<br> *<br> * Who wants to add proper memory management? */<br> uninstall_input_item_observer( p_item );<br> ARRAY_APPEND( pl_priv(p_playlist)->items_to_delete, p_item);<br> return VLC_SUCCESS;<br> }</blockquote></div><br>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.</blockquote><div><br></div><div>It is the point of my mail, right.</div><div><br></div><blockquote type="cite">So then we *will* have the problem I was writing about. <br></blockquote><div><br></div><div>For sure, there will be a synchronization issue between the playlist data and the view.</div><br><blockquote type="cite">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.</blockquote><div><br></div><div>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</div><div><br></div><blockquote type="cite">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).</blockquote><div><br></div><div>If you use refcounting they will *alway* be valid. Again, with current implementation those pointer will also always be valid.</div><br><blockquote type="cite">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.</blockquote><div><br></div>This is addressed by refcounting, and proper ownership. You are not trying to address segmentation fault but synchronization, unless I am mistaken.</div><div><br></div><div>Pierre.<br><br></div><div><br></div><br></body></html>