VSE: Faster and more consistent thumbnail cache #126405
No reviewers
Labels
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#126405
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "aras_p/blender:vse_thumbs_cache"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Implementing part of design outlined in #126087.
index within media file, and any extra data (e.g. steam index for multi-steam videos)
and here's how it looks now (ignore orange label in the middle; WIP debugging aid):
timeline, or F12 render.
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.
90cff02ec0
to88a783ae3a
@blender-bot build
WIP: VSE: new thumbnail cacheto VSE: Faster and more consistent thumbnail cache@blender-bot package
Package build started. Download here when ready.
@ -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?
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.
@ -0,0 +36,4 @@
// #define DEBUG_PRINT_THUMB_JOB_TIMES
static ThreadMutex thumb_cache_lock = BLI_MUTEX_INITIALIZER;
Should be using
std::mutex
andstd::scoped_lock
?Ah ok! I was not sure whether BLI utilities or C++ std is preferred. Will change.
@ -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?
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.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.
@ -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 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.
@ -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.
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:
"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.
@ -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.
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.
@ -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.
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.
@ -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.
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?
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
andblender::ed::seq
codebases.@ -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.Good catch! Pushed a fix, the thumbnails job was not cleaned up properly when destroying the thumbnail cache (as part of Ctrl+E refresh).
@ -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.Defensive habits, I guess. Will replace with an assert.
@ -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
?Good point, will do.
@ -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.
It solves an issue of new incoming requests that would be piled up. Consider:
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.
@ -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 namekvp
I would have no idea what this is just by looking at the code.There's a ton of
for (const auto &
orfor (auto &
within Blender code, I assumed usingauto
for iterators or things being iterated on was common practice.kvp
here is "key value pair", but indeed perhaps I should change it toitem
instead.My bad, I confused
items()
withvalues()
and assumed this could be namedfile_entry
. I never sawkvp
used previously, so it confused me even more.@ -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.
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.
@ -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?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.
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.
@ -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?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.