[vlc-devel] commit: Qt4: fix race in requestVideo and simplify ( Rémi Denis-Courmont )

git version control git at videolan.org
Wed Aug 12 18:51:07 CEST 2009


vlc | branch: master | Rémi Denis-Courmont <remi at remlab.net> | Wed Aug 12 19:40:50 2009 +0300| [7c6ef80c3e80d00e7c2000733b1feee0b537267e] | committer: Rémi Denis-Courmont 

Qt4: fix race in requestVideo and simplify

We were accessing some main interface variables from random threads
without serialization. This simply moves the blocking connection signal
from sole size setting to the whole video widget request call. In the
process, we can remove two signals. We still need a blocking connection
signal. It does not add any new deadlock case, since there was already
blocking connection signal in the same call and in the same direction.

> http://git.videolan.org/gitweb.cgi/vlc.git/?a=commit;h=7c6ef80c3e80d00e7c2000733b1feee0b537267e
---

 modules/gui/qt4/main_interface.cpp |   54 ++++++++++++++++++++++--------------
 modules/gui/qt4/main_interface.hpp |   10 ++++--
 modules/gui/qt4/qt4.cpp            |    4 +-
 3 files changed, 41 insertions(+), 27 deletions(-)

diff --git a/modules/gui/qt4/main_interface.cpp b/modules/gui/qt4/main_interface.cpp
index 46ee59b..715d2fe 100644
--- a/modules/gui/qt4/main_interface.cpp
+++ b/modules/gui/qt4/main_interface.cpp
@@ -216,18 +216,18 @@ MainInterface::MainInterface( intf_thread_t *_p_intf ) : QVLCMW( _p_intf )
     var_AddCallback( p_intf->p_libvlc, "intf-popupmenu", PopupMenuCB, p_intf );
 
 
-    /* VideoWidget connects to avoid different threads speaking to each other */
+    /* VideoWidget connects for asynchronous calls */
+    connect( this, SIGNAL(askGetVideo(WId*,int*,int*,unsigned*,unsigned *)),
+             this, SLOT(getVideoSlot(WId*,int*,int*,unsigned*,unsigned*)),
+             Qt::BlockingQueuedConnection );
     connect( this, SIGNAL(askReleaseVideo( void )),
-             this, SLOT(releaseVideoSlot( void )), Qt::BlockingQueuedConnection );
+             this, SLOT(releaseVideoSlot( void )),
+             Qt::BlockingQueuedConnection );
 
     if( videoWidget )
     {
         CONNECT( this, askVideoToResize( unsigned int, unsigned int ),
                  videoWidget, SetSizing( unsigned int, unsigned int ) );
-
-        connect( this, SIGNAL(askVideoToShow( unsigned int, unsigned int)),
-             videoWidget, SLOT(SetSizing(unsigned int, unsigned int )),
-             Qt::BlockingQueuedConnection );
     }
 
     CONNECT( this, askUpdate(), this, doComponentsUpdate() );
@@ -373,7 +373,6 @@ void MainInterface::createMainWidget( QSettings *settings )
     bgWidget->resize(
             settings->value( "backgroundSize", QSize( 300, 200 ) ).toSize() );
     bgWidget->updateGeometry();
-    CONNECT( this, askBgWidgetToToggle(), bgWidget, toggle() );
 
     if( i_visualmode != QT_ALWAYS_VIDEO_MODE &&
         i_visualmode != QT_MINIMAL_MODE )
@@ -703,41 +702,54 @@ private:
 };
 
 /**
- * README
- * Thou shall not call/resize/hide widgets from on another thread.
- * This is wrong, and this is THE reason to emit signals on those Video Functions
- **/
-WId MainInterface::requestVideo( int *pi_x, int *pi_y,
-                                 unsigned int *pi_width,
-                                 unsigned int *pi_height )
+ * NOTE:
+ * You must note change the state of this object or other Qt4 UI objects,
+ * from the video output thread - only from the Qt4 UI main loop thread.
+ * All window provider queries must be handled through signals or events.
+ * That's why we have all those emit statements...
+ */
+WId MainInterface::getVideo( int *pi_x, int *pi_y,
+                             unsigned int *pi_width, unsigned int *pi_height )
+{
+    if( !videoWidget )
+        return 0;
+
+    /* This is a blocking call signal. Results are returned through pointers.
+     * Beware of deadlocks! */
+    WId id;
+    emit askGetVideo( &id, pi_x, pi_y, pi_width, pi_height );
+    return id;
+}
+
+void MainInterface::getVideoSlot( WId *p_id, int *pi_x, int *pi_y,
+                                  unsigned *pi_width, unsigned *pi_height )
 {
     /* Request the videoWidget */
-    if( !videoWidget ) return 0;
     WId ret = videoWidget->request( pi_x, pi_y,
                                     pi_width, pi_height, b_keep_size );
+    *p_id = ret;
     if( ret ) /* The videoWidget is available */
     {
         /* Did we have a bg ? Hide it! */
         if( VISIBLE( bgWidget) )
         {
             bgWasVisible = true;
-            emit askBgWidgetToToggle();
+            bgWidget->toggle();
         }
         else
             bgWasVisible = false;
 
         /* ask videoWidget to show */
-        emit askVideoToShow( *pi_width, *pi_height );
+        videoWidget->SetSizing( *pi_width, *pi_height );
 
         /* Consider the video active now */
         videoIsActive = true;
 
         emit askUpdate();
     }
-    return ret;
 }
 
-/* Call from the WindowClose function */
+/* Asynchronous call from the WindowClose function */
 void MainInterface::releaseVideo( void )
 {
     emit askReleaseVideo( );
@@ -761,7 +773,7 @@ void MainInterface::releaseVideoSlot( void )
     doComponentsUpdate();
 }
 
-/* Call from WindowControl function */
+/* Asynchronous call from WindowControl function */
 int MainInterface::controlVideo( int i_query, va_list args )
 {
     switch( i_query )
@@ -848,7 +860,7 @@ void MainInterface::toggleMinimalView( bool b_switch )
     { /* NORMAL MODE then */
         if( !videoWidget || videoWidget->isHidden() )
         {
-            emit askBgWidgetToToggle();
+            bgWidget->toggle();
         }
         else
         {
diff --git a/modules/gui/qt4/main_interface.hpp b/modules/gui/qt4/main_interface.hpp
index 371799f..aeeffa8 100644
--- a/modules/gui/qt4/main_interface.hpp
+++ b/modules/gui/qt4/main_interface.hpp
@@ -74,8 +74,8 @@ public:
     virtual ~MainInterface();
 
     /* Video requests from core */
-    WId requestVideo( int *pi_x, int *pi_y,
-                      unsigned int *pi_width, unsigned int *pi_height );
+    WId getVideo( int *pi_x, int *pi_y,
+                  unsigned int *pi_width, unsigned int *pi_height );
     void releaseVideo( void  );
     int controlVideo( int i_query, va_list args );
 
@@ -159,6 +159,8 @@ public slots:
     void popupMenu( const QPoint& );
 
     /* Manage the Video Functions from the vout threads */
+    void getVideoSlot( WId *p_id, int *pi_x, int *pi_y,
+                       unsigned *pi_width, unsigned *pi_height );
     void releaseVideoSlot( void );
 
 private slots:
@@ -177,10 +179,10 @@ private slots:
 
     void showCryptedLabel( bool );
 signals:
+    void askGetVideo( WId *p_id, int *pi_x, int *pi_y,
+                      unsigned int *pi_width, unsigned int *pi_height );
     void askReleaseVideo( );
     void askVideoToResize( unsigned int, unsigned int );
-    void askVideoToShow( unsigned int, unsigned int );
-    void askBgWidgetToToggle();
     void askUpdate();
     void minimalViewToggled( bool );
     void fullscreenInterfaceToggled( bool );
diff --git a/modules/gui/qt4/qt4.cpp b/modules/gui/qt4/qt4.cpp
index 28d10d8..06dea24 100644
--- a/modules/gui/qt4/qt4.cpp
+++ b/modules/gui/qt4/qt4.cpp
@@ -556,12 +556,12 @@ static int WindowOpen( vlc_object_t *p_obj )
     unsigned i_height = p_wnd->cfg->height;
 
 #if defined (Q_WS_X11)
-    p_wnd->handle.xid = p_mi->requestVideo( &i_x, &i_y, &i_width, &i_height );
+    p_wnd->handle.xid = p_mi->getVideo( &i_x, &i_y, &i_width, &i_height );
     if( !p_wnd->handle.xid )
         return VLC_EGENERIC;
 
 #elif defined (WIN32)
-    p_wnd->handle.hwnd = p_mi->requestVideo( &i_x, &i_y, &i_width, &i_height );
+    p_wnd->handle.hwnd = p_mi->getVideo( &i_x, &i_y, &i_width, &i_height );
     if( !p_wnd->handle.hwnd )
         return VLC_EGENERIC;
 #endif




More information about the vlc-devel mailing list