Fix #106023: File Browser Lists with Incomplete Previews #107698

Harley Acheson · 2023-05-07T03:52:47+02:00

Harley Acheson commented

2023-05-07 03:52:47 +02:00

Ensure all File Browser list items get previews.

When we start creating a preview for a File Browser item we give it a FILE_ENTRY_PREVIEW_LOADING bit flag and we remove it when complete. This is done to make sure that multiple threads don't work on the same preview at the same time.

However there are MANY times when we clear the preview cache (filelist_cache_previews_clear) for a variety of reasons. When this happens any items that were in the cache queue retain their FILE_ENTRY_PREVIEW_LOADING flag. This means they won't be previewed if we try to add them to the queue again. This results in two types of bad behavior. The first is that given a list of items that have never been previewed, if you scroll while these are being created there might be some items that are never done and will remain showing a generic image icon. The second problem is if a folder contains more items than the cache size (usually 255 items) you might see some generic icons on either end of the list even if all items have been previewed.

This PR just checks for the state of a cleared cache (cache->previews_todo_count == 0) at the point of adding new items to the queue. If this is the case we remove that FILE_ENTRY_PREVIEW_LOADING bit flag since there is no good reason for it to be there at that time; it will only be there in error because the cache was cleared while it was in there.

Ensure all File Browser list items get previews. --- When we start creating a preview for a File Browser item we give it a FILE_ENTRY_PREVIEW_LOADING bit flag and we remove it when complete. This is done to make sure that multiple threads don't work on the same preview at the same time. However there are MANY times when we clear the preview cache (filelist_cache_previews_clear) for a variety of reasons. When this happens any items that were in the cache queue retain their FILE_ENTRY_PREVIEW_LOADING flag. This means they won't be previewed if we try to add them to the queue again. This results in two types of bad behavior. The first is that given a list of items that have never been previewed, if you scroll while these are being created there might be some items that are never done and will remain showing a generic image icon. The second problem is if a folder contains more items than the cache size (usually 255 items) you might see some generic icons on either end of the list even if all items have been previewed. This PR just checks for the state of a cleared cache (cache->previews_todo_count == 0) at the point of adding new items to the queue. If this is the case we remove that FILE_ENTRY_PREVIEW_LOADING bit flag since there is no good reason for it to be there at that time; it will only be there in error because the cache was cleared while it was in there.

Harley Acheson added 1 commit 2023-05-07 03:52:56 +02:00

31af641f82 Fix #106023 : File Browser Lists with Incomplete Previews

Ensure all File Browser list items get previews.

Harley Acheson added this to the User Interface project 2023-05-07 03:53:10 +02:00

Harley Acheson requested review from Julian Eisel 2023-05-07 03:53:16 +02:00

Jesse Yurkovich commented

2023-05-07 05:04:31 +02:00

I can confirm this PR does seem to fix the issue for my case. I was going to suggest that the flag be removed inside filelist_cache_previews_clear rather than where you did here, since that would make the most sense, but I don't see a way to do that?

I can confirm this PR does seem to fix the issue for my case. I was going to suggest that the flag be removed inside `filelist_cache_previews_clear` rather than where you did here, since that would make the most sense, but I don't see a way to do that?

Harley Acheson commented

2023-05-07 05:19:42 +02:00

...suggest that the flag be removed inside `filelist_cache_previews_clear...I don't see a way to do that?

Yeah, me neither. I also looked at that and couldn't see a nice way to do this there while this PR is simple even if a little removed from the problem.

I also found a wonderful performance improvement if filelist_file_cache_slidingwindow_set is not allowed to make filelist_cache.size go below the initial value of FILELIST_ENTRYCACHESIZE_DEFAULT (1024). Mine is normally at 256 even though the intention is that it be between 256-8092. I'll probably post a PR for that but still thinking of how to sell it.

> ...suggest that the flag be removed inside `filelist_cache_previews_clear...I don't see a way to do that? Yeah, me neither. I also looked at that and couldn't see a nice way to do this there while this PR is simple even if a little removed from the problem. I also found a wonderful performance improvement if `filelist_file_cache_slidingwindow_set` is not allowed to make `filelist_cache.size` go below the initial value of FILELIST_ENTRYCACHESIZE_DEFAULT (1024). Mine is normally at 256 even though the intention is that it be between 256-8092. I'll probably post a PR for that but still thinking of how to sell it.

Harley Acheson referenced this pull request

2023-05-07 19:53:32 +02:00

UI: Increase File Browser Minimum Cache size #107711

Julian Eisel commented

2023-05-08 16:58:54 +02:00

This is indeed a quite random place to put this in, and I'm not sure how reliable it is.

This should be better:

diff --git a/source/blender/editors/space_file/filelist.cc b/source/blender/editors/space_file/filelist.cc
index c5ff4ea30e6..7c1e2c32d79 100644
--- a/source/blender/editors/space_file/filelist.cc
+++ b/source/blender/editors/space_file/filelist.cc
@@ -1552,6 +1552,10 @@ static void filelist_cache_previews_clear(FileListEntryCache *cache)
   if (cache->previews_pool) {
     BLI_task_pool_cancel(cache->previews_pool);
 
+    LISTBASE_FOREACH (FileDirEntry *, entry, &cache->cached_entries) {
+      entry->flags &= ~FILE_ENTRY_PREVIEW_LOADING;
+    }
+
     FileListEntryPreview *preview;
     while ((preview = static_cast<FileListEntryPreview *>(
                 BLI_thread_queue_pop_timeout(cache->previews_done, 0))))

This is indeed a quite random place to put this in, and I'm not sure how reliable it is. This should be better: ```diff diff --git a/source/blender/editors/space_file/filelist.cc b/source/blender/editors/space_file/filelist.cc index c5ff4ea30e6..7c1e2c32d79 100644 --- a/source/blender/editors/space_file/filelist.cc +++ b/source/blender/editors/space_file/filelist.cc @@ -1552,6 +1552,10 @@ static void filelist_cache_previews_clear(FileListEntryCache *cache) if (cache->previews_pool) { BLI_task_pool_cancel(cache->previews_pool); + LISTBASE_FOREACH (FileDirEntry *, entry, &cache->cached_entries) { + entry->flags &= ~FILE_ENTRY_PREVIEW_LOADING; + } + FileListEntryPreview *preview; while ((preview = static_cast<FileListEntryPreview *>( BLI_thread_queue_pop_timeout(cache->previews_done, 0)))) ```

Harley Acheson added 1 commit 2023-05-08 19:17:49 +02:00

1b1c6c966f Remove the FILE_ENTRY_PREVIEW_LOADING flag in a more logical location.

Harley Acheson commented

2023-05-08 19:20:33 +02:00