First pass to clean up snapshot code (vout).

It is thread safe (or should be).
No snapshot should be lost.
The image encoding is now done outside vout thread (in the callback).

src/video_output/video_output.c

@@ -399,6 +399,12 @@ vout_thread_t * __vout_Create( vlc_object_t *p_parent, video_format_t *p_fmt )
     p_vout->p->b_picture_empty = false;
     p_vout->p->i_picture_qtype = QTYPE_NONE;
 
+    p_vout->p->snapshot.b_available = true;
+    p_vout->p->snapshot.i_request = 0;
+    p_vout->p->snapshot.p_picture = NULL;
+    vlc_mutex_init( &p_vout->p->snapshot.lock );
+    vlc_cond_init( &p_vout->p->snapshot.wait );
+
     /* Initialize locks */
     vlc_mutex_init( &p_vout->picture_lock );
     vlc_cond_init( &p_vout->p->picture_wait );
@@ -575,6 +581,12 @@ void vout_Close( vout_thread_t *p_vout )
     p_vout->p->b_done = true;
     vlc_cond_signal( &p_vout->p->change_wait );
     vlc_mutex_unlock( &p_vout->change_lock );
+
+    vlc_mutex_lock( &p_vout->p->snapshot.lock );
+    p_vout->p->snapshot.b_available = false;
+    vlc_cond_broadcast( &p_vout->p->snapshot.wait );
+    vlc_mutex_unlock( &p_vout->p->snapshot.lock );
+
     vlc_join( p_vout->p->thread, NULL );
     module_unneed( p_vout, p_vout->p_module );
     p_vout->p_module = NULL;
@@ -599,6 +611,21 @@ static void vout_Destructor( vlc_object_t * p_this )
     vlc_mutex_destroy( &p_vout->change_lock );
     vlc_mutex_destroy( &p_vout->p->vfilter_lock );
 
+    /* */
+    for( ;; )
+    {
+        picture_t *p_picture = p_vout->p->snapshot.p_picture;
+        if( !p_picture )
+            break;
+
+        p_vout->p->snapshot.p_picture = p_picture->p_next;
+
+        picture_Release( p_picture );
+    }
+    vlc_cond_destroy( &p_vout->p->snapshot.wait );
+    vlc_mutex_destroy( &p_vout->p->snapshot.lock );
+
+    /* */
     free( p_vout->p->psz_filter_chain );
     free( p_vout->p->psz_title );
 
@@ -1166,7 +1193,9 @@ static void* RunThread( void *p_this )
 
         /* FIXME it is ugly that b_snapshot is not locked but I do not
          * know which lock to use (here and in the snapshot callback) */
-        const bool b_snapshot = p_vout->p->b_snapshot && p_picture != NULL;
+        vlc_mutex_lock( &p_vout->p->snapshot.lock );
+        const bool b_snapshot = p_vout->p->snapshot.i_request > 0 && p_picture != NULL;
+        vlc_mutex_unlock( &p_vout->p->snapshot.lock );
 
         /*
          * Check for subpictures to display
@@ -1188,10 +1217,25 @@ static void* RunThread( void *p_this )
          */
         if( p_directbuffer && b_snapshot )
         {
-            /* FIXME lock (see b_snapshot) */
-            p_vout->p->b_snapshot = false;
-
-            vout_Snapshot( p_vout, p_directbuffer );
+            vlc_mutex_lock( &p_vout->p->snapshot.lock );
+            assert( p_vout->p->snapshot.i_request > 0 );
+            while( p_vout->p->snapshot.i_request > 0 )
+            {
+                picture_t *p_pic = picture_New( p_vout->fmt_out.i_chroma,
+                                                p_vout->fmt_out.i_width,
+                                                p_vout->fmt_out.i_height,
+                                                p_vout->fmt_out.i_aspect  );
+                if( !p_pic )
+                    break;
+
+                picture_Copy( p_pic, p_directbuffer );
+
+                p_pic->p_next = p_vout->p->snapshot.p_picture;
+                p_vout->p->snapshot.p_picture = p_pic;
+                p_vout->p->snapshot.i_request--;
+            }
+            vlc_cond_broadcast( &p_vout->p->snapshot.wait );
+            vlc_mutex_unlock( &p_vout->p->snapshot.lock );
         }
 
         /*

src/video_output/vout_internal.h

@@ -86,8 +86,15 @@ struct vout_thread_sys_t
     filter_chain_t *p_vf2_chain;
     char           *psz_vf2;
 
-    /* Misc */
-    bool            b_snapshot;     /**< take one snapshot on the next loop */
+    /* Snapshot interface */
+    struct
+    {
+        bool        b_available;
+        int         i_request;
+        picture_t   *p_picture;
+        vlc_mutex_t lock;
+        vlc_cond_t  wait;
+    } snapshot;
 
     /* Show media title on videoutput */
     bool            b_title_show;
@@ -107,7 +114,5 @@ picture_t *vout_RenderPicture( vout_thread_t *, picture_t *,
  */
 void vout_UsePictureLocked( vout_thread_t *p_vout, picture_t *p_pic  );
 
-int vout_Snapshot( vout_thread_t *, picture_t *p_pic );
-
 #endif
 

src/video_output/vout_intf.c

@@ -584,13 +584,9 @@ static char *VoutSnapshotGetDefaultDirectory( void )
     return psz_path;
 }
 
-int vout_Snapshot( vout_thread_t *p_vout, picture_t *p_picture_private )
+#warning "Remove vout_Snapshot"
+static int vout_Snapshot( vout_thread_t *p_vout, picture_t *p_pic )
 {
-    /* FIXME find why it is sometimes needed (format being wrong/invalid) */
-    picture_t pic = *p_picture_private;
-    pic.format = p_vout->fmt_out;
-    picture_t *p_pic = &pic;
-
     /* */
     char *psz_path = var_GetNonEmptyString( p_vout, "snapshot-path" );
 
@@ -817,6 +813,46 @@ error:
     return VLC_EGENERIC;
 }
 
+static picture_t *VoutGetSnapshot( vout_thread_t *p_vout, mtime_t i_timeout )
+{
+    vout_thread_sys_t *p_sys = p_vout->p;
+
+    vlc_mutex_lock( &p_sys->snapshot.lock );
+    p_sys->snapshot.i_request++;
+
+    const mtime_t i_deadline = mdate() + i_timeout;
+    while( p_sys->snapshot.b_available && !p_sys->snapshot.p_picture &&
+           mdate() < i_deadline )
+    {
+        vlc_cond_timedwait( &p_sys->snapshot.wait, &p_sys->snapshot.lock,
+                            i_timeout );
+    }
+
+    picture_t *p_picture = p_sys->snapshot.p_picture;
+    if( p_picture )
+        p_sys->snapshot.p_picture = p_picture->p_next;
+    else if( p_sys->snapshot.i_request > 0 )
+        p_sys->snapshot.i_request--;
+    vlc_mutex_unlock( &p_sys->snapshot.lock );
+
+    if( !p_picture )
+        msg_Err( p_vout, "Failed to grab a snapshot" );
+
+    return p_picture;
+}
+
+static void VoutSaveSnapshot( vout_thread_t *p_vout )
+{
+    /* 500ms timeout */
+    picture_t *p_picture = VoutGetSnapshot( p_vout, 500*1000 );
+
+    if( p_picture )
+    {
+        vout_Snapshot( p_vout, p_picture );
+        picture_Release( p_picture );
+    }
+}
+
 /*****************************************************************************
  * Handle filters
  *****************************************************************************/
@@ -1222,11 +1258,10 @@ static int SnapshotCallback( vlc_object_t *p_this, char const *psz_cmd,
                        vlc_value_t oldval, vlc_value_t newval, void *p_data )
 {
     vout_thread_t *p_vout = (vout_thread_t *)p_this;
-
-    /* FIXME: this is definitely not thread-safe */
-    p_vout->p->b_snapshot = true;
     VLC_UNUSED(psz_cmd); VLC_UNUSED(oldval);
     VLC_UNUSED(newval); VLC_UNUSED(p_data);
+
+    VoutSaveSnapshot( p_vout );
     return VLC_SUCCESS;
 }