[shotwell/shotwell-0.15] Crash in video interpretable check: Closes #5071

[Jim Nelson <jnelson src gnome org>]

commit 79bf9894b3a5adcc25898a50e7419aab31d2f7f3
Author: Jim Nelson <jim yorba org>
Date:   Tue Nov 19 14:53:40 2013 -0800

    Crash in video interpretable check: Closes #5071
    
    The database and ThumbnailCache calls in check_is_interpretable
    are not thread-safe although check_is_interpretable was being
    called from a background thread.  Now check_is_interpretable returns
    an object with a foreground_finish() function that does the thread-
    unsafe work there.

 src/VideoMonitor.vala |   14 +++++-
 src/VideoSupport.vala |  102 +++++++++++++++++++++++++++++--------------------
 2 files changed, 72 insertions(+), 44 deletions(-)
---
diff --git a/src/VideoMonitor.vala b/src/VideoMonitor.vala
index 86993b5..bf7f4b2 100644
--- a/src/VideoMonitor.vala
+++ b/src/VideoMonitor.vala
@@ -40,7 +40,11 @@ private class VideoMonitor : MediaMonitor {
     // Performs interpretable check on video. In a background job because
     // this will create a new thumbnail for the video.
     private class VideoInterpretableCheckJob : BackgroundJob {
-        private Video video;
+        // IN
+        public Video video;
+        
+        // OUT
+        public Video.InterpretableResults? results = null;
         
         public VideoInterpretableCheckJob(Video video, CompletionCallback? callback = null) {
             base (video, callback);
@@ -48,7 +52,7 @@ private class VideoMonitor : MediaMonitor {
         }
         
         public override void execute() {
-            video.check_is_interpretable();
+            results = video.check_is_interpretable();
         }
     }
     
@@ -284,7 +288,11 @@ private class VideoMonitor : MediaMonitor {
         }
     }
     
-    void on_interpretable_check_complete(BackgroundJob job) {
+    void on_interpretable_check_complete(BackgroundJob j) {
+        VideoInterpretableCheckJob job = (VideoInterpretableCheckJob) j;
+        
+        job.results.foreground_finish();
+        
         --background_jobs;
         if (background_jobs <= 0)
             Video.notify_normal_thumbs_regenerated();
diff --git a/src/VideoSupport.vala b/src/VideoSupport.vala
index b6acc82..b5e1357 100644
--- a/src/VideoSupport.vala
+++ b/src/VideoSupport.vala
@@ -313,6 +313,34 @@ public class Video : VideoSource, Flaggable, Monitorable, Dateable {
     public const uint64 FLAG_OFFLINE =  0x0000000000000002;
     public const uint64 FLAG_FLAGGED =  0x0000000000000004;
     
+    public class InterpretableResults {
+        internal Video video;
+        internal bool update_interpretable = false;
+        internal bool is_interpretable = false;
+        internal Gdk.Pixbuf? new_thumbnail = null;
+        
+        public InterpretableResults(Video video) {
+            this.video = video;
+        }
+        
+        public void foreground_finish() {
+            if (update_interpretable)
+                video.set_is_interpretable(is_interpretable);
+            
+            if (new_thumbnail != null) {
+                try {
+                    ThumbnailCache.replace(video, ThumbnailCache.Size.BIG, new_thumbnail);
+                    ThumbnailCache.replace(video, ThumbnailCache.Size.MEDIUM, new_thumbnail);
+                    
+                    video.notify_thumbnail_altered();
+                } catch (Error err) {
+                    message("Unable to update video thumbnails for %s: %s", video.to_string(),
+                        err.message);
+                }
+            }
+        }
+    }
+    
     private static bool interpreter_state_changed;
     private static int current_state;
     private static bool normal_regen_complete;
@@ -687,8 +715,9 @@ public class Video : VideoSource, Flaggable, Monitorable, Dateable {
     
     public override void mark_online() {
         remove_flags(FLAG_OFFLINE);
+        
         if ((!get_is_interpretable()) && has_interpreter_state_changed())
-            check_is_interpretable();
+            check_is_interpretable().foreground_finish();
     }
 
     public override void trash() {
@@ -840,58 +869,49 @@ public class Video : VideoSource, Flaggable, Monitorable, Dateable {
             AppWindow.database_error(e);
         }
     }
-
-    public void check_is_interpretable() {
-        VideoReader backing_file_reader = new VideoReader(get_file());
-
+    
+    // Intended to be called from a background thread but can be called from foreground as well.
+    // Caller should call InterpretableResults.foreground_process() only from foreground thread,
+    // however
+    public InterpretableResults check_is_interpretable() {
+        InterpretableResults results = new InterpretableResults(this);
+        
         double clip_duration = -1.0;
         Gdk.Pixbuf? preview_frame = null;
-
+        
+        VideoReader backing_file_reader = new VideoReader(get_file());
         try {
             clip_duration = backing_file_reader.read_clip_duration();
             preview_frame = backing_file_reader.read_preview_frame();
         } catch (VideoError e) {
-            // if we catch an error on an interpretable video here, then this video was
-            // interpretable in the past but has now become non-interpretable (e.g. its
-            // codec was removed from the users system).
-            if (get_is_interpretable()) {
-                lock (backing_row) {
-                    backing_row.is_interpretable = false;
-                }
-                
-                try {
-                    VideoTable.get_instance().update_is_interpretable(get_video_id(), false);
-                } catch (DatabaseError e) {
-                    AppWindow.database_error(e);
-                }
-            }
-            return;
+            // if we catch an error on an interpretable video here, then this video is
+            // non-interpretable (e.g. its codec is not present on the users system).
+            results.update_interpretable = get_is_interpretable();
+            results.is_interpretable = false;
+            
+            return results;
         }
-
-        if (get_is_interpretable())
-            return;
-
-        debug("video %s has become interpretable", get_file().get_basename());
-
-        try {
-            ThumbnailCache.replace(this, ThumbnailCache.Size.BIG, preview_frame);
-            ThumbnailCache.replace(this, ThumbnailCache.Size.MEDIUM, preview_frame);
-        } catch (Error e) {
-            critical("video has become interpretable but couldn't replace cached thumbnails");
+        
+        // if already marked interpretable, this is only confirming what we already knew
+        if (get_is_interpretable()) {
+            results.update_interpretable = false;
+            results.is_interpretable = true;
+            
+            return results;
         }
-
+        
+        debug("video %s has become interpretable", get_file().get_basename());
+        
+        // save this here, this can be done in background thread
         lock (backing_row) {
-            backing_row.is_interpretable = true;
             backing_row.clip_duration = clip_duration;
         }
-
-        try {
-            VideoTable.get_instance().update_is_interpretable(get_video_id(), true);
-        } catch (DatabaseError e) {
-            AppWindow.database_error(e);
-        }
         
-        notify_thumbnail_altered();
+        results.update_interpretable = true;
+        results.is_interpretable = true;
+        results.new_thumbnail = preview_frame;
+        
+        return results;
     }
     
     public override void destroy() {