From: Rémi Duraffort <ivoire@videolan.org>
Date: Wed, 11 Feb 2009 14:29:51 +0000 (+0100)
Subject: The playlist have to be locked when calling playlist_ItemGetById (not tested for... 
X-Git-Tag: 1.0.0-pre1~729
X-Git-Url: https://git.sesse.net/?a=commitdiff_plain;h=7e4419bcc6ed629cd9a06863965260b96b9cb1fa;p=vlc

The playlist have to be locked when calling playlist_ItemGetById (not tested for wince and pda interface).
---

diff --git a/include/vlc_playlist.h b/include/vlc_playlist.h
index ac477ac6a6..c339b825f2 100644
--- a/include/vlc_playlist.h
+++ b/include/vlc_playlist.h
@@ -322,7 +322,7 @@ VLC_EXPORT( int, playlist_BothAddInput, ( playlist_t *, input_item_t *,playlist_
 VLC_EXPORT( playlist_item_t*, playlist_ItemToNode, (playlist_t *,playlist_item_t *, bool) );
 
 /********************************** Item search *************************/
-VLC_EXPORT( playlist_item_t *, playlist_ItemGetById, (playlist_t *, int, bool ) );
+VLC_EXPORT( playlist_item_t *, playlist_ItemGetById, (playlist_t *, int ) );
 VLC_EXPORT( playlist_item_t *, playlist_ItemGetByInput, (playlist_t *,input_item_t *, bool ) );
 VLC_EXPORT( playlist_item_t *, playlist_ItemGetByInputId, (playlist_t *, int, playlist_item_t *) );
 
diff --git a/modules/control/dbus.c b/modules/control/dbus.c
index c67efe0196..6da7504453 100644
--- a/modules/control/dbus.c
+++ b/modules/control/dbus.c
@@ -867,7 +867,7 @@ static int TrackListChangeEmit( vlc_object_t *p_this, const char *psz_var,
         playlist_t *p_playlist = (playlist_t*)p_this;
         playlist_add_t *p_add = newval.p_address;
         playlist_item_t *p_item;
-        p_item = playlist_ItemGetById( p_playlist, p_add->i_node, pl_Locked );
+        p_item = playlist_ItemGetById( p_playlist, p_add->i_node );
         assert( p_item );
         while( p_item->p_parent )
             p_item = p_item->p_parent;
diff --git a/modules/control/http/http.c b/modules/control/http/http.c
index 518c1cb06e..9f1bdf438a 100644
--- a/modules/control/http/http.c
+++ b/modules/control/http/http.c
@@ -776,10 +776,12 @@ int  ArtCallback( httpd_handler_sys_t *p_args,
     i_id = atoi( psz_id );
     if( i_id )
     {
+        playlist_Lock( p_sys->p_playlist );
         playlist_item_t *p_pl_item = playlist_ItemGetById( p_sys->p_playlist,
-                                                           i_id, false );
+                                                           i_id );
         if( p_pl_item )
             p_item = p_pl_item->p_input;
+        playlist_Unlock( p_sys->p_playlist );
     }
     else
     {
diff --git a/modules/control/http/macro.c b/modules/control/http/macro.c
index 0d5d90a640..38a6cc1af2 100644
--- a/modules/control/http/macro.c
+++ b/modules/control/http/macro.c
@@ -199,10 +199,11 @@ static void MacroDo( httpd_file_sys_t *p_args,
                         msg_Dbg( p_intf, "requested playlist play" );
                         break;
                     }
+                    //TODO: really locked here ?
                     playlist_Control( p_sys->p_playlist, PLAYLIST_VIEWPLAY,
                                       true, NULL,
                                       playlist_ItemGetById( p_sys->p_playlist,
-                                      i_item, true ) );
+                                      i_item ) );
                     msg_Dbg( p_intf, "requested playlist item: %i", i_item );
                     break;
                 }
diff --git a/modules/control/http/rpn.c b/modules/control/http/rpn.c
index bd7c169f2c..e58bcf560e 100644
--- a/modules/control/http/rpn.c
+++ b/modules/control/http/rpn.c
@@ -494,7 +494,7 @@ void EvaluateRPN( intf_thread_t *p_intf, mvar_t  *vars,
             i_ret = playlist_Control( p_sys->p_playlist, PLAYLIST_VIEWPLAY,
                                       pl_Locked, NULL,
                                       playlist_ItemGetById( p_sys->p_playlist,
-                                      i_id, pl_Locked ) );
+                                      i_id ) );
             playlist_Unlock( p_sys->p_playlist );
             msg_Dbg( p_intf, "requested playlist item: %i", i_id );
             SSPushN( st, i_ret );
@@ -878,12 +878,13 @@ void EvaluateRPN( intf_thread_t *p_intf, mvar_t  *vars,
         else if( !strcmp( s, "playlist_delete" ) )
         {
             int i_id = SSPopN( st, vars );
+            playlist_Lock( p_sys->p_playlist );
             playlist_item_t *p_item = playlist_ItemGetById( p_sys->p_playlist,
-                                                            i_id, pl_Unlocked );
+                                                            i_id );
             if( p_item )
             {
                 playlist_DeleteFromInput( p_sys->p_playlist,
-                                          p_item->p_input->i_id, pl_Unlocked );
+                                          p_item->p_input->i_id, pl_Locked );
                 msg_Dbg( p_intf, "requested playlist delete: %d", i_id );
             }
             else
@@ -891,6 +892,7 @@ void EvaluateRPN( intf_thread_t *p_intf, mvar_t  *vars,
                 msg_Dbg( p_intf, "couldn't find playlist item to delete (%d)",
                          i_id );
             }
+            playlist_Unlock( p_sys->p_playlist );
         }
         else if( !strcmp( s, "playlist_move" ) )
         {
diff --git a/modules/gui/pda/pda_callbacks.c b/modules/gui/pda/pda_callbacks.c
index c0c93c28a6..9811b79968 100644
--- a/modules/gui/pda/pda_callbacks.c
+++ b/modules/gui/pda/pda_callbacks.c
@@ -164,7 +164,7 @@ void PlaylistRebuildListStore( intf_thread_t *p_intf,
     PL_LOCK;
     for( i_dummy = 0; i_dummy < playlist_CurrentSize(p_playlist) ; i_dummy++ )
     {
-        playlist_item_t *p_item = playlist_ItemGetById( p_playlist, i_dummy, pl_Locked );
+        playlist_item_t *p_item = playlist_ItemGetById( p_playlist, i_dummy );
         if( p_item )
         {
             ppsz_text[0] = p_item->p_input->psz_name;
diff --git a/modules/gui/qt4/components/playlist/playlist_model.cpp b/modules/gui/qt4/components/playlist/playlist_model.cpp
index fd45e755a2..4cb8c2cabe 100644
--- a/modules/gui/qt4/components/playlist/playlist_model.cpp
+++ b/modules/gui/qt4/components/playlist/playlist_model.cpp
@@ -168,10 +168,8 @@ bool PLModel::dropMimeData( const QMimeData *data, Qt::DropAction action,
 
             PL_LOCK;
             playlist_item_t *p_target =
-                        playlist_ItemGetById( p_playlist, targetItem->i_id,
-                                              pl_Locked );
-            playlist_item_t *p_src = playlist_ItemGetById( p_playlist, srcId,
-                                                           pl_Locked );
+                        playlist_ItemGetById( p_playlist, targetItem->i_id );
+            playlist_item_t *p_src = playlist_ItemGetById( p_playlist, srcId );
 
             if( !p_target || !p_src )
             {
@@ -183,8 +181,7 @@ bool PLModel::dropMimeData( const QMimeData *data, Qt::DropAction action,
                 PLItem *parentItem = targetItem->parent();
                 assert( parentItem );
                 playlist_item_t *p_parent =
-                         playlist_ItemGetById( p_playlist, parentItem->i_id,
-                                               pl_Locked );
+                         playlist_ItemGetById( p_playlist, parentItem->i_id );
                 if( !p_parent )
                 {
                     PL_UNLOCK;
@@ -246,8 +243,7 @@ void PLModel::activateItem( const QModelIndex &index )
     PLItem *item = static_cast<PLItem*>(index.internalPointer());
     assert( item );
     PL_LOCK;
-    playlist_item_t *p_item = playlist_ItemGetById( p_playlist, item->i_id,
-                                                    pl_Locked );
+    playlist_item_t *p_item = playlist_ItemGetById( p_playlist, item->i_id );
     activateItem( p_item );
     PL_UNLOCK;
 }
@@ -563,7 +559,7 @@ void PLModel::ProcessItemAppend( playlist_add_t *p_add )
     PL_LOCK;
     if( !nodeItem ) goto end;
 
-    p_item = playlist_ItemGetById( p_playlist, p_add->i_item, pl_Locked );
+    p_item = playlist_ItemGetById( p_playlist, p_add->i_item );
     if( !p_item || p_item->i_flags & PLAYLIST_DBL_FLAG ) goto end;
     if( i_depth == DEPTH_SEL && p_item->p_parent &&
                         p_item->p_parent->i_id != rootItem->i_id )
@@ -632,8 +628,7 @@ void PLModel::rebuild( playlist_item_t *p_root )
 /* This function must be entered WITH the playlist lock */
 void PLModel::UpdateNodeChildren( PLItem *root )
 {
-    playlist_item_t *p_node = playlist_ItemGetById( p_playlist, root->i_id,
-                                                    pl_Locked );
+    playlist_item_t *p_node = playlist_ItemGetById( p_playlist, root->i_id );
     UpdateNodeChildren( p_node, root );
 }
 
@@ -654,8 +649,7 @@ void PLModel::UpdateNodeChildren( playlist_item_t *p_node, PLItem *root )
 /* This function must be entered WITH the playlist lock */
 void PLModel::UpdateTreeItem( PLItem *item, bool signal, bool force )
 {
-    playlist_item_t *p_item = playlist_ItemGetById( p_playlist, item->i_id,
-                                                    pl_Locked );
+    playlist_item_t *p_item = playlist_ItemGetById( p_playlist, item->i_id );
     UpdateTreeItem( p_item, item, signal, force );
 }
 
@@ -714,11 +708,11 @@ void PLModel::doDeleteItem( PLItem *item, QModelIndexList *fullList )
     fullList->removeAll( deleteIndex );
 
     PL_LOCK;
-    playlist_item_t *p_item = playlist_ItemGetById( p_playlist, item->i_id,
-                                                    pl_Locked );
+    playlist_item_t *p_item = playlist_ItemGetById( p_playlist, item->i_id );
     if( !p_item )
     {
-        PL_UNLOCK; return;
+        PL_UNLOCK;
+        return;
     }
     if( p_item->i_children == -1 )
         playlist_DeleteFromInput( p_playlist, item->i_input_id, pl_Locked );
@@ -752,8 +746,7 @@ next:
     PL_LOCK;
     {
         playlist_item_t *p_root = playlist_ItemGetById( p_playlist,
-                                                        rootItem->i_id,
-                                                        pl_Locked );
+                                                        rootItem->i_id );
         if( p_root )
         {
             playlist_RecursiveNodeSort( p_playlist, p_root,
@@ -772,8 +765,7 @@ void PLModel::search( QString search_text )
     PL_LOCK;
     {
         playlist_item_t *p_root = playlist_ItemGetById( p_playlist,
-                                                        rootItem->i_id,
-                                                        pl_Locked );
+                                                        rootItem->i_id );
         assert( p_root );
         char *psz_name = search_text.toUtf8().data();
         playlist_LiveSearchUpdate( p_playlist , p_root, psz_name );
@@ -787,8 +779,7 @@ void PLModel::popup( QModelIndex & index, QPoint &point, QModelIndexList list )
 {
     assert( index.isValid() );
     PL_LOCK;
-    playlist_item_t *p_item = playlist_ItemGetById( p_playlist,
-                                                    itemId( index ), pl_Locked );
+    playlist_item_t *p_item = playlist_ItemGetById( p_playlist, itemId( index ) );
     if( p_item )
     {
         i_popup_item = p_item->i_id;
@@ -867,8 +858,7 @@ void PLModel::popupPlay()
     PL_LOCK;
     {
         playlist_item_t *p_item = playlist_ItemGetById( p_playlist,
-                                                        i_popup_item,
-                                                        pl_Locked );
+                                                        i_popup_item );
         activateItem( p_item );
     }
     PL_UNLOCK;
@@ -876,12 +866,16 @@ void PLModel::popupPlay()
 
 void PLModel::popupInfo()
 {
+    PL_LOCK;
     playlist_item_t *p_item = playlist_ItemGetById( p_playlist,
-                                                    i_popup_item,
-                                                    pl_Unlocked );
+                                                    i_popup_item );
     if( p_item )
     {
-        MediaInfoDialog *mid = new MediaInfoDialog( p_intf, p_item->p_input );
+        input_item_t* p_input = p_item->p_input;
+        vlc_gc_incref( p_input );
+        PL_UNLOCK;
+        MediaInfoDialog *mid = new MediaInfoDialog( p_intf, p_input );
+        vlc_gc_decref( p_input );
         mid->setParent( PlaylistDialog::getInstance( p_intf ),
                         Qt::Dialog );
         mid->show();
@@ -908,13 +902,14 @@ void PLModel::popupSave()
 #include <QDesktopServices>
 void PLModel::popupExplore()
 {
+    PL_LOCK;
     playlist_item_t *p_item = playlist_ItemGetById( p_playlist,
-                                                    i_popup_item,
-                                                    pl_Unlocked );
+                                                    i_popup_item );
     if( p_item )
     {
        input_item_t *p_input = p_item->p_input;
        char *psz_meta = input_item_GetURI( p_input );
+       PL_UNLOCK;
        if( psz_meta )
        {
            const char *psz_access;
@@ -933,6 +928,8 @@ void PLModel::popupExplore()
            free( psz_meta );
        }
     }
+    else
+        PL_UNLOCK;
 }
 
 /**********************************************************************
diff --git a/modules/gui/qt4/components/playlist/standardpanel.cpp b/modules/gui/qt4/components/playlist/standardpanel.cpp
index e5ba2e6d13..6b57047e85 100644
--- a/modules/gui/qt4/components/playlist/standardpanel.cpp
+++ b/modules/gui/qt4/components/playlist/standardpanel.cpp
@@ -305,8 +305,7 @@ void StandardPLPanel::doPopup( QModelIndex index, QPoint point )
 void StandardPLPanel::setRoot( int i_root_id )
 {
     QPL_LOCK;
-    playlist_item_t *p_item = playlist_ItemGetById( THEPL, i_root_id,
-                                                    pl_Locked );
+    playlist_item_t *p_item = playlist_ItemGetById( THEPL, i_root_id );
     assert( p_item );
     p_item = playlist_GetPreferredNode( THEPL, p_item );
     assert( p_item );
diff --git a/modules/gui/skins2/vars/playtree.cpp b/modules/gui/skins2/vars/playtree.cpp
index 405f0726ce..0179f8c4c2 100644
--- a/modules/gui/skins2/vars/playtree.cpp
+++ b/modules/gui/skins2/vars/playtree.cpp
@@ -174,14 +174,20 @@ void Playtree::onAppend( playlist_add_t *p_add )
         Iterator item =  findById( p_add->i_item );
         if( item == end() )
         {
+            playlist_Lock( m_pPlaylist );
             playlist_item_t *p_item = playlist_ItemGetById(
-                                        m_pPlaylist, p_add->i_item, pl_Unlocked );
-            if( !p_item ) return;
+                                        m_pPlaylist, p_add->i_item );
+            if( !p_item )
+            {
+                playlist_Unlock( m_pPlaylist );
+                return;
+            }
             UString *pName = new UString( getIntf(),
                                           p_item->p_input->psz_name );
             node->add( p_add->i_item, UStringPtr( pName ),
                       false,false, false, p_item->i_flags & PLAYLIST_RO_FLAG,
                       p_item );
+            playlist_Unlock( m_pPlaylist );
         }
     }
     tree_update descr;
diff --git a/modules/gui/wince/playlist.cpp b/modules/gui/wince/playlist.cpp
index 538485e43e..14411175fb 100644
--- a/modules/gui/wince/playlist.cpp
+++ b/modules/gui/wince/playlist.cpp
@@ -525,20 +525,24 @@ LRESULT Playlist::ProcessCustomDraw( LPARAM lParam )
             pl_Release( p_intf );
             return CDRF_NEWFONT;
         }
- 
+
+        PL_LOCK;
         playlist_item_t *p_item = playlist_ItemGetById( p_playlist,
-                                        (int)lplvcd->nmcd.dwItemSpec, FALSE );
+                                        (int)lplvcd->nmcd.dwItemSpec );
         if( !p_item )
         {
+            PL_UNLOCK;
             pl_Release( p_intf );
             return CDRF_DODEFAULT;
         }
         if( p_item->i_flags & PLAYLIST_DBL_FLAG )
         {
             lplvcd->clrText = RGB(192,192,192);
+            PL_UNLOCK;
             pl_Release( p_intf );
             return CDRF_NEWFONT;
         }
+        PL_UNLOCK;
         pl_Release( p_intf );
     }
 
@@ -663,10 +667,12 @@ void Playlist::UpdateItem( int i )
 
     if( p_playlist == NULL ) return;
 
-    playlist_item_t *p_item = playlist_ItemGetById( p_playlist, i, FALSE );
+    PL_LOCK;
+    playlist_item_t *p_item = playlist_ItemGetById( p_playlist, i );
 
     if( !p_item )
     {
+        PL_UNLOCK;
         pl_Release( p_intf );
         return;
     }
@@ -679,6 +685,7 @@ void Playlist::UpdateItem( int i )
 
     char psz_duration[MSTRTIME_MAX_SIZE];
     mtime_t dur = input_item_GetDuration( p_item->p_input );
+    PL_UNLOCK;
 
     if( dur != -1 ) secstotimestr( psz_duration, dur/1000000 );
     else memcpy( psz_duration , "-:--:--", sizeof("-:--:--") );
@@ -816,9 +823,10 @@ void Playlist::OnEnableSelection()
     {
         if( ListView_GetItemState( hListView, item, LVIS_SELECTED ) )
         {
-            playlist_item_t *p_item =
-                playlist_ItemGetById( p_playlist, item, FALSE );
+            PL_LOCK;
+            playlist_item_t *p_item = playlist_ItemGetById( p_playlist, item );
             p_item->i_flags ^= PLAYLIST_DBL_FLAG;
+            PL_UNLOCK;
             UpdateItem( item );
         }
     }
@@ -835,10 +843,10 @@ void Playlist::OnDisableSelection()
     {
         if( ListView_GetItemState( hListView, item, LVIS_SELECTED ) )
         {
-            /*XXX*/
-            playlist_item_t *p_item =
-                playlist_ItemGetById( p_playlist, item, FALSE );
+            PL_LOCK;
+            playlist_item_t *p_item = playlist_ItemGetById( p_playlist, item );
             p_item->i_flags |= PLAYLIST_DBL_FLAG;
+            PL_UNLOCK;
             UpdateItem( item );
         }
     }
@@ -870,13 +878,12 @@ void Playlist::ShowInfos( HWND hwnd, int i_item )
     if( p_playlist == NULL ) return;
 
     PL_LOCK;
-    playlist_item_t *p_item = playlist_ItemGetById( p_playlist, i_item, true );
-    PL_UNLOCK;
-
+    playlist_item_t *p_item = playlist_ItemGetById( p_playlist, i_item );
     if( p_item )
     {
         ItemInfoDialog *iteminfo_dialog =
             new ItemInfoDialog( p_intf, this, hInst, p_item );
+        PL_UNLOCK;
         CreateDialogBox( hwnd, iteminfo_dialog );
         UpdateItem( i_item );
         delete iteminfo_dialog;
@@ -1098,8 +1105,8 @@ void Playlist::OnPopupEna()
     playlist_t *p_playlist = pl_Hold( p_intf );
     if( p_playlist == NULL ) return;
 
-    playlist_item_t *p_item =
-        playlist_ItemGetById( p_playlist, i_popup_item, FALSE );
+    PL_LOCK;
+    playlist_item_t *p_item = playlist_ItemGetById( p_playlist, i_popup_item );
 
     if( !(p_playlist->items.p_elems[i_popup_item]->i_flags & PLAYLIST_DBL_FLAG) )
         //playlist_IsEnabled( p_playlist, i_popup_item ) )
@@ -1111,6 +1118,7 @@ void Playlist::OnPopupEna()
         p_item->i_flags ^= PLAYLIST_DBL_FLAG;
     }
 
+    PL_UNLOCK;
     pl_Release( p_intf );
     UpdateItem( i_popup_item );
 }
diff --git a/modules/misc/lua/libs/playlist.c b/modules/misc/lua/libs/playlist.c
index b4113bf026..404064dfb6 100644
--- a/modules/misc/lua/libs/playlist.c
+++ b/modules/misc/lua/libs/playlist.c
@@ -150,8 +150,7 @@ static int vlclua_playlist_goto( lua_State * L )
     PL_LOCK;
     int i_ret = playlist_Control( p_playlist, PLAYLIST_VIEWPLAY,
                                   true, NULL,
-                                  playlist_ItemGetById( p_playlist, i_id,
-                                                        true ) );
+                                  playlist_ItemGetById( p_playlist, i_id ) );
     PL_UNLOCK;
     vlclua_release_playlist_internal( p_playlist );
     return vlclua_push_ret( L, i_ret );
@@ -243,7 +242,7 @@ static int vlclua_playlist_get( lua_State *L )
     if( lua_isnumber( L, 1 ) )
     {
         int i_id = lua_tointeger( L, 1 );
-        p_item = playlist_ItemGetById( p_playlist, i_id, true );
+        p_item = playlist_ItemGetById( p_playlist, i_id );
         if( !p_item )
         {
             PL_UNLOCK;
diff --git a/src/playlist/item.c b/src/playlist/item.c
index a1f1d72c52..29142f536b 100644
--- a/src/playlist/item.c
+++ b/src/playlist/item.c
@@ -315,8 +315,7 @@ void playlist_Clear( playlist_t * p_playlist, bool b_locked )
 int playlist_DeleteFromItemId( playlist_t *p_playlist, int i_id )
 {
     PL_ASSERT_LOCKED;
-    playlist_item_t *p_item = playlist_ItemGetById( p_playlist, i_id,
-                                                    pl_Locked );
+    playlist_item_t *p_item = playlist_ItemGetById( p_playlist, i_id );
     if( !p_item ) return VLC_EGENERIC;
     return DeleteInner( p_playlist, p_item, true );
 }
diff --git a/src/playlist/search.c b/src/playlist/search.c
index 45b0c6d881..f7708b5824 100644
--- a/src/playlist/search.c
+++ b/src/playlist/search.c
@@ -34,25 +34,21 @@
  ***************************************************************************/
 
 /**
- * Search a playlist item by its playlist_item id
- *
- * \param p_playlist the playlist
- * \param i_id the id to find
- * \return the item or NULL on failure
+ * Search a playlist item by its playlist_item id.
+ * The playlist have to be locked
+ * @param p_playlist: the playlist
+ * @param i_id: the id to find
+ * @return the item or NULL on failure
  */
-playlist_item_t * playlist_ItemGetById( playlist_t * p_playlist , int i_id,
-                                        bool b_locked )
+playlist_item_t* playlist_ItemGetById( playlist_t * p_playlist , int i_id )
 {
     int i;
-    PL_LOCK_IF( !b_locked );
+    PL_ASSERT_LOCKED;
     ARRAY_BSEARCH( p_playlist->all_items,->i_id, int, i_id, i );
     if( i != -1 )
-    {
-        PL_UNLOCK_IF( !b_locked );
         return ARRAY_VAL( p_playlist->all_items, i );
-    }
-    PL_UNLOCK_IF( !b_locked );
-    return NULL;
+    else
+        return NULL;
 }
 
 /**