[shotwell] Crash in video interpretable check: Closes #5071
- From: Jim Nelson <jnelson src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [shotwell] Crash in video interpretable check: Closes #5071
- Date: Wed, 20 Nov 2013 00:11:09 +0000 (UTC)
commit 17f34137e6b213542bdfd3cf2def71c60342f7d5
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() {
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]