Thumbnails: Always use cached thumbnails for offline files #112101

Julian Eisel · 2023-09-07T20:31:41+02:00

Julian Eisel commented

2023-09-07 20:31:41 +02:00

bd9f94e917 made it so the file browser doesn't bring files online for
the purpuse of creating their thumbnail, because that can take a while.
Instead it uses a previously cached thumbnail if available. This should
be the behavior for all cases thumbnails are requested, it's not only
the file browser that does this.

In fact it makes sense to move this into the normal function to "manage"
thumbnails (that is, load and if necessary (re)create cached
thumbnails) since there are no currently known use-cases for
different behavior.

Also, seems like the previous solution didn't work when loading ID
previews from offline .blend files. For that we need to use the path to
the .blend file to check the offline status, not the full path to the ID.

Found while working on #109234 (Use UI preview system for async loading of
file/asset previews).

bd9f94e917 made it so the file browser doesn't bring files online for the purpuse of creating their thumbnail, because that can take a while. Instead it uses a previously cached thumbnail if available. This should be the behavior for all cases thumbnails are requested, it's not only the file browser that does this. In fact it makes sense to move this into the normal function to "manage" thumbnails (that is, load and if necessary (re)create cached thumbnails) since there are no currently known use-cases for different behavior. Also, seems like the previous solution didn't work when loading ID previews from offline .blend files. For that we need to use the path to the .blend file to check the offline status, not the full path to the ID. Found while working on #109234 (Use UI preview system for async loading of file/asset previews).

Julian Eisel added the

Module

User Interface

label 2023-09-07 20:31:41 +02:00

Julian Eisel added 1 commit 2023-09-07 20:31:57 +02:00

17b491f7db Thumbnails: Use cached thumbnails for offline files, don't access file

bd9f94e917 made it so the file browser doesn't bring files online for
the purpuse of creating their thumbnail, because that can take a while.
Instead it uses a previously cached thumbnail if available. This should
be the behavior for all cases thumbnails are requested, it's not only
the file browser that does this.

In fact it makes sense to move this into the normal function to "manage"
thumbnails (that is, load and if necessary (re)create cached
thumbnails).

Also, seems like the previous solution didn't work when loading ID
previews from offline .blend files. For that we need to use the path to
the .blend file to check the offline status, not the full path to the ID.

Found while working on #109234 (Use UI preview system for async loading of
file/asset previews).

Julian Eisel changed title from ~~Thumbnails: Use cached thumbnails for offline files, don't access file~~ to Thumbnails: Always use cached thumbnails for offline files

2023-09-07 20:32:21 +02:00

Julian Eisel requested review from Harley Acheson 2023-09-07 20:36:32 +02:00

Harley Acheson commented

2023-09-07 22:06:39 +02:00

Works great. I tested this with files in "One Drive". Works correctly when offline and online, with existing thumbnails and without.

Fairly certain I had only added attributes to that FileListEntryPreview so that they would be cached, but I like this idea better.

However, this PR has the attribute check fairly early in IMB_thumb_manage, even before checking for the failed ones. Fairly certain we get the same effect but more simply if we check the attributes in thumb_create_or_fail where it can just return nullptr. Unless I need more coffee, I think that gets us what we want: return the already-created thumbnail if we have it, return nullptr if a failed one, create a new one only if not offline. At least it tests out okay:

diff --git a/source/blender/editors/space_file/filelist.cc b/source/blender/editors/space_file/filelist.cc
index e551f4cecb7..fc730bbee99 100644
--- a/source/blender/editors/space_file/filelist.cc
+++ b/source/blender/editors/space_file/filelist.cc
@@ -181,11 +181,10 @@ enum {
 struct FileListEntryPreview {
   /** Use #FILE_MAX_LIBEXTRA as this is the size written into by #filelist_file_get_full_path. */
   char filepath[FILE_MAX_LIBEXTRA];
   uint flags;
   int index;
-  int attributes; /* from FileDirEntry. */
   int icon_id;
 };
 
 /* Dummy wrapper around FileListEntryPreview to ensure we do not access freed memory when freeing
  * tasks' data (see #74609). */
@@ -1532,14 +1531,12 @@ static void filelist_cache_preview_runf(TaskPool *__restrict pool, void *taskdat
     source = THB_SOURCE_OBJECT_IO;
   }
 
   IMB_thumb_path_lock(preview->filepath);
   /* Always generate biggest preview size for now, it's simpler and avoids having to re-generate
-   * in case user switch to a bigger preview size. Do not create preview when file is offline. */
-  ImBuf *imbuf = (preview->attributes & FILE_ATTR_OFFLINE) ?
-                     IMB_thumb_read(preview->filepath, THB_LARGE) :
-                     IMB_thumb_manage(preview->filepath, THB_LARGE, source);
+   * in case user switch to a bigger preview size. */
+  ImBuf *imbuf = IMB_thumb_manage(preview->filepath, THB_LARGE, source);
   IMB_thumb_path_unlock(preview->filepath);
   if (imbuf) {
     preview->icon_id = BKE_icon_imbuf_create(imbuf);
   }
 
@@ -1665,11 +1662,10 @@ static void filelist_cache_previews_push(FileList *filelist, FileDirEntry *entry
   entry->flags |= FILE_ENTRY_PREVIEW_LOADING;
 
   FileListEntryPreview *preview = MEM_new<FileListEntryPreview>(__func__);
   preview->index = index;
   preview->flags = entry->typeflag;
-  preview->attributes = entry->attributes;
   preview->icon_id = 0;
 
   if (preview_in_memory) {
     /* TODO(mano-wii): No need to use the thread API here. */
     BLI_assert(BKE_previewimg_is_finished(preview_in_memory, ICON_SIZE_PREVIEW));
diff --git a/source/blender/imbuf/IMB_thumbs.h b/source/blender/imbuf/IMB_thumbs.h
index 9506546e3b8..0d5cd0e21de 100644
--- a/source/blender/imbuf/IMB_thumbs.h
+++ b/source/blender/imbuf/IMB_thumbs.h
@@ -72,10 +72,13 @@ struct ImBuf *IMB_thumb_read(const char *file_or_lib_path, ThumbSize size);
  */
 void IMB_thumb_delete(const char *file_or_lib_path, ThumbSize size);
 
 /**
  * Create the thumb if necessary and manage failed and old thumbs.
+ * Will not attempt to (re)create thumbnails of offline files. In this case only a preexisting
+ * thumbnail is returned, or null if none was found.
+ *
  * \param file_or_lib_path: File path or library-ID path (e.g. `/a/b.blend/Material/MyMaterial`) to
  *                          the thumbnail to be created/managed.
  */
 struct ImBuf *IMB_thumb_manage(const char *file_or_lib_path, ThumbSize size, ThumbSource source);
 
diff --git a/source/blender/imbuf/intern/thumbs.cc b/source/blender/imbuf/intern/thumbs.cc
index da9fc72f6b5..0760e6a4ace 100644
--- a/source/blender/imbuf/intern/thumbs.cc
+++ b/source/blender/imbuf/intern/thumbs.cc
@@ -462,10 +462,15 @@ static ImBuf *thumb_create_or_fail(const char *file_path,
                                    const char *blen_group,
                                    const char *blen_id,
                                    ThumbSize size,
                                    ThumbSource source)
 {
+  const eFileAttributes file_attributes = BLI_file_attributes(file_path);
+  if (file_attributes & FILE_ATTR_OFFLINE) {
+    return nullptr;
+  }
+
   ImBuf *img = thumb_create_ex(
       file_path, uri, thumb, use_hash, hash, blen_group, blen_id, size, source, nullptr);
 
   if (!img) {
     /* thumb creation failed, write fail thumb */

Works great. I tested this with files in "One Drive". Works correctly when offline and online, with existing thumbnails and without. Fairly certain I had only added attributes to that FileListEntryPreview so that they would be cached, but I like this idea better. However, this PR has the attribute check fairly early in `IMB_thumb_manage`, even before checking for the failed ones. Fairly certain we get the same effect but more simply if we check the attributes in `thumb_create_or_fail` where it can just return nullptr. Unless I need more coffee, I think that gets us what we want: return the already-created thumbnail if we have it, return nullptr if a failed one, create a new one only if not offline. At least it tests out okay: ``` diff --git a/source/blender/editors/space_file/filelist.cc b/source/blender/editors/space_file/filelist.cc index e551f4cecb7..fc730bbee99 100644 --- a/source/blender/editors/space_file/filelist.cc +++ b/source/blender/editors/space_file/filelist.cc @@ -181,11 +181,10 @@ enum { struct FileListEntryPreview { /** Use #FILE_MAX_LIBEXTRA as this is the size written into by #filelist_file_get_full_path. */ char filepath[FILE_MAX_LIBEXTRA]; uint flags; int index; - int attributes; /* from FileDirEntry. */ int icon_id; }; /* Dummy wrapper around FileListEntryPreview to ensure we do not access freed memory when freeing * tasks' data (see #74609). */ @@ -1532,14 +1531,12 @@ static void filelist_cache_preview_runf(TaskPool *__restrict pool, void *taskdat source = THB_SOURCE_OBJECT_IO; } IMB_thumb_path_lock(preview->filepath); /* Always generate biggest preview size for now, it's simpler and avoids having to re-generate - * in case user switch to a bigger preview size. Do not create preview when file is offline. */ - ImBuf *imbuf = (preview->attributes & FILE_ATTR_OFFLINE) ? - IMB_thumb_read(preview->filepath, THB_LARGE) : - IMB_thumb_manage(preview->filepath, THB_LARGE, source); + * in case user switch to a bigger preview size. */ + ImBuf *imbuf = IMB_thumb_manage(preview->filepath, THB_LARGE, source); IMB_thumb_path_unlock(preview->filepath); if (imbuf) { preview->icon_id = BKE_icon_imbuf_create(imbuf); } @@ -1665,11 +1662,10 @@ static void filelist_cache_previews_push(FileList *filelist, FileDirEntry *entry entry->flags |= FILE_ENTRY_PREVIEW_LOADING; FileListEntryPreview *preview = MEM_new<FileListEntryPreview>(__func__); preview->index = index; preview->flags = entry->typeflag; - preview->attributes = entry->attributes; preview->icon_id = 0; if (preview_in_memory) { /* TODO(mano-wii): No need to use the thread API here. */ BLI_assert(BKE_previewimg_is_finished(preview_in_memory, ICON_SIZE_PREVIEW)); diff --git a/source/blender/imbuf/IMB_thumbs.h b/source/blender/imbuf/IMB_thumbs.h index 9506546e3b8..0d5cd0e21de 100644 --- a/source/blender/imbuf/IMB_thumbs.h +++ b/source/blender/imbuf/IMB_thumbs.h @@ -72,10 +72,13 @@ struct ImBuf *IMB_thumb_read(const char *file_or_lib_path, ThumbSize size); */ void IMB_thumb_delete(const char *file_or_lib_path, ThumbSize size); /** * Create the thumb if necessary and manage failed and old thumbs. + * Will not attempt to (re)create thumbnails of offline files. In this case only a preexisting + * thumbnail is returned, or null if none was found. + * * \param file_or_lib_path: File path or library-ID path (e.g. `/a/b.blend/Material/MyMaterial`) to * the thumbnail to be created/managed. */ struct ImBuf *IMB_thumb_manage(const char *file_or_lib_path, ThumbSize size, ThumbSource source); diff --git a/source/blender/imbuf/intern/thumbs.cc b/source/blender/imbuf/intern/thumbs.cc index da9fc72f6b5..0760e6a4ace 100644 --- a/source/blender/imbuf/intern/thumbs.cc +++ b/source/blender/imbuf/intern/thumbs.cc @@ -462,10 +462,15 @@ static ImBuf *thumb_create_or_fail(const char *file_path, const char *blen_group, const char *blen_id, ThumbSize size, ThumbSource source) { + const eFileAttributes file_attributes = BLI_file_attributes(file_path); + if (file_attributes & FILE_ATTR_OFFLINE) { + return nullptr; + } + ImBuf *img = thumb_create_ex( file_path, uri, thumb, use_hash, hash, blen_group, blen_id, size, source, nullptr); if (!img) { /* thumb creation failed, write fail thumb */ ```

Harley Acheson commented

2023-09-07 22:30:46 +02:00

Wait, no...

There is probably an edge case with mine. If the file is newer than its thumbnail, but now offline, your version should still return the last thumbnail. My version will return a generic icon.

Wait, no... There is probably an edge case with mine. If the file is **newer** than its thumbnail, but now offline, your version should still return the last thumbnail. My version will return a generic icon.

Harley Acheson commented

2023-09-07 23:05:28 +02:00

This would do everything, fairly sure. Loads a thumbnail if it already exists, but then does not create or recreate if offline:

diff --git a/source/blender/editors/space_file/filelist.cc b/source/blender/editors/space_file/filelist.cc
index e551f4cecb7..fc730bbee99 100644
--- a/source/blender/editors/space_file/filelist.cc
+++ b/source/blender/editors/space_file/filelist.cc
@@ -183,7 +183,6 @@ struct FileListEntryPreview {
   char filepath[FILE_MAX_LIBEXTRA];
   uint flags;
   int index;
-  int attributes; /* from FileDirEntry. */
   int icon_id;
 };
 
@@ -1534,10 +1533,8 @@ static void filelist_cache_preview_runf(TaskPool *__restrict pool, void *taskdat
 
   IMB_thumb_path_lock(preview->filepath);
   /* Always generate biggest preview size for now, it's simpler and avoids having to re-generate
-   * in case user switch to a bigger preview size. Do not create preview when file is offline. */
-  ImBuf *imbuf = (preview->attributes & FILE_ATTR_OFFLINE) ?
-                     IMB_thumb_read(preview->filepath, THB_LARGE) :
-                     IMB_thumb_manage(preview->filepath, THB_LARGE, source);
+   * in case user switch to a bigger preview size. */
+  ImBuf *imbuf = IMB_thumb_manage(preview->filepath, THB_LARGE, source);
   IMB_thumb_path_unlock(preview->filepath);
   if (imbuf) {
     preview->icon_id = BKE_icon_imbuf_create(imbuf);
@@ -1667,7 +1664,6 @@ static void filelist_cache_previews_push(FileList *filelist, FileDirEntry *entry
   FileListEntryPreview *preview = MEM_new<FileListEntryPreview>(__func__);
   preview->index = index;
   preview->flags = entry->typeflag;
-  preview->attributes = entry->attributes;
   preview->icon_id = 0;
 
   if (preview_in_memory) {
diff --git a/source/blender/imbuf/IMB_thumbs.h b/source/blender/imbuf/IMB_thumbs.h
index 9506546e3b8..0d5cd0e21de 100644
--- a/source/blender/imbuf/IMB_thumbs.h
+++ b/source/blender/imbuf/IMB_thumbs.h
@@ -74,6 +74,9 @@ void IMB_thumb_delete(const char *file_or_lib_path, ThumbSize size);
 
 /**
  * Create the thumb if necessary and manage failed and old thumbs.
+ * Will not attempt to (re)create thumbnails of offline files. In this case only a preexisting
+ * thumbnail is returned, or null if none was found.
+ *
  * \param file_or_lib_path: File path or library-ID path (e.g. `/a/b.blend/Material/MyMaterial`) to
  *                          the thumbnail to be created/managed.
  */
diff --git a/source/blender/imbuf/intern/thumbs.cc b/source/blender/imbuf/intern/thumbs.cc
index da9fc72f6b5..81f23c8ded9 100644
--- a/source/blender/imbuf/intern/thumbs.cc
+++ b/source/blender/imbuf/intern/thumbs.cc
@@ -583,7 +583,10 @@ ImBuf *IMB_thumb_manage(const char *file_or_lib_path, ThumbSize size, ThumbSourc
     }
     else {
       img = IMB_loadiffname(thumb_path, IB_rect | IB_metadata, nullptr);
-      if (img) {
+      if (BLI_file_attributes(file_path) & FILE_ATTR_OFFLINE) {
+        /* Pass. Do not create or recreate thumb. */
+      }
+      else if (img) {
         bool regenerate = false;
 
         char mtime[40];

This would do everything, fairly sure. Loads a thumbnail if it already exists, but then does not create or recreate if offline: ``` diff --git a/source/blender/editors/space_file/filelist.cc b/source/blender/editors/space_file/filelist.cc index e551f4cecb7..fc730bbee99 100644 --- a/source/blender/editors/space_file/filelist.cc +++ b/source/blender/editors/space_file/filelist.cc @@ -183,7 +183,6 @@ struct FileListEntryPreview { char filepath[FILE_MAX_LIBEXTRA]; uint flags; int index; - int attributes; /* from FileDirEntry. */ int icon_id; }; @@ -1534,10 +1533,8 @@ static void filelist_cache_preview_runf(TaskPool *__restrict pool, void *taskdat IMB_thumb_path_lock(preview->filepath); /* Always generate biggest preview size for now, it's simpler and avoids having to re-generate - * in case user switch to a bigger preview size. Do not create preview when file is offline. */ - ImBuf *imbuf = (preview->attributes & FILE_ATTR_OFFLINE) ? - IMB_thumb_read(preview->filepath, THB_LARGE) : - IMB_thumb_manage(preview->filepath, THB_LARGE, source); + * in case user switch to a bigger preview size. */ + ImBuf *imbuf = IMB_thumb_manage(preview->filepath, THB_LARGE, source); IMB_thumb_path_unlock(preview->filepath); if (imbuf) { preview->icon_id = BKE_icon_imbuf_create(imbuf); @@ -1667,7 +1664,6 @@ static void filelist_cache_previews_push(FileList *filelist, FileDirEntry *entry FileListEntryPreview *preview = MEM_new<FileListEntryPreview>(__func__); preview->index = index; preview->flags = entry->typeflag; - preview->attributes = entry->attributes; preview->icon_id = 0; if (preview_in_memory) { diff --git a/source/blender/imbuf/IMB_thumbs.h b/source/blender/imbuf/IMB_thumbs.h index 9506546e3b8..0d5cd0e21de 100644 --- a/source/blender/imbuf/IMB_thumbs.h +++ b/source/blender/imbuf/IMB_thumbs.h @@ -74,6 +74,9 @@ void IMB_thumb_delete(const char *file_or_lib_path, ThumbSize size); /** * Create the thumb if necessary and manage failed and old thumbs. + * Will not attempt to (re)create thumbnails of offline files. In this case only a preexisting + * thumbnail is returned, or null if none was found. + * * \param file_or_lib_path: File path or library-ID path (e.g. `/a/b.blend/Material/MyMaterial`) to * the thumbnail to be created/managed. */ diff --git a/source/blender/imbuf/intern/thumbs.cc b/source/blender/imbuf/intern/thumbs.cc index da9fc72f6b5..81f23c8ded9 100644 --- a/source/blender/imbuf/intern/thumbs.cc +++ b/source/blender/imbuf/intern/thumbs.cc @@ -583,7 +583,10 @@ ImBuf *IMB_thumb_manage(const char *file_or_lib_path, ThumbSize size, ThumbSourc } else { img = IMB_loadiffname(thumb_path, IB_rect | IB_metadata, nullptr); - if (img) { + if (BLI_file_attributes(file_path) & FILE_ATTR_OFFLINE) { + /* Pass. Do not create or recreate thumb. */ + } + else if (img) { bool regenerate = false; char mtime[40]; ```

Julian Eisel commented

2023-09-08 13:01:02 +02:00

Makes sense to respect failed thumbnails. Only issue is that failed thumbnail handling deletes out-of-date failed thumbnails, I don't think we should do that for offline ones, because we can't recreate them.

Also I'd prefer to not mix special-cases like this with the main function logic. For example in your snippet it can easily happen that a new branch is added on the level of the if (BLI_path_ncmp()) branch, and in there the offline file case is forgotten. Generally I'd consider it good practice to separate edge case handling from normal logic (which is often more complicated). Do the former first, so the latter doesn't have to deal with it -- this is more future proof.

Makes sense to respect failed thumbnails. Only issue is that failed thumbnail handling deletes out-of-date failed thumbnails, I don't think we should do that for offline ones, because we can't recreate them. Also I'd prefer to not mix special-cases like this with the main function logic. For example in your snippet it can easily happen that a new branch is added on the level of the `if (BLI_path_ncmp())` branch, and in there the offline file case is forgotten. Generally I'd consider it good practice to separate edge case handling from normal logic (which is often more complicated). Do the former first, so the latter doesn't have to deal with it -- this is more future proof.

Harley Acheson approved these changes 2023-09-08 16:27:30 +02:00

Harley Acheson left a comment

Make sense. It was nice to look at this code again.

Julian Eisel merged commit 3589533908 into main

2023-09-08 16:41:52 +02:00

Julian Eisel referenced this issue from a commit

2023-09-08 16:41:53 +02:00

Thumbnails: Always use cached thumbnails for offline files

Julian Eisel deleted branch temp-offline-file-thumbnail

2023-09-08 16:41:54 +02:00

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Thumbnails: Always use cached thumbnails for offline files #112101