VSE: Faster and more consistent thumbnail cache #126405

Merged
Aras Pranckevicius merged 30 commits from aras_p/blender:vse_thumbs_cache into main 2024-08-29 08:27:32 +02:00

Implementing part of design outlined in #126087.

  • VSE thumbnail cache has a new implementation, hopefully simpler and easier to understand.
    • Instead of cache key being a VSE strip, frame index, plus complicated logic for cache items linking etc.,
    • The cache is keyed by media file path (if multiple strips use the same input file, they will share cache entries), frame
      index within media file, and any extra data (e.g. steam index for multi-steam videos)
  • Much reduced cache flickering and strange/weird thumbnail choices.
    • When just zooming around the timeline, here's how the previous cache looked like:

      and here's how it looks now (ignore orange label in the middle; WIP debugging aid):
    • Likewise, thumbnails no longer disappear-and-reload on operations like Undo, dragging new video strip into
      timeline, or F12 render.
  • Thumbnails now load faster.
    • Windows PC (Ryzen 5950X) Gold: about 3x faster (15sec -> 5sec), Sprite Fright: 2x faster (27sec -> 14sec)
    • Mac laptop (M1 Max) Sprite Fright: 1.3x faster (44sec -> 34sec). (this is different view into timeline than PC, don't compare numbers)
    • Images use dedicated/faster thumbnail loading routines when a format can do that (e.g. JPG and EXR can).
    • Movies reuse ffmpeg decoding context for neighboring strips that use the same file (as often happens when cutting footage)
    • Thumbnail requests are processed on several threads now too.

An obvious next step (again outlined in #126087) would be to reduce CPU side work in actual thumbnail drawing and/or stop GPU textures from being re-created on every draw. But it feels like that is best left as a separate later PR.

Implementing part of design outlined in #126087. - VSE thumbnail cache has a new implementation, hopefully simpler and easier to understand. - Instead of cache key being a VSE strip, frame index, plus complicated logic for cache items linking etc., - The cache is keyed by media file path (if multiple strips use the same input file, they will share cache entries), frame index within media file, and any extra data (e.g. steam index for multi-steam videos) - Much reduced cache flickering and strange/weird thumbnail choices. - When just zooming around the timeline, here's how the previous cache looked like: <video src="/attachments/c50038b9-2d06-4e64-a1ab-ad8763cce6dc" width="400px" title="zoom_old.mp4" controls></video> and here's how it looks now (ignore orange label in the middle; WIP debugging aid): <video src="/attachments/11d67c1d-a1aa-4092-9df4-adf1e218cc55" width="400px" title="zoom_new.mp4" controls></video> - Likewise, thumbnails no longer disappear-and-reload on operations like Undo, dragging new video strip into timeline, or F12 render. - Thumbnails now load faster. - Windows PC (Ryzen 5950X) Gold: about 3x faster (15sec -> 5sec), Sprite Fright: 2x faster (27sec -> 14sec) - Mac laptop (M1 Max) Sprite Fright: 1.3x faster (44sec -> 34sec). (this is different view into timeline than PC, don't compare numbers) - Images use dedicated/faster thumbnail loading routines when a format can do that (e.g. JPG and EXR can). - Movies reuse ffmpeg decoding context for neighboring strips that use the same file (as often happens when cutting footage) - Thumbnail requests are processed on several threads now too. An obvious next step (again outlined in #126087) would be to reduce CPU side work in actual thumbnail drawing and/or stop GPU textures from being re-created on every draw. But it feels like that is best left as a separate later PR.
Aras Pranckevicius force-pushed vse_thumbs_cache from 90cff02ec0 to 88a783ae3a 2024-08-20 08:09:03 +02:00 Compare
Aras Pranckevicius added 1 commit 2024-08-20 08:35:28 +02:00
Fix Linux build since some X11 stuff steals None define
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
ff4178e26b
Author
Member

@blender-bot build

@blender-bot build
Aras Pranckevicius added 1 commit 2024-08-20 08:44:32 +02:00
Aras Pranckevicius added 1 commit 2024-08-20 08:45:49 +02:00
Fix gcc build
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
621ee7c26d
Aras Pranckevicius changed title from WIP: VSE: new thumbnail cache to VSE: Faster and more consistent thumbnail cache 2024-08-20 13:26:41 +02:00
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR126405) when ready.
Aras Pranckevicius added 1 commit 2024-08-21 08:42:35 +02:00
No need to use actual time source, can just track "logical time"
by bumping up a counter on each thumbnail_cache_maintain_capacity
Aras Pranckevicius added 2 commits 2024-08-22 17:38:25 +02:00
Aras Pranckevicius requested review from Sergey Sharybin 2024-08-22 17:40:46 +02:00
Aras Pranckevicius requested review from Richard Antalik 2024-08-22 17:40:53 +02:00
Aras Pranckevicius requested review from Francesco Siddi 2024-08-22 17:41:00 +02:00
Sergey Sharybin reviewed 2024-08-26 16:18:40 +02:00
@ -1578,1 +1579,4 @@
/* Discard thumbnail requests that are far enough from viewing area. */
rctf rect = timeline_ctx->v2d->cur;
rect.xmin -= 30;

Those are interesting numbers. Where are they coming from? Is it guaranteed that partially visible thumbnails are drawn? Should there be some multiplied for UI scale or zoom level?

Those are interesting numbers. Where are they coming from? Is it guaranteed that partially visible thumbnails are drawn? Should there be some multiplied for UI scale or zoom level?
Author
Member

Timeline area X axis coordinate is "frames", Y axis is "channels". So this is +-30 frames from what is visible, and +-1 channels. It is similar to the previous cache code, but indeed perhaps would be good with additional comment.

Timeline area X axis coordinate is "frames", Y axis is "channels". So this is +-30 frames from what is visible, and +-1 channels. It is similar to the previous cache code, but indeed perhaps would be good with additional comment.

This code was part of cache limiting. Currently it seems to remove oldest images, but it is quite possible, that you zoom to part where there is oldest image, which would be freed and then it will have to be loaded again.
Here, the images are sorted by time at which they are accessed, so this should not be an issue.

This code was part of cache limiting. Currently it seems to remove oldest images, but it is quite possible, that you zoom to part where there is oldest image, which would be freed and then it will have to be loaded again. Here, the images are sorted by time at which they are accessed, so this should not be an issue.
aras_p marked this conversation as resolved
@ -0,0 +36,4 @@
// #define DEBUG_PRINT_THUMB_JOB_TIMES
static ThreadMutex thumb_cache_lock = BLI_MUTEX_INITIALIZER;

Should be using std::mutex and std::scoped_lock?

Should be using `std::mutex` and `std::scoped_lock`?
Author
Member

Ah ok! I was not sure whether BLI utilities or C++ std is preferred. Will change.

Ah ok! I was not sure whether BLI utilities or C++ std is preferred. Will change.
aras_p marked this conversation as resolved
@ -0,0 +50,4 @@
struct ThumbnailCache {
struct FrameEntry {
int frame_index = 0;
int stream_index = 0;

Does this refer to a stream of a video file?

Does this refer to a stream of a video file?
aras_p marked this conversation as resolved
Richard Antalik requested changes 2024-08-26 22:58:44 +02:00
Dismissed
Richard Antalik left a comment
Member

First when I just skimmed this patch, I thought, that IMB_thumb_load_image does also work for movies, but that doesn't seem to be the case. It would probably simplify few things here, but not by much.

In any case, this is definitely better, than reusing cache for storing preview images!

First when I just skimmed this patch, I thought, that `IMB_thumb_load_image` does also work for movies, but that doesn't seem to be the case. It would probably simplify few things here, but not by much. In any case, this is definitely better, than reusing cache for storing preview images!
@ -59,2 +38,2 @@
SEQ_sequence_free(item->scene, item->seq_dupli);
MEM_freeN(val);
int first_drawable_frame = max_iii(
SEQ_time_left_handle_frame_get(scene, seq), seq->start, view_area->xmin);

Looks like it is behavior also in main, but considering view_area->xmin cause first thumbnail to change when panning. That seems undesirable. Perhaps this needs thumbnail width subtracted, would have to check this in detail.

Looks like it is behavior also in main, but considering `view_area->xmin` cause first thumbnail to change when panning. That seems undesirable. Perhaps this needs thumbnail width subtracted, would have to check this in detail.
Author
Member

Yes there's quite a bit of logic that is weird/funky but I have not changed it, I just moved it from render.cc. Cleaning all that up and "somehow" improving which parts of strips want to show which frames for thumbnails I've left for future PRs.

Yes there's quite a bit of logic that is weird/funky but I have not changed it, I just moved it from render.cc. Cleaning all that up and "somehow" improving which parts of strips want to show which frames for thumbnails I've left for future PRs.
aras_p marked this conversation as resolved
@ -94,3 +57,1 @@
}
if (seq->machine > view_area->ymax) {
return false;
/* If handle position was displayed, align next frame with `seq->start`. */

I don't quite understand what this means, and it is not obvious from the code either.

I don't quite understand what this means, and it is not obvious from the code either.
Author
Member

I don't understand it either. I just moved the function from render.cc, since it is no longer needed there. What is the logic of the function keeps on being mystery to me, but I have not changed it :)

I don't understand it either. I just moved the function from `render.cc`, since it is no longer needed there. What is the logic of the function keeps on being mystery to me, but I have not changed it :)

I would guess that it forced drawing of whole last thumbnail when last handle was translated. Would have to check if that ever worked though.

I would guess that it forced drawing of whole last thumbnail when last handle was translated. Would have to check if that ever worked though.
aras_p marked this conversation as resolved
@ -1579,0 +1583,4 @@
rect.xmax += 30;
rect.ymin -= 2;
rect.ymax += 2;
seq::thumbnail_cache_discard_requests_outside(timeline_ctx->scene, rect);

I was about to ask, why is it necessary to provide such rect to thumbnail_cache_discard_requests_outside, and why such requests are being made, but I guess this is due to them being processed in threads.
Will have to read through that code yet.

I was about to ask, why is it necessary to provide such rect to `thumbnail_cache_discard_requests_outside`, and why such requests are being made, but I guess this is due to them being processed in threads. Will have to read through that code yet.
Author
Member

Previous code, whenever the timeline view changed at all (panning/zooming), was canceling any in-flight thumbnail requests and starting from scratch, essentially.

Now I changed it to not flat out just cancel all in-flight requests, but rather cancel in-flight requests that are "far away enough" from current viewing area. Scenario:

  • You start viewing some part of timeline, it needs many (hundreds) of thumbnails.
  • Said hundreds of thumbnails are started to get loaded in the background, will take several seconds.
  • Meanwhile, you pan somewhere entirely else.
  • This code, for thumbnail requests that are still not loaded, will "cancel" them since they are "quite far away" from where you are looking.

"where are you looking" is the rectangle that is being passed here.

Previous code, whenever the timeline view changed at all (panning/zooming), was canceling any in-flight thumbnail requests and starting from scratch, essentially. Now I changed it to not flat out just cancel all in-flight requests, but rather cancel in-flight requests that are "far away enough" from current viewing area. Scenario: - You start viewing some part of timeline, it needs many (hundreds) of thumbnails. - Said hundreds of thumbnails are started to get loaded in the background, will take several seconds. - Meanwhile, you pan somewhere entirely else. - This code, for thumbnail requests that are still not loaded, will "cancel" them since they are "quite far away" from where you are looking. "where are you looking" is the rectangle that is being passed here.

Yes, I should have deleted this comment after reading through whole code. This seems OK.

Yes, I should have deleted this comment after reading through whole code. This seems OK.
iss marked this conversation as resolved
@ -0,0 +42,4 @@
* If total amount of resident thumbnails is too large, try to remove oldest-used ones to
* keep the cache size in check.
*/
void thumbnail_cache_maintain_capacity(Scene *scene);

Personally I would not make such function public, since there is no guarantee, that user of this cache will call this function. I know, that this is not a library, but still.

Personally I would not make such function public, since there is no guarantee, that user of this cache will call this function. I know, that this is not a library, but still.
Author
Member

To me this feels fine -- this is a function called "once per frame" to maintain cache capacity. "once per frame" is quite important since it ticks the logical timestamps etc. "user of this cache" in this case is only the sequencer code itself, basically from exactly one place. If we can't get this to work, then I think we have bigger problems :)

To me this feels fine -- this is a function called "once per frame" to maintain cache capacity. "once per frame" is quite important since it ticks the logical timestamps etc. "user of this cache" in this case is only the sequencer code itself, basically from exactly one place. If we can't get this to work, then I think we have bigger problems :)

Right so you want to lower frequency of doing these things, which makes sense. Not that it could not be implemented in autonomous way still, but I can imagine code getting unnecessary more complicated.

Right so you want to lower frequency of doing these things, which makes sense. Not that it could not be implemented in autonomous way still, but I can imagine code getting unnecessary more complicated.
iss marked this conversation as resolved
@ -0,0 +164,4 @@
if (!ELEM(seq->type, SEQ_TYPE_MOVIE, SEQ_TYPE_IMAGE)) {
return false;
}
if ((seq->flag & SEQ_FLAG_SKIP_THUMBNAILS) != 0) {

There is no code that sets SEQ_FLAG_SKIP_THUMBNAILS anymore. This was used to avoid endless loop of adding requests, when thumbnails can't be loaded.

Not sure if this PR would have that problem. If such mechanism is needed, this flag could be removed and replaced by missing media feature, or it can be kept for simplicity sake.

There is no code that sets `SEQ_FLAG_SKIP_THUMBNAILS` anymore. This was used to avoid endless loop of adding requests, when thumbnails can't be loaded. Not sure if this PR would have that problem. If such mechanism is needed, this flag could be removed and replaced by missing media feature, or it can be kept for simplicity sake.
Author
Member

Ah cool, I'll remove it then! Missing media is not an issue for this PR, it actually gets "null" thumbnails and handles them just fine, without causing endless requests.

Ah cool, I'll remove it then! Missing media is not an issue for this PR, it actually gets "null" thumbnails and handles them just fine, without causing endless requests.
aras_p marked this conversation as resolved
@ -0,0 +254,4 @@
void ThumbGenerationJob::ensure_job(const bContext *C, ThumbnailCache *cache)
{
wmWindowManager *wm = CTX_wm_manager(C);

IMO it is bit hacky to use windowmanager functions in VSE "core" code. I am guilty of this myself in proxy_job.cc, but I would not do it now (there is good chance, that somebody would stop me during review as well). Looking at BKE code, it is not done there either, so I would be against doing it here.

Let me know if there is good reason to do this. I would need to discuss this and this "rule" may not need to be as strict for VSE.

IMO it is bit hacky to use windowmanager functions in VSE "core" code. I am guilty of this myself in `proxy_job.cc`, but I would not do it now (there is good chance, that somebody would stop me during review as well). Looking at BKE code, it is not done there either, so I would be against doing it here. Let me know if there is good reason to do this. I would need to discuss this and this "rule" may not need to be as strict for VSE.
Author
Member

Right, previously this was within space_sequencer indeed. What would you suggest to do? WM jobs are kinda "perfect" for actually doing this -- they run in the background, then can be cancelled when scene is destroyed, etc. etc. Would you suggest building some sort of "abstraction" to do this, or not use WM jobs at all?

Right, previously this was within space_sequencer indeed. What would you suggest to do? WM jobs are kinda "perfect" for actually doing this -- they run in the background, then can be cancelled when scene is destroyed, etc. etc. Would you suggest building some sort of "abstraction" to do this, or not use WM jobs at all?
Author
Member

Sergey says "I don't see issue with that" on the chat

Sergey says "I don't see issue with that" on the chat

To your question, I would use wm jobs, but would provide a layer of abstraction. I have expected bit more involved discussion, but if this is limited to jobs (pretty much) and we don't try to directly interact with actual editor stuff, I am fine with it. Otherwise, I would probably want to merge blender::seq and blender::ed::seq codebases.

To your question, I would use wm jobs, but would provide a layer of abstraction. I have expected bit more involved discussion, but if this is limited to jobs (pretty much) and we don't try to directly interact with actual editor stuff, I am fine with it. Otherwise, I would probably want to merge `blender::seq` and `blender::ed::seq` codebases.
iss marked this conversation as resolved
@ -0,0 +377,4 @@
/* Add result into the cache (under cache mutex lock). */
BLI_mutex_lock(&thumb_cache_lock);
ThumbnailCache::FileEntry *val = job->cache_->map_.lookup_ptr(request.file_path);

I got crash here, when doing following: press Ctrl+E and pan timeline up (so channels move down)
I had bunch of videos in channels 1 and 4.

job->cache_->map_ was garbage when lokking at it via debugger.

I got crash here, when doing following: press Ctrl+E and pan timeline up (so channels move down) I had bunch of videos in channels 1 and 4. `job->cache_->map_` was garbage when lokking at it via debugger.
Author
Member

Good catch! Pushed a fix, the thumbnails job was not cleaned up properly when destroying the thumbnail cache (as part of Ctrl+E refresh).

Good catch! Pushed a fix, the thumbnails job was not cleaned up properly when destroying the thumbnail cache (as part of Ctrl+E refresh).
aras_p marked this conversation as resolved
@ -0,0 +435,4 @@
val = cache.map_.lookup_ptr(key);
}
if (val == nullptr) {

How would this happen? I would assume, you can use cache.map_.lookup_or_add_default() here.

How would this happen? I would assume, you can use `cache.map_.lookup_or_add_default()` here.
Author
Member

Defensive habits, I guess. Will replace with an assert.

Defensive habits, I guess. Will replace with an assert.
aras_p marked this conversation as resolved
@ -0,0 +445,4 @@
for (int64_t index = 0; index < val->frames.size(); index++) {
/* Make video stream mismatch count way more than a frame mismatch. */
int score = math::abs(frame_index - val->frames[index].frame_index) +
math::abs(seq->streamindex - val->frames[index].stream_index) * 1024;

That's a bit hacky, why not just continue?

That's a bit hacky, why not just `continue`?
Author
Member

Good point, will do.

Good point, will do.
aras_p marked this conversation as resolved
@ -0,0 +470,4 @@
img_width,
img_height);
cache.requests_.add(request);
ThumbGenerationJob::ensure_job(C, &cache);

This may be better as a comment, but it's bit weird to me, that cache gathers all the requests, when you can pass these to a job directly. It does not do anything with these anyway.

This may be better as a comment, but it's bit weird to me, that cache gathers all the requests, when you can pass these to a job directly. It does not do anything with these anyway.
Author
Member

It solves an issue of new incoming requests that would be piled up. Consider:

  1. You start looking at some area that needs hundreds of thumbnails.
  2. The job starts, copies them over to itself and starts processing them.
  3. You keep on slightly panning in the timeline. Pretty much the same thumbnails are still needed, but they are not done yet. So these would get new requests added for them. But that's kinda pointless, they are already being processed, just not finished yet.

The actual storage of "set of requests" could be in the job itself too, yeah. To me it does not feel like there's much difference either way. I can change if it feels more natural to store them there for others.

It solves an issue of new incoming requests that would be piled up. Consider: 1. You start looking at some area that needs hundreds of thumbnails. 2. The job starts, copies them over to itself and starts processing them. 3. You keep on slightly panning in the timeline. Pretty much the same thumbnails are still needed, but they are not done yet. So these would get *new* requests added for them. But that's kinda pointless, they are already being processed, just not finished yet. The actual storage of "set of requests" could be in the job itself too, yeah. To me it does not feel like there's much difference either way. I can change if it feels more natural to store them there for others.

Yes, there isn't much difference, but IMO it would result in cleaner code if requests would be given to job instead of to the cache. On the other hand, you use recency to remove thumbnails if limit is reached, which means, that cache kinda needs to do other stuff.

I would probably feel itch to rewrite this aspect of the code and end up with cache implementation about 100 lines long and a monstrosity for thumbnail job. So I will leave this up to you and trust your judgement.

Yes, there isn't much difference, but IMO it would result in cleaner code if requests would be given to job instead of to the cache. On the other hand, you use recency to remove thumbnails if limit is reached, which means, that cache kinda needs to do other stuff. I would probably feel itch to rewrite this aspect of the code and end up with cache implementation about 100 lines long and a monstrosity for thumbnail job. So I will leave this up to you and trust your judgement.
iss marked this conversation as resolved
@ -0,0 +555,4 @@
/* Do not remove thumbnails for files used within last 10 updates. */
int64_t oldest_time = cache->logical_time_ - 10;
int64_t oldest_entries = 0;
for (const auto &kvp : cache->map_.items()) {

I was asked to not use auto kw, but spell out the type, which seems verbose, but I would agree, that it is good idea. With variable name kvp I would have no idea what this is just by looking at the code.

I was asked to not use `auto` kw, but spell out the type, which seems verbose, but I would agree, that it is good idea. With variable name `kvp` I would have no idea what this is just by looking at the code.
Author
Member

There's a ton of for (const auto & or for (auto & within Blender code, I assumed using auto for iterators or things being iterated on was common practice. kvp here is "key value pair", but indeed perhaps I should change it to item instead.

There's a ton of `for (const auto &` or `for (auto &` within Blender code, I assumed using `auto` for iterators or things being iterated on was common practice. `kvp` here is "key value pair", but indeed perhaps I should change it to `item` instead.

My bad, I confused items() with values() and assumed this could be named file_entry. I never saw kvp used previously, so it confused me even more.

My bad, I confused `items()` with `values()` and assumed this could be named `file_entry`. I never saw `kvp` used previously, so it confused me even more.
aras_p marked this conversation as resolved
@ -0,0 +571,4 @@
}
/* If we're still beyond capacity, remove individual long-unused (but not within
* last 100 updates) individual frames. */

I guess this is fine, but I would assume, that this is very unlikely to happen. And when it happens, it's quite likely, that upon 10ish UI redraws, the balance would be correct. But I didn't test this, so I can be wrong.

I guess this is fine, but I would assume, that this is very unlikely to happen. And when it happens, it's quite likely, that upon 10ish UI redraws, the balance would be correct. But I didn't test this, so I can be wrong.
Author
Member

This does happen in "heavy" tests that I did -- tons of video clips, large timeline, and several source files that span the whole timeline that are almost always visible. After wild panning and zooming in/out the "very long" source files can fill up the whole cache, and since they are always visible, they never get removed. This part makes sure that even the file itself is visible somewhere, we can discard "long unused" thumbnails from the file.

This does happen in "heavy" tests that I did -- tons of video clips, large timeline, and several source files that span the whole timeline that are almost always visible. After wild panning and zooming in/out the "very long" source files can fill up the whole cache, and since they are always visible, they never get removed. This part makes sure that even the file itself is visible somewhere, we can discard "long unused" thumbnails from the file.
aras_p marked this conversation as resolved
@ -0,0 +586,4 @@
BLI_mutex_unlock(&thumb_cache_lock);
}
void thumbnail_cache_discard_requests_outside(Scene *scene, const rctf &rect)

Since wmJob already copied the requests, I was wondering, whether this even works, but it seems that it does. However I don't understand how... The job is never stopped to update requests. I guess, that some requests are made, then job is spun up, while requests are incoming?

Since `wmJob` already copied the requests, I was wondering, whether this even works, but it seems that it does. However I don't understand how... The job is never stopped to update requests. I guess, that some requests are made, then job is spun up, while requests are incoming?
Author
Member

The job exits when there are no more incoming requests. But since processing requests takes some time, it can (and very often does) happen that while the job is processing some requests, some more are coming in.

The job exits when there are no more incoming requests. But since processing requests takes some time, it can (and very often does) happen that while the job is processing some requests, some more are coming in.
iss marked this conversation as resolved
Aras Pranckevicius added 3 commits 2024-08-27 08:27:44 +02:00
Aras Pranckevicius added 1 commit 2024-08-27 09:02:04 +02:00
Aras Pranckevicius added 1 commit 2024-08-27 09:10:05 +02:00
Sergey Sharybin approved these changes 2024-08-28 11:45:04 +02:00
Sergey Sharybin left a comment
Owner

Just some minor things from me.
I didn't test the PR after the recent fixes, but previous version worked very good.

Just some minor things from me. I didn't test the PR after the recent fixes, but previous version worked very good.
@ -844,7 +849,6 @@ enum {
SEQ_CACHE_PREFETCH_ENABLE = (1 << 10),
SEQ_CACHE_DISK_CACHE_ENABLE = (1 << 11),
SEQ_CACHE_STORE_THUMBNAIL = (1 << 12),

Similar to above.

Similar to above.
@ -620,3 +625,3 @@
SEQ_FILTERY = (1 << 4),
SEQ_MUTE = (1 << 5),
SEQ_FLAG_SKIP_THUMBNAILS = (1 << 6),
/* SEQ_FLAG_SKIP_THUMBNAILS = (1 << 6), */ /* no longer used */

Shall this be renamed to SEQ_FLAG_UNUSED_6 and cleared in do-versions to avoid unused flags from still being in the bitmask?

Shall this be renamed to `SEQ_FLAG_UNUSED_6` and cleared in do-versions to avoid unused flags from still being in the bitmask?
Aras Pranckevicius requested review from Richard Antalik 2024-08-28 21:12:02 +02:00
Richard Antalik approved these changes 2024-08-29 02:28:38 +02:00
Richard Antalik left a comment
Member

Overall this works great.

I did one more kinda edge case test that could be optimized:
If you have a long strip and you zoom out smoothly with Ctrl+MMB, requests are not discarded, which will result in quite a bit of time for rendering thumbnails, that are not drawn. This will happen rarely. Perhaps mainly on Mac with magic mouse. With shorter strips this effect is weirdly not as pronounced though.

Overall this works great. I did one more kinda edge case test that could be optimized: If you have a long strip and you zoom out smoothly with Ctrl+MMB, requests are not discarded, which will result in quite a bit of time for rendering thumbnails, that are not drawn. This will happen rarely. Perhaps mainly on Mac with magic mouse. With shorter strips this effect is weirdly not as pronounced though. <video src="/attachments/e8b86c55-4a1f-4af6-b2af-792ae2233ded" title="2024-08-29 02-19-55.mp4" controls></video>
Aras Pranckevicius merged commit 528471541b into main 2024-08-29 08:27:32 +02:00
Aras Pranckevicius deleted branch vse_thumbs_cache 2024-08-29 08:27:36 +02:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#126405
No description provided.