Page MenuHome

VSE: Disk cache
Needs ReviewPublic

Authored by Richard Antalik (ISS) on Mon, Aug 19, 9:18 AM.

Details

Summary

This patch implements dumping images from cache to HDD.

The main goal of this system is to provide a means to achieve consistent playback speed mainly for strips that are not possible to preview in real time. Function is similar to a proxy system. The main difference is, that lossless compression method is used.
In fact current non-movie proxy system could be replaced by disk cache.
Another use-case may be a storage for strip thumbnails.

(lazy copy of design notes)
For each cached non-temp image image data and supplementary info
are written on HDD. Multiple(DCACHE_IMAGES_PER_FILE) images share the same file.
Each of these files contains header DiskCacheHeader followed by image data.
Zlib compression with user definable level is used to compress image data(per image)
Images are written in oreder in which they are rendered.
Each image must have it's unique place(filename + header entry).
Overwriting of individual entry is not possible.

All images are stored in directory of blend file, or in user-defined path.
Path is human-readable to allow manual manipulation by "advanced users" (copy/delete data)

Stored images can be deleted by invalidation of cache entry.
To make this possible, we need to store information about all contexts used to render (resolution view type etc)
This is done by storing this data in cache struct and on HDD in file used_views.
When cache entry is written, data of last used context is compared to current one.
If there is any difference, we look to file on HDD and append it by current context if needed.
This could slow down performance of invalidation if a lot of contexts were used in the past.
(end of design notes)

My biggest concerns:

  • I am not sure if function BLI_path_sanitize_filename is appropriate. I guess, we should support unicode in paths, and I don't know nothing about unicode, other than, I can mess up big time, if I don't know what I am doing.
  • I don't have much experience with file IO, so there may be hidden bugs or room for improvement.
  • use of G.main in seq_disk_cache_invalidate can be eliminated if I can store main in cache (which we do store in keys to calculate hash)
  • I did not try to protect strings for building path against overflow, but seems that BLI_path functions are safe. User can choose some deep path, when things will fail silently though. Should I rather allocate memory for strings as needed?
  • The way image data is saved may lead to loss of some information that cache would preserve - metadata for example. only colorspace info is stored, because it is necessary to restore float buffers properly. Ideally we would serialize whole ImBuf struct, but I didn't want to overcomplicate things now.

Diff Detail

Repository
rB Blender
Branch
dcache (branched from master)
Build Status
Buildable 4863
Build 4863: arc lint + arc unit

Event Timeline

  • add flag to eneable disk cache and compression level
  • I guess I will close this with one huge commit then...
Richard Antalik (ISS) retitled this revision from VSE: Disk cache [wip] to VSE: Disk cache.Mon, Sep 2, 1:45 AM
Richard Antalik (ISS) edited the summary of this revision. (Show Details)
  • remove TODO's
  • add design notes
Richard Antalik (ISS) edited the summary of this revision. (Show Details)Mon, Sep 2, 3:21 AM
Richard Antalik (ISS) edited the summary of this revision. (Show Details)
Richard Antalik (ISS) edited the summary of this revision. (Show Details)Mon, Sep 2, 9:22 PM
Richard Antalik (ISS) updated this revision to Diff 17764.
  • Fix comment style
Richard Antalik (ISS) edited the summary of this revision. (Show Details)Mon, Sep 2, 11:04 PM
Sergey Sharybin (sergey) requested changes to this revision.Fri, Sep 6, 10:37 AM

Some feedback while scrolling through the code getting it compiled.

The unicode issue i am not quite sure. I would expect BLI_fopen() to properly handle that?

More feedback later.

source/blender/blenkernel/intern/seqcache.c
348

Guess it should be something like:

end = seq->enddisp < seq_changed->enddisp ? seq->enddisp : seq_changed->enddisp;
352
for (; start <= end; start += DCACHE_IMAGES_PER_FILE) {
366
for (; start <= end; start += DCACHE_IMAGES_PER_FILE) {
387

Don't think we should be having such a lower level implementations in areas implemented. This is something up to BLI.

For example, we've already have BLI_file_ungzip_to_mem() and BLI_file_gzip(). Some tweaks might be needed to them to support in-memory or file-path based operations.

511
memset(header, 0, sizeof(*header));
570

Should it be bytes_written == 0 ? Because everything which affects on a return value of deflate_imbuf_to_file is unsigned, and can not be negative.

608–609

You'll have integer overflow on the right hand side before assignment happens. You have to explicitly cast key->context.rectx to uint64_t to avoid this.

This revision now requires changes to proceed.Fri, Sep 6, 10:37 AM

Did some quick tests to work on a little project, and have some usability concerns.

In short: the system feels much less reliable with the disk cache.

Options

I don't think those belong to sequencer. but rather to user preferences where you specify the following:

  • Amount of RAM allowed to be used for caching
  • Folder for the disk cache
  • Size allowed to be used on the disk

The path on disk is really a preference, since it depends on a specific hardware configuration (which could have a dedicated or desired SSD/NVMe to be used for the cache).
Also, opening .blend files over the network should not start using network drive for cache.

Th default path could point to folder like XDG's cache.

Cache format

Seems the disk cache is supposed to be permanent. Which is fine. But then there is no version indication in the files, making it hard to extend it further.

There is also no endian conversion. But if cache is not supposed to be shareable, then it's fine.

Cache invalidation

This is the biggest concern so far.

Seems there is a lot of missing invalidation. Not sure if those are design limitations or a bugs in the code, but the following scenarios failed:

Scenario 1:

  • Add image strip.
  • Add blur effect.
  • Change Blur size => view does not refresh.

Scenario 2:

  • Add image strip.
  • Add blur effect.
  • Change Blur size.
  • Remove image and effect.
  • Add another image.
  • Add Blur effect => Previous image is displayed.

Creating a meta-strip does not remove "old" strips caches as well.

Not sure why is it happening though. Would imagine that the disk cache is an extension of the in-memory cache, living under same API and just using disk as a storage when running out of RAM allowed to be used.
Doing it this way would avoid issues like mentioned above, be transparent for all the areas which are currently using or are planning to use the moviecache.

source/blender/blenkernel/intern/seqcache.c
628–637

You're closing file multiple times here, which should not be happening.
Caused memory to be freed twice and other nasty things.

Richard Antalik (ISS) marked 7 inline comments as done.Mon, Sep 9, 1:53 AM
Richard Antalik (ISS) edited the summary of this revision. (Show Details)
Richard Antalik (ISS) updated this revision to Diff 17965.
  • review
  • cleanup, add version to header

Options
I don't think those belong to sequencer. but rather to user preferences where you specify the following:

  • Amount of RAM allowed to be used for caching

OK, already in place

  • Folder for the disk cache

I don't have a strong opinion here. It is more accessible in side panel and custom dir for proxies is also in side panel.

  • Size allowed to be used on the disk

This is unnecessary IMO.

  • these days you will be likely notified by OS in multiple stages, that you are running low on space
  • how would you even check this? Also if done periodically this can generate quite a lot of IO calls possibly affecting performance.

The path on disk is really a preference, since it depends on a specific hardware configuration (which could have a dedicated or desired SSD/NVMe to be used for the cache).
Also, opening .blend files over the network should not start using network drive for cache.
Th default path could point to folder like XDG's cache.

Not sure what XDG stands for.
I saw particle caches to use blend file dir as default
I would rather not use OS-specific "temp" dirs. It would be OK for sub MB size files, but we are talking multiple GB's this should be placed "by hand" by default.

Cache format
Seems the disk cache is supposed to be permanent. Which is fine. But then there is no version indication in the files, making it hard to extend it further.
There is also no endian conversion. But if cache is not supposed to be shareable, then it's fine.

There is no reason to not have conversion - it can be specified in header. I will add this tomorrow.

Cache invalidation
This is the biggest concern so far.
Seems there is a lot of missing invalidation. Not sure if those are design limitations or a bugs in the code, but the following scenarios failed:

There is option to enable invalidation for disk cache - invalidate_disk_cache I guess it should be on by default... With this option enabled, I wasn't able to replicate both issues.
I was thinking that building cache is quite time consuming and users would rather have this option disabled most of the time.

There is quite a bit of missing invalidation, I am doing sessions and fixing that.
Disk cache behavior should be identical to cache however.
There is possibility of displaying old content by accident - at this point it's quite hard to predict how often that would happen. With content that is designed to be pernament, it's also hard to deal with.

This patch is primary a replacement for scene strip proxies, which had no invalidation. And users still want this feature back.

source/blender/blenkernel/intern/seqcache.c
387

Functions you suggested use gzopen() which I would say is optimized to work on pure gzip stream or .gz file.

My format combines multiple gzip streams and raw header. Even if whole file would be single gzip stream, seeking in such file would be very slow.

I can make BLI functions like BLI_gzip_mem_to_file_at_pos(buf, len, file, pos, comp_lvl) and BLI_ungzip_file_to_mem_at_pos(buf, len, file, pos). There will be some miniscule performance loss because BLI_ungzip_file_to_mem_at_pos() will have no idea how long the stream will be, and will read in chunks.

  • Add endian encoding detection and conversion
Richard Antalik (ISS) marked an inline comment as done.Wed, Sep 11, 8:24 AM

Folder for the disk cache

I don't have a strong opinion here. It is more accessible in side panel and custom dir for proxies is also in side panel.

Custom directory for proxies is rather a weak concept on it's own. Custom cache directory is not meant to be per-blend file, it is a preference on a specific computer.

This is unnecessary IMO.

These days you will be likely notified by OS in multiple stages, that you are running low on space how would you even check this?

Even after moving caches to a single centralized directory it is still unnecessary annoying to manually worry about clearing cache folders.

Also if done periodically this can generate quite a lot of IO calls possibly affecting performance.

I don't see this an issue in real life world. People will almost certainly use SSD/NVMe for cache, and those can push a lot of IOPS. Also, such things will most likely be cached by the kernel, having sequential requests very fast.

Not sure what XDG stands for.

https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html
Specifically, XDG_CACHE_HOME. Other OS have equivalents.

I would rather not use OS-specific "temp" dirs. It would be OK for sub MB size files, but we are talking multiple GB's this should be placed "by hand" by default.

It is not "temp", it is cache. I do not see any advantages of placing cache "by hand". Disadvantages i've listed in previous reply.

There is option to enable invalidation for disk cache - invalidate_disk_cache I guess it should be on by default...

I don't think that should be an option at all. Cache should just work, without adding burden on artists to always remember to keep track of those fewzillion different options.

There is quite a bit of missing invalidation, I am doing sessions and fixing that.
Disk cache behavior should be identical to cache however.

Ideally disk cache should be an extension of in-memory cache. And have the same mechanisms for invalidation.

There is possibility of displaying old content by accident - at this point it's quite hard to predict how often that would happen. With content that is designed to be pernament, it's also hard to deal with.

I don't think we should be adding anything which makes sequencer even less predictable.

I think the good cache system design should work transparently for artists, helping them to achieve good playback performance without adding a burden of requirement to keep track of lots of different settings and other manual work.

source/blender/blenkernel/intern/seqcache.c
400

Control reaches end of non-void function.

412

Same as above.

431

And again.

I agree, that user shouldn't be required to maintain the cache.

It would be possible to limit the cache to certain size and then remove files based on file open timestamp.
I think, there are no BLI functions that I can use to implement such functionality. I can write them, but I will need more time.

As for using OS cache storage - Disk cache is designed to be pernament, not limited to session duration.
I guess we could require user to specify disk cache directory before it is possible to use it?

Disk cache behavior should be identical to cache however.

Ideally disk cache should be an extension of in-memory cache. And have the same mechanisms for invalidation.

Disk cache is an extension of in-memory cache, and it does have the same invalidation mechanism.

Fixing missing invalidation is not in scope of this patch.

As for using OS cache storage - Disk cache is designed to be pernament, not limited to session duration.
I guess we could require user to specify disk cache directory before it is possible to use it?

That was the suggestion: make it a setting in user preferences, defaulting to standard cache folder. (cache folder is not limited by a session, have a look at your ~/.cache/).

Disk cache is an extension of in-memory cache, and it does have the same invalidation mechanism.

Ok, think i see now. A bit weird why BKE_sequencer_cache_cleanup() doesn't do any disk cache, but BKE_sequencer_cache_cleanup_sequence() does.

Fixing missing invalidation is not in scope of this patch.

If the disk cache have same missing invalidations and does respect "Refresh Sequencer" think it's fine.

On a side note: it seems that only the top-most sequence gets files stored on the disk. Is it intentional? If only final result is cached we should be able to remove per-strip folders.

Disk cache is an extension of in-memory cache, and it does have the same invalidation mechanism.

Ok, think i see now. A bit weird why BKE_sequencer_cache_cleanup() doesn't do any disk cache, but BKE_sequencer_cache_cleanup_sequence() does.

BKE_sequencer_cache_cleanup() clears whole RAM cache, because view context(resolution etc) has changed, not because output of sequence has changed.

I guess refresh sequencer should clear disk cache also, because using that operator implies,that something is wrong.

On a side note: it seems that only the top-most sequence gets files stored on the disk. Is it intentional?

That is default behavior - final output of the sequencer, to optimize cached time vs cache capacity.

Otherwise, what get's cached is configurable on per-scene and per-sequence basis. This can be wrapped in some user-friendly way, but it's quite raw at this point. I am not very good at making systems user-friendly.

My idea was, that there could be operator for building sequence disk cache, that would enable cahce at desired level on that sequence (raw / preprocessed / composite) and pre-render it. So the functionality would be similar to proxy system.

In any case,it's too late for this patch to be included in current release, so will focus on fixing bugs and working on smaller features and return to this earlier in 2.82 cycle.

I guess refresh sequencer should clear disk cache also, because using that operator implies,that something is wrong.

Exactly. Probably, matter of passing extra argument to BKE_sequencer_cache_cleanup().

Also thing to consider is that render farm should not save disk cache.
If the disk cache option is moved to user preferences this becomes simpler, but still would be nice to find a solution which would not require extra configuration of Blender.
Maybe, animation render should only use disk cache as read-only?

That is default behavior - final output of the sequencer, to optimize cached time vs cache capacity.

Think that's fine. In fact, not sure it should be configurable. See further notes.

Otherwise, what get's cached is configurable on per-scene and per-sequence basis.

Per-scene-strip is probably fine, since those are kind of known to be slow. But doing disk cache on per-strip basis seems weird to me. We should optimize strips evaluation rather than going into a mess of per-strip knowbs. One thing to consider in this direction is to make sequencer more scalable (as in, don't run all the effects and what not on the original resolution, which gets scaled down on display anyway). Sure, that's not in the scope of this change, but considering it will allow simplifying user experience with this change.

So to move forward i'm thinking plan like this:

  • Move disk cache settings to User Preferences. This will include path to where cache is stored (defaulting to standard cache directory on the platform?), and allowed size of the disk cache. Also, compression ratio could go there (if we really need it. Is just not really clear for users which value to use, so don't think it will be really set to absolute optimal, meaning, we can as well just use some good-enough ballpark value).
  • Do caching for final output, scene and meta strips (the rest of the strips i don't think should be covered with disk cache).
  • Profit?

P.S. One thing i am still unsure of is putting everything in one file. That makes it quite impossible to invalidate parts of the timeline (and i think partial invalidation is something we should consider in long term).

source/blender/blenkernel/intern/seqcache.c
984–986

Shouldn't this be inside seq_cache_lock(scene); as well?

Maybe, animation render should only use disk cache as read-only?

Animation render(final production output) should never read cache. Cache is for preview only, which can be significantly lower quality then production output.

That is default behavior - final output of the sequencer, to optimize cached time vs cache capacity.

Think that's fine. In fact, not sure it should be configurable. See further notes.

Otherwise, what get's cached is configurable on per-scene and per-sequence basis.

Per-scene-strip is probably fine, since those are kind of known to be slow. But doing disk cache on per-strip basis seems weird to me. We should optimize strips evaluation rather than going into a mess of per-strip knowbs. One thing to consider in this direction is to make sequencer more scalable (as in, don't run all the effects and what not on the original resolution, which gets scaled down on display anyway). Sure, that's not in the scope of this change, but considering it will allow simplifying user experience with this change.

If is per scene strip, it is simpler to be written per any strip. Another thing is what we allow to do in UI/API.
Reason for having configuration for any strip is to allow for various workflows - grading would require raw or composite image cache. Applying keyed or computed transparent strips will require postprocessed or composite cache.

Invalidation algorithm doesn't look at cache content and then decides what to do. Instead it assumes, that everything is cached to maximum extent, so nothing is missed.
Sequencer RAM cache has a hack to chain images as they are rendered in frame, but this is only to lower number of iterations through cache content to clear those images and it's intended to be utilized only when cache reaches it's capacity and starts to 'recycle' items. Doesn't affect invalidation.

Perhaps I am thinking too much ahead here, where sequencer is more integrated with features as motion tracking and able to use node system to do stuff.

So to move forward i'm thinking plan like this:

  • Move disk cache settings to User Preferences. This will include path to where cache is stored (defaulting to standard cache directory on the platform?), and allowed size of the disk cache. Also, compression ratio could go there (if we really need it. Is just not really clear for users which value to use, so don't think it will be really set to absolute optimal, meaning, we can as well just use some good-enough ballpark value).

Agree.
I can change use_compression to be bool - 0 is for no compression and values 1-9 are about the same.
I would keep this option present for variety of HW configuration. Use of compression makes huge difference in filesize, which can be used on smaller drives (even single HDD in some cases). You would probably want to use raw for ultra high FPS footage and on machines with slower CPU.

  • Do caching for final output, scene and meta strips (the rest of the strips i don't think should be covered with disk cache).

Agree. I will change defaults. We can hide other settings in UI, but they are intentionally kinda burried, so they are not touched unless you really need to do it.

  • Profit?

I guess. This is still a workaround, not a solution to performance issues, but it's essential feature IMO.

P.S. One thing i am still unsure of is putting everything in one file. That makes it quite impossible to invalidate parts of the timeline (and i think partial invalidation is something we should consider in long term).

We can store less images in a file. I think 100 is not too much and not too little.
Thing is, that kernel cache only so much files. 1 file per image is unusable on many systems. About 20 would be minimum I think.
We can eliminate slight delays on uncached file access with prefetching D5386: VSE: prefetching.

Animation render(final production output) should never read cache. Cache is for preview only, which can be significantly lower quality then production output.

Could be handy for playblasts perhaps. To quicker gain an idea how, for example, color grading is going to look like applied on top a scene strip.
Something where input from actual VSE users.

I can change use_compression to be bool - 0 is for no compression and values 1-9 are about the same.

On my E5-2699 with EVO 960 NVMe should i use value of 9? 8? 7? Point is: such a grained values are hard to specify, even by technical people.
Think it should be more a preset-based thing:

  • No compression (less CPU overhead, bigger size on the disk)
  • Medium compression (balanced CPU and disk usage overhead)
  • Best compression (biggest CPU overhead, lowest size on the disk)

That is something more tangible by users.

You would probably want to use raw for ultra high FPS footage and on machines with slower CPU.

Actually, depends. If the disk is even slower than the CPU you'd rather minimize file size somewhat to avoid bottleneck of slow disk access on a big files.

We can store less images in a file. I think 100 is not too much and not too little.

Ah, so it's not a single file for all frames, but some "chunked" storage. Is not too bad then, but see the next note.

Thing is, that kernel cache only so much files.

Are we talking OS kernel here?
In Linux's page cache i am not aware of any setting which is related on number of files. It's more or less global cache for all block devices. Not quite scientific, but with big enough RAM i can have 1000s of files cached.

We can eliminate slight delays on uncached file access with prefetching

In the context of disk cache we shouldn't rely on OS's page cache. Disk cache should be considered cold. Otherwise what's the point of it, just use higher value for RAM cache :)