Prefetching can corrupt .blend files #78837

New Issue

Jordan Lawson · 2020-07-11T19:29:22+02:00

Jordan Lawson commented

2020-07-11 19:29:22 +02:00

System Information
Operating system: Windows-10-10.0.18362-SP0 64 Bits
Graphics card: Radeon RX 570 Series ATI Technologies Inc. 4.5.13572 Core Profile Context 19.11.2 26.20.13031.15006

Blender Version
Broken: version: 2.90.0 Alpha, branch: master, commit date: 2020-07-04 15:31, hash: 464aaf2701

Short description of error.
Prefetching can corrupt .blend files. Following steps may not work on all hardware. This is quite random issue as far as I can tell.

Exact steps for others to reproduce the error
#78837.blend

Open file
Add color strip
Change length to 2500
While color strip is prefetching, save file
.blend file is now broken - seq->effectdata is corrupted.

**System Information** Operating system: Windows-10-10.0.18362-SP0 64 Bits Graphics card: Radeon RX 570 Series ATI Technologies Inc. 4.5.13572 Core Profile Context 19.11.2 26.20.13031.15006 **Blender Version** Broken: version: 2.90.0 Alpha, branch: master, commit date: 2020-07-04 15:31, hash: `464aaf2701` **Short description of error**. Prefetching can corrupt .blend files. Following steps may not work on all hardware. This is quite random issue as far as I can tell. **Exact steps for others to reproduce the error** [#78837.blend](https://archive.blender.org/developer/F8694362/T78837.blend) - Open file - Add color strip - Change length to 2500 - While color strip is prefetching, save file .blend file is now broken - `seq->effectdata` is corrupted.

Jordan Lawson commented

2020-07-11 19:29:22 +02:00

Added subscriber: @jordanp.lawson

Jordan Lawson commented

2020-07-11 19:42:11 +02:00

On further testing many other actions seem to crash the file, like rendering from the first frame. Just before these crashes began, I used the opacity property to have my intro fade in from black, maybe it's related.

Richard Antalik commented

2020-07-13 15:08:11 +02:00

Added subscriber: @iss

Richard Antalik commented

2020-07-13 15:08:11 +02:00

Changed status from 'Needs Triage' to: 'Confirmed'

Richard Antalik commented

2020-07-13 15:08:11 +02:00

Thanks for report.

There seems to be issue with prefetching corrupting .blend files. There have been several reports for this, I have not been able to reproduce this issue until now.

I will leave notes for me to look into this:

There is thread race with depsgraph: this seems to happen with color strips mainly. To reproduce, add color strip and copy / paste it several times. .
Corruption: have array with color strips, save file while prefetching them.

Thanks for report. There seems to be issue with prefetching corrupting .blend files. There have been several reports for this, I have not been able to reproduce this issue until now. I will leave notes for me to look into this: - There is thread race with depsgraph: this seems to happen with color strips mainly. To reproduce, add color strip and copy / paste it several times. . - Corruption: have array with color strips, save file while prefetching them.

Sergey Sharybin commented

2020-07-14 15:07:34 +02:00

Added subscriber: @Sergey

Sergey Sharybin commented

2020-07-14 15:07:34 +02:00

Modifications of strips should stop and re-initialize the pre-fetch job. This is because prefetch is sharing data from the main database, and can access/write to it from a no-main thread.

There are two possible ways to solve the problem:

Any operator and RNA property which affects on sequencer is to stop the prefetch job (similar to the material preview)
Or, the prefetch is to become fully autonomous (similar to what render pipeline does with DEG_graph_build_for_render_pipeline). It still would need to be re-started after modifications, but at least this could happen as a "listener" of the dependency graph.

Modifications of strips should stop and re-initialize the pre-fetch job. This is because prefetch is sharing data from the main database, and can access/write to it from a no-main thread. There are two possible ways to solve the problem: - Any operator and RNA property which affects on sequencer is to stop the prefetch job (similar to the material preview) - Or, the prefetch is to become fully autonomous (similar to what render pipeline does with `DEG_graph_build_for_render_pipeline`). It still would need to be re-started after modifications, but at least this could happen as a "listener" of the dependency graph.

Richard Antalik commented

2020-07-14 21:37:57 +02:00

@Sergey Prefetching is stopped when cache is invalidated, which should be every time some strip is changed. Also I am already using DEG_graph_build_for_render_pipeline, but maybe incorrectly?

Bigger issue is actual file corruption. I am not sure what is mechanism of that yet. I will trace it. Prefetch should not touch any data in bmain other than own structs.

@Sergey Prefetching is stopped when cache is invalidated, which should be every time some strip is changed. Also I am already using `DEG_graph_build_for_render_pipeline`, but maybe incorrectly? Bigger issue is actual file corruption. I am not sure what is mechanism of that yet. I will trace it. Prefetch should not touch any data in bmain other than own structs.

Richard Antalik changed title from ~~Scrolling in VSE crashes blender.~~ to Prefetching can corrupt .blend files

2020-07-14 22:40:52 +02:00

Richard Antalik commented

2020-07-14 22:59:44 +02:00

I thought I fixed this already. It seems that files get corrupted because of typo:

diff --git a/source/blender/blenkernel/intern/sequencer.c b/source/blender/blenkernel/intern/sequencer.c
index bde94f366b9..07caf84474b 100644
--- a/source/blender/blenkernel/intern/sequencer.c
+++ b/source/blender/blenkernel/intern/sequencer.c
@@ -5667,7 +5667,7 @@ static Sequence *seq_dupli(const Scene *scene_src,
     struct SeqEffectHandle sh;
     sh = BKE_sequence_get_effect(seq);
     if (sh.copy) {
-      sh.copy(seq, seqn, flag);
+      sh.copy(seqn, seq, flag);
     }

     seqn->strip->stripdata = NULL;

Second issue I described in comment seems to go away as well, so it only looked like thread race.

I thought I fixed this already. It seems that files get corrupted because of typo: ``` diff --git a/source/blender/blenkernel/intern/sequencer.c b/source/blender/blenkernel/intern/sequencer.c index bde94f366b9..07caf84474b 100644 --- a/source/blender/blenkernel/intern/sequencer.c +++ b/source/blender/blenkernel/intern/sequencer.c @@ -5667,7 +5667,7 @@ static Sequence *seq_dupli(const Scene *scene_src, struct SeqEffectHandle sh; sh = BKE_sequence_get_effect(seq); if (sh.copy) { - sh.copy(seq, seqn, flag); + sh.copy(seqn, seq, flag); } seqn->strip->stripdata = NULL; ``` Second issue I described in comment seems to go away as well, so it only looked like thread race.

Sergey Sharybin commented

2020-07-15 09:41:06 +02:00

Prefetching is stopped when cache is invalidated, which should be every time some strip is changed. Also I am already using DEG_graph_build_for_render_pipeline, but maybe incorrectly?

If you're using DEG_graph_build_for_render_pipeline, then the stopping prefetch on cache invalidation seems fine.

I do see some issues in the seqprefetch.c though. Summary goes as:

DEG_graph_build_for_render_pipeline is happen from main thread.
Initial depsgraph update is to happen from main thread as well.
Depsgraph is never to be freed and re-initialized during prefetch. If something what affects sequencer did change, it should re-initialize the prefetch job.
Use DEG_evaluate_on_framechange to update depsgraph for the new frame from the prefetch job.

The reason of this is because first evaluation of the depsgraph needs to run copy-on-write operations, which do access original datablocks. So if the copy-on-write happens from a thread, while someone is modifying strip you will run into threading conflicts.

Building depsgraph and evaluating it once from the main thread makes sure that no user interaction happens during copy-on-write is in progress.

> Prefetching is stopped when cache is invalidated, which should be every time some strip is changed. Also I am already using `DEG_graph_build_for_render_pipeline`, but maybe incorrectly? If you're using `DEG_graph_build_for_render_pipeline`, then the stopping prefetch on cache invalidation seems fine. I do see some issues in the `seqprefetch.c` though. Summary goes as: - `DEG_graph_build_for_render_pipeline` is happen from main thread. - Initial depsgraph update is to happen from main thread as well. - Depsgraph is never to be freed and re-initialized during prefetch. If something what affects sequencer did change, it should re-initialize the prefetch job. - Use `DEG_evaluate_on_framechange` to update depsgraph for the new frame from the prefetch job. The reason of this is because first evaluation of the depsgraph needs to run copy-on-write operations, which do access original datablocks. So if the copy-on-write happens from a thread, while someone is modifying strip you will run into threading conflicts. Building depsgraph and evaluating it once from the main thread makes sure that no user interaction happens during copy-on-write is in progress.

blender-admin commented

2020-07-16 02:24:37 +02:00

This issue was referenced by 3058bf0757

This issue was referenced by 3058bf075776de6a426391024aeb0d5257eb5898

blender-admin commented

2020-07-16 02:24:37 +02:00

This issue was referenced by 75a09ba2ed

This issue was referenced by 75a09ba2ed268fb1fec62e1ed41f2d689c28962a

Richard Antalik commented

2020-07-16 02:37:38 +02:00

Changed status from 'Confirmed' to: 'Resolved'

Richard Antalik closed this issue

2020-07-16 02:37:38 +02:00

Richard Antalik self-assigned this 2020-07-16 02:37:38 +02:00

Richard Antalik commented

2020-07-16 02:46:06 +02:00

I see issue with initializing depsgraph from thread. I will change to do this from main thread.

Thomas Dinges added this to the 2.90 milestone 2023-02-08 16:26:16 +01:00

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Prefetching can corrupt .blend files #78837