Details

Reviewers

Commits

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

Richard Antalik (ISS) created this revision.Aug 1 2019, 12:46 AM

Harbormaster completed remote builds in B4241: Diff 16748.Aug 1 2019, 12:46 AM

Duarte Farrajota Ramos (duarteframos) awarded a token.Aug 2 2019, 11:21 AM

Duarte Farrajota Ramos (duarteframos) added a subscriber: Duarte Farrajota Ramos (duarteframos).Aug 2 2019, 11:27 AM

Luciano Muñoz Sessarego (looch) awarded a token.Aug 12 2019, 4:30 PM

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

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) mentioned this in D3934: VSE cache with frame prefetching (WIP).Aug 16 2019, 10:11 PM

Maciej Jutrzenka (Kramon) added a subscriber: Maciej Jutrzenka (Kramon).Aug 16 2019, 10:20 PM

Update diff for review.

Harbormaster completed remote builds in B4486: Diff 17254.Aug 19 2019, 8:52 AM

Richard Antalik (ISS) added inline comments.Aug 19 2019, 8:52 AM

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

Brecht Van Lommel (brecht) added inline comments.Aug 19 2019, 10:14 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

Harbormaster completed remote builds in B4528: Diff 17321.Aug 21 2019, 1:34 AM

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); }

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

Harbormaster completed remote builds in B4580: Diff 17471.Aug 24 2019, 1:51 AM

In D5386#127080, @Brecht Van Lommel (brecht) wrote:

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

Harbormaster completed remote builds in B4581: Diff 17473.Aug 24 2019, 2:34 AM

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

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).

In D5386#127450, @Richard Antalik (ISS) wrote:

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

Richard Antalik (ISS) mentioned this in D5524: VSE: Disk cache.Sep 13 2019, 2:22 PM

Closed by commit rBab3a9dc1ed28: VSE: prefetching (authored by Richard Antalik (ISS)). · Explain WhySep 14 2019, 3:01 AM

This revision was automatically updated to reflect the committed changes.

Richard Antalik (ISS) added a commit: rBab3a9dc1ed28: VSE: prefetching.

VSE: prefetching
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 16748

release/scripts/startup/bl_ui/space_sequencer.py

source/blender/blenkernel/BKE_sequencer.h

source/blender/blenkernel/CMakeLists.txt

source/blender/blenkernel/intern/seqcache.c

source/blender/blenkernel/intern/seqprefetch.c

source/blender/blenkernel/intern/sequencer.c

source/blender/blenloader/intern/readfile.c

source/blender/editors/space_sequencer/sequencer_draw.c

source/blender/makesdna/DNA_sequence_types.h

source/blender/makesrna/intern/rna_sequencer.c

VSE: prefetchingClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 16748

release/scripts/startup/bl_ui/space_sequencer.py

source/blender/blenkernel/BKE_sequencer.h

source/blender/blenkernel/CMakeLists.txt

source/blender/blenkernel/intern/seqcache.c

source/blender/blenkernel/intern/seqprefetch.c

source/blender/blenkernel/intern/sequencer.c

source/blender/blenloader/intern/readfile.c

source/blender/editors/space_sequencer/sequencer_draw.c

source/blender/makesdna/DNA_sequence_types.h

source/blender/makesrna/intern/rna_sequencer.c

VSE: prefetching
ClosedPublic
Actions

Revision Contents
Changeset List