Page MenuHome

VSE: prefetching
ClosedPublic

Authored by Richard Antalik (ISS) on Aug 1 2019, 12:46 AM.

Details

Summary

A bit of refactoring was done from last "version", no cleanup of old code in this diff.

Short description:
When enabled prefetching(preview panel>view settings), a pernament running job is created, that will render frames in the background until the cache is full.
Cache recycling behavior will be changed to "free furthest frame to the left of playhead if possible, otherwise rightmost frame".
If the cache is not filled fast enough, prefetch job suspends itself at the last moment and will wait until it has chance to "catch up"
Effectively this will decouple rendering to separate thread, so rendering itself is a bit faster (more frequent technically).

Prefetch job contains a copy of scene in which sequencer is used. This copy is updated every time a cache is invalidated.

It should be possible to use prefetch API to create jobs running in in-house jobs system, so prefetch job manager was quite quickly dropped(also would introduced too much
ugly code). Because prefetch is pernament in it's nature, current job system is not usable.

  • There is still collision with blf library.
  • Scene strips are rendered in "rendered" mode only (other modes are not supported if rendered from non-main thread).

I am not aware of any other issues (but haven't really used this patch for a while).

Diff Detail

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

Event Timeline

Getting a build error with this patch.

/home/brecht/dev/worktree/source/blender/blenkernel/intern/seqprefetch.c: In function ‘BKE_sequencer_prefetch_update_scene’:
/home/brecht/dev/worktree/source/blender/blenkernel/intern/seqprefetch.c:248:3: error: implicit declaration of function ‘BKE_sequencer_prefetch_stop’; did you mean ‘BKE_sequencer_prefetch_start’? [-Werror=implicit-function-declaration]
   BKE_sequencer_prefetch_stop(pfjob);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   BKE_sequencer_prefetch_start
At top level:
/home/brecht/dev/worktree/source/blender/blenkernel/intern/seqprefetch.c:227:13: warning: ‘seq_prefetch_stop’ defined but not used [-Wunused-function]
 static void seq_prefetch_stop(PrefetchJob *pfjob)
             ^~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
Brecht Van Lommel (brecht) requested changes to this revision.Aug 14 2019, 6:33 PM

I didn't get to fully reviewing this today, mainly did just one pass reading through the code.

There's nothing problematic in the overall design that jumped out at me though.

source/blender/blenkernel/BKE_sequencer.h
101

Maybe rename to cache_id? Or at least something that explains a bit what this is for.

357

area -> time_range maybe a bit more clear terminology

source/blender/blenkernel/intern/seqcache.c
235–237

Can this comment explain what it does and why it's easier to fit it in here?

This function in general perhaps could use comments about what it does.

736

Nitpick: this line could be moved into its own function and code shared with BKE_sequencer_cache_recycle_item.

source/blender/blenkernel/intern/seqprefetch.c
41

Don't use .. for include paths.

Currently we don't call the window manager from blenkernel, which is why it's not part of the include paths. We should not add a dependency in this direction.

The simple solution would be to just loop through the screens in main, only active screens will have an animtimer or scrubbbing set.

72

Maybe renames to num_frames_prefetched or something like that?

It's not immediately obvious from the name that this is a number of frames.

75–78

We use volatile in a few more places like this for threading, but this doesn't actually do anything useful and can be left out.

scrubbing is unused.

358

One potential issues here, is that the depsgraph is built for a specific view layer. I'm not sure what happens if you for example delete a view layer and so also it associated depsgraph, if this pointer will remain valid.

source/blender/makesrna/intern/rna_sequencer.c
1934

Naming convention: use_prefetch.

This revision now requires changes to proceed.Aug 14 2019, 6:33 PM
Richard Antalik (ISS) marked 8 inline comments as done.
  • Update diff for review.
source/blender/blenkernel/BKE_sequencer.h
101

renamed and added a comment.
I wasn't sure, if I should create some enum for this purpose to avoid confusion and even collision in worst case.

I guess I should, so I did now.

source/blender/blenkernel/intern/seqprefetch.c
358

I can create a new depsgraph instance for my new scene.
Not sure what is the minimal setup right now.

Also I am not sure, what is the lifecycle of a depsgraph to recreate "native" rendering situation.

I just rolled the dice here to be honest.

Richard Antalik (ISS) edited the summary of this revision. (Show Details)Aug 19 2019, 8:59 AM
source/blender/blenkernel/intern/seqprefetch.c
358

You can create a "render pipeline depsgraph" like render_init_depsgraph, which contains only the scene with sequencer and compositor, without any objects. So it's pretty lightweight and something you can create for a prefetch thread like this.

  • Use own depsgraph
  • Use depsgraph to copy scene instead custom method

A few warnings to fix:

/home/brecht/dev/worktree/source/blender/blenkernel/intern/seqprefetch.c: In function ‘BKE_sequencer_prefetch_is_playing’:
/home/brecht/dev/worktree/source/blender/blenkernel/intern/seqprefetch.c:83:60: warning: unused parameter ‘scene’ [-Wunused-parameter]
 bool BKE_sequencer_prefetch_is_playing(Main *bmain, Scene *scene)
                                                            ^~~~~
/home/brecht/dev/worktree/source/blender/blenkernel/intern/seqprefetch.c: In function ‘BKE_sequencer_prefetch_is_scrubbing’:
/home/brecht/dev/worktree/source/blender/blenkernel/intern/seqprefetch.c:93:62: warning: unused parameter ‘scene’ [-Wunused-parameter]
 bool BKE_sequencer_prefetch_is_scrubbing(Main *bmain, Scene *scene)
                                                              ^~~~~
[15/1551] Building C object source/blender/blenkernel/CMakeFiles/bf_blenkernel.dir/intern/seqcache.c.o
/home/brecht/dev/worktree/source/blender/blenkernel/intern/seqcache.c:170:15: warning: function declaration isn’t a prototype [-Wstrict-prototypes]
 static size_t seq_cache_get_mem_total()
               ^~~~~~~~~~~~~~~~~~~~~~~
/home/brecht/dev/worktree/source/blender/blenkernel/intern/seqcache.c:322:14: warning: no previous prototype for ‘seq_cache_get_item_for_removal’ [-Wmissing-prototypes]
 SeqCacheKey *seq_cache_get_item_for_removal(Scene *scene)
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I found this problem in testing:

  • Enable Prefetch frames
  • Add 4K video strip (one that plays back very slow at 4fps, I used this one)
  • Prefetching starts
  • Start playing animation quickly after, so it goes beyond the cached frames
  • Press Escape to cancel
  • For about 10s, music keeps playing and interface is unresponsive, instead of cancelling immediately.

I can imagine some locking causes an unresponsive interface for about the duration of rendering one frame, but this is a lot longer of course.

Scrubbing this video is quite unresponsive with or without prefetching, but that's not really unexpected.

source/blender/blenkernel/intern/sequencer.c
4123–4135

Can some of this logic be moved into seqprefetch.c?

BKE_sequencer_give_ibuf is getting a bit long, and better to have this prefetching logic together.

source/blender/editors/space_sequencer/sequencer_draw.c
1232–1243

Can this logic be moved in seqprefetch.c so it's closer together?

Something like:

if (BKE_sequencer_prefetch_need_redraw(bmain, scene) ) {
  WM_event_add_notifier(C, NC_SCENE | ND_SEQUENCER, NULL);
}
Brecht Van Lommel (brecht) requested changes to this revision.Aug 22 2019, 6:35 PM

Other responsiveness issues than that this seems to be working fine in my tests.

This revision now requires changes to proceed.Aug 22 2019, 6:35 PM
  • Fix warnings
  • Refactor logic
  • For about 10s, music keeps playing and interface is unresponsive, instead of cancelling immediately.

To me it seems that those 10s are about average time it takes to seek to a frame.
You can reproduce this without prefetch by lowering cache limit or just make sure, that frame where you start playing is no longer cached. This should result in the same effect.

A note to update: I modified behavior, so you don't call for scene update directly, but instead just stop prefetching.
Scene will be updated on restart. This is to allow changes to SeqRenderData context when user will change render size. I could make explicit function to change render size, but this feels more future-proof.
I guess I will at least leave a comment in the code, that this is how it's done...

  • add comment, cleanup

Maybe the prefetch should be switched off during 'transform' operations, to avoid delays during those operations?

Richard Antalik (ISS) marked 4 inline comments as done.Aug 29 2019, 11:22 PM

Just checked if I missed something here, and sure enough, I forgot to mark inlines as done.

I think this is ok to go (the endframe fix should be trivial).

To me it seems that those 10s are about average time it takes to seek to a frame.
You can reproduce this without prefetch by lowering cache limit or just make sure, that frame where you start playing is no longer cached. This should result in the same effect.

Yes, I think that would explain it.

I guess I didn't expect extra seeking to happen just by using prefetch, I don't know the implementation here at all. Maybe some seeking state for current frame, animation playback and prefetching can be decoupled?

source/blender/blenkernel/intern/seqprefetch.c
328

One more issue found in testing: prefetching stops one frame before the end frame.

I think this should be <= instead of <.

This revision is now accepted and ready to land.Aug 30 2019, 4:17 PM
This revision was automatically updated to reflect the committed changes.