Page MenuHome

Fix T88237: Blender crash in VSE prefetch
AbandonedPublic

Authored by Sergey Sharybin (sergey) on Aug 9 2021, 4:08 PM.

Details

Summary

There are two sources of problem involved.

One of them is that the sound handles creation is to be delayed until
scene/VSE evaluation. This was violated for scene strips duplication
(a code which is used for copy-on-write). The Sound strips already did
a proper thing to delay sound handle creation, so just did the same
for the Scene strip.

The other issue is related on how prefetch localized data, combined
together with the scene strip rendering. It seems that the prefetch
relies on the dependency copy-on-write mechanism to localize data,
which works for "regular" strips. It was failing for scene strips
because rendering of scene strips creates a dependency graph used
by a render engine. And it happened to be a second level of copy on
write, which was not supported yet.

Now it is possible to create dependency graph from an evaluated
state of another dependency graph. Such dependency graph can never
become active, but other than that it "should just work".

There might be other ways of fixing the issue, like localizing part
of bmain, but then one would need to manually maintain required
dependencies.

Further improvement would be to avoid creating copy-on-write data
blocks in the dependency graph, because in-place modification is
possible in-place.

Still need to verify that modifiers are applied correctly (i.e.
that subsurf is not applied twice). Before spending more time on
the topic wanted to have design feedback from developers who are
involved in the areas and see if the direction this patch is moving
towards is good.

Diff Detail

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

Event Timeline

Sergey Sharybin (sergey) requested review of this revision.Aug 9 2021, 4:08 PM
Sergey Sharybin (sergey) created this revision.

Thanks for looking into this. I definitely welcome posibility to render scenes in background.

There are some issues still, best presented with following file:

When you open the file, you can see, that prefetch is rendering frames in background, but interface is locked during rendering and rendered image is incorrect (I assume this is due to patch being WIP).

But solely due to locked interface, I think patch D11247 would be still needed for prefetch to not render scene strips in background. I have tried to force seq_render_scene_strip() to always use offscreen rendering, and it was fairly fast, but UI was rather colorful. So another question is, would it be OK to have global thread lock for drawing loop, to allow scene rendering in background?

@Richard Antalik (ISS), did you verify that the depsgraph used by the prefetch is correct? (as in, contains all data-blocks needed to properly render sequencer)

Disabling prefetch for scene strips which needs to be rendered seems to make sense from interactivity point of view. In fact, disabling such prefetch is safer and faster to implement option to resolve the crash. Avoiding the crash should be a priority.

Now, even if the prefetch is disabled there are two points from this patch:

  • Need to apply fixes done for the scene_sound.
  • Possibility to create depsgraph from an evaluated state seems to pop-up every now and then, so might still be interesting to finish this patch.

@Richard Antalik (ISS), did you verify that the depsgraph used by the prefetch is correct? (as in, contains all data-blocks needed to properly render sequencer)

No I haven't. Does that mean that depsgraph used by the prefetch would need to localize also scene that is to be rendered? Is there API for that?
Right now it seems to render scene background only (not sure of which scene too)

Disabling prefetch for scene strips which needs to be rendered seems to make sense from interactivity point of view. In fact, disabling such prefetch is safer and faster to implement option to resolve the crash. Avoiding the crash should be a priority.

Right, so I will update patch so it is as nice as possible and set for review.

Now, even if the prefetch is disabled there are two points from this patch:

  • Need to apply fixes done for the scene_sound.
  • Possibility to create depsgraph from an evaluated state seems to pop-up every now and then, so might still be interesting to finish this patch.

Yes, that still sounds useful.

No I haven't. Does that mean that depsgraph used by the prefetch would need to localize also scene that is to be rendered? Is there API for that?

Depsgraph is kind of localized copy already. I'll have a dig later.

Right, so I will update patch so it is as nice as possible and set for review.

Cool!

Right, so I will update patch so it is as nice as possible and set for review.

Are you still planning updates to this patch? If so, please mark it as "changes planned".

That was note about different patch AFAIR.
The original issue this patch is addressing is solved in a different way. So while it might still be interesting to support building depsgraph from evaluated state of another one better be done when the actual need arises. We can always keep this patch in mind!