blender/blender

Fix #110751: Clear all strokes prior to baking lineart #119381

Closed

YimingWu wants to merge 1 commits from ChengduLittleA/blender:fix-110751 into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.

YimingWu commented

2024-03-12 15:08:43 +01:00

Member

This is a attempt to fix line art crashing from baking while there are
already strokes in the gpencil object that aren't generated by lineart.

Side effect: If you bake the second time in a different frame range, the
previous bake is going to be cleared.

This crash is probably related to the change to using job system
(instead of a simple loop), and during
BKE_scene_graph_update_for_newframe() call, blender crashes trying to
duplicate some existing strokes.

This is a attempt to fix line art crashing from baking while there are already strokes in the gpencil object that aren't generated by lineart. Side effect: If you bake the second time in a different frame range, the previous bake is going to be cleared. This crash is probably related to the change to using job system (instead of a simple loop), and during `BKE_scene_graph_update_for_newframe()` call, blender crashes trying to duplicate some existing strokes.

YimingWu added the

labels 2024-03-12 15:08:43 +01:00

YimingWu added 1 commit 2024-03-12 15:08:55 +01:00

Fix #110751 : Clear all strokes prior to baking lineart 5cbc815fb8

This is a attempt to fix line art crashing from baking while there are
already strokes in the gpencil object that's not generated by lineart.

Side effect: If you bake the second time in a different frame range, the
previous bake is going to be cleared.

This crash is probably related to the change to using job system
(instead of a simple loop), and during
`BKE_scene_graph_update_for_newframe()` call, blender crashes trying to
duplicate some existing strokes.

Falk David requested review from Hans Goudey 2024-03-14 10:57:14 +01:00

Hans Goudey reviewed 2024-03-14 14:43:06 +01:00

Hans Goudey left a comment

Member

This seems fine I guess, but I'm not really sure why I'm the reviewer here TBH. I have to say I don't find the PR description that convincing though-- it would be nice if we had more confidence about why there is a crash in the first place.

This seems fine I guess, but I'm not really sure why I'm the reviewer here TBH. I have to say I don't find the PR description that convincing though-- it would be nice if we had more confidence about why there is a crash in the first place.

YimingWu commented

2024-03-14 15:54:59 +01:00

Author

Member

TBH I'm not really sure either. All I know is that if there are existing strokes prior to baking line art, it will definitely crash. However start baking from empty gp object it won't, even if it will bake new frames of strokes by itself. My guess is that it involves the change to job system (as opposed to blocking) and the UI needs refreshing, which caused conflict since it crashed in depsgraph evaluation while copying strokes from original data block.

TBH I'm not really sure either. All I know is that if there are existing strokes prior to baking line art, it will definitely crash. However start baking from empty gp object it won't, even if it will bake new frames of strokes by itself. My guess is that it involves the change to job system (as opposed to blocking) and the UI needs refreshing, which caused conflict since it crashed in depsgraph evaluation while copying strokes from original data block.

Brecht Van Lommel commented

2024-03-18 18:13:43 +01:00

Owner

I think it's important to understand things, we should only guess if we really can't figure it out.

Running ASAN shows this:

=================================================================
==1281396==ERROR: AddressSanitizer: heap-use-after-free on address 0x6060000c5d98 at pc 0x000000dc7781 bp 0x7fff79d237c0 sp 0x7fff79d237b8
READ of size 8 at 0x6060000c5d98 thread T190
    #0 0xdc7780 in BKE_gpencil_frame_copy_strokes /home/brecht/dev/worktree/source/blender/blenkernel/intern/gpencil_legacy.cc:911:3
    #1 0xe028f2 in copy_frame_to_eval_ex(bGPDframe*, bGPDframe*) /home/brecht/dev/worktree/source/blender/blenkernel/intern/gpencil_modifier_legacy.cc:688:3
    #2 0xdfc511 in gpencil_copy_visible_frames_to_eval(Depsgraph*, Scene*, Object*) /home/brecht/dev/worktree/source/blender/blenkernel/intern/gpencil_modifier_legacy.cc:722:7
    #3 0xdfad88 in BKE_gpencil_prepare_eval_data /home/brecht/dev/worktree/source/blender/blenkernel/intern/gpencil_modifier_legacy.cc:795:3
    #4 0x19dd0ae in BKE_object_handle_data_update(Depsgraph*, Scene*, Object*) /home/brecht/dev/worktree/source/blender/blenkernel/intern/object_update.cc:183:7
    #5 0x19dfaac in BKE_object_eval_uber_data(Depsgraph*, Scene*, Object*) /home/brecht/dev/worktree/source/blender/blenkernel/intern/object_update.cc:315:3
    #6 0x59283f2 in blender::deg::DepsgraphNodeBuilder::build_object_data_geometry(Object*)::$_24::operator()(Depsgraph*) const /home/brecht/dev/worktree/source/blender/depsgraph/intern/builder/deg_builder_nodes.cc:1681:34

0x6060000c5d98 is located 24 bytes inside of 64-byte region [0x6060000c5d80,0x6060000c5dc0)
freed by thread T197 here:
    #0 0xa380fd in free (/home/brecht/dev/worktree_debug/bin/blender+0xa380fd)
    #1 0x59a890c in MEM_lockfree_freeN /home/brecht/dev/worktree/intern/guardedalloc/intern/mallocn_lockfree_impl.c:110:5
    #2 0x51a9c72 in BLI_freelinkN /home/brecht/dev/worktree/source/blender/blenlib/intern/listbase.cc:278:3
    #3 0xdcf505 in BKE_gpencil_layer_frame_delete /home/brecht/dev/worktree/source/blender/blenkernel/intern/gpencil_legacy.cc:1392:3
    #4 0x112183a5 in clear_strokes(Object*, GpencilModifierData*, int) /home/brecht/dev/worktree/source/blender/gpencil_modifiers_legacy/intern/lineart/lineart_ops.cc:74:3
    #5 0x11217b9f in lineart_gpencil_bake_single_target(LineartBakeJob*, Object*, int) /home/brecht/dev/worktree/source/blender/gpencil_modifiers_legacy/intern/lineart/lineart_ops.cc:186:9
    #6 0x11216230 in lineart_gpencil_bake_startjob(void*, wmJobWorkerStatus*) /home/brecht/dev/worktree/source/blender/gpencil_modifiers_legacy/intern/lineart/lineart_ops.cc:245:11
    #7 0x5ac6225 in do_job_thread(void*) /home/brecht/dev/worktree/source/blender/windowmanager/intern/wm_jobs.cc:398:3
    #8 0x56c43eb in tslot_thread_start(void*) /home/brecht/dev/worktree/source/blender/blenlib/intern/threads.cc:176:10
    #9 0x7ffff7bd7608 in start_thread /build/glibc-wuryBv/glibc-2.31/nptl/pthread_create.c:477:8

What this indicates is that the depsgraph is accessing strokes that were freed in the baking code. One of the potential causes is that the depsgraph was not tagged for these changes, and did not copy the original to the evaluated object before evaluation. And indeed it seems that clear_strokes does not directly cause a depsgraph tag.

This fix seems to work from a quick test:

diff --git a/source/blender/gpencil_modifiers_legacy/intern/lineart/lineart_ops.cc b/source/blender/gpencil_modifiers_legacy/intern/lineart/lineart_ops.cc
index 5752a085a17..33f6249607e 100644
--- a/source/blender/gpencil_modifiers_legacy/intern/lineart/lineart_ops.cc
+++ b/source/blender/gpencil_modifiers_legacy/intern/lineart/lineart_ops.cc
@@ -52,26 +52,27 @@ static bool lineart_mod_is_disabled(GpencilModifierData *md)
   return disabled;
 }
 
-static void clear_strokes(Object *ob, GpencilModifierData *md, int frame)
+static bool clear_strokes(Object *ob, GpencilModifierData *md, int frame)
 {
   if (md->type != eGpencilModifierType_Lineart) {
-    return;
+    return false;
   }
   LineartGpencilModifierData *lmd = (LineartGpencilModifierData *)md;
   bGPdata *gpd = static_cast<bGPdata *>(ob->data);
 
   bGPDlayer *gpl = BKE_gpencil_layer_get_by_name(gpd, lmd->target_layer, 1);
   if (!gpl) {
-    return;
+    return false;
   }
   bGPDframe *gpf = BKE_gpencil_layer_frame_find(gpl, frame);
 
   if (!gpf) {
     /* No greasepencil frame found. */
-    return;
+    return false;
   }
 
   BKE_gpencil_layer_frame_delete(gpl, gpf);
+  return true;
 }
 
 static bool bake_strokes(Object *ob,
@@ -183,7 +184,9 @@ static bool lineart_gpencil_bake_single_target(LineartBakeJob *bj, Object *ob, i
   if (bj->overwrite_frames) {
     LISTBASE_FOREACH (GpencilModifierData *, md, &ob->greasepencil_modifiers) {
       if (md->type == eGpencilModifierType_Lineart) {
-        clear_strokes(ob, md, frame);
+        if (clear_strokes(ob, md, frame)) {
+          touched = true;
+        }
       }
     }
   }

I think it's important to understand things, we should only guess if we really can't figure it out. Running ASAN shows this: ``` ================================================================= ==1281396==ERROR: AddressSanitizer: heap-use-after-free on address 0x6060000c5d98 at pc 0x000000dc7781 bp 0x7fff79d237c0 sp 0x7fff79d237b8 READ of size 8 at 0x6060000c5d98 thread T190 #0 0xdc7780 in BKE_gpencil_frame_copy_strokes /home/brecht/dev/worktree/source/blender/blenkernel/intern/gpencil_legacy.cc:911:3 #1 0xe028f2 in copy_frame_to_eval_ex(bGPDframe*, bGPDframe*) /home/brecht/dev/worktree/source/blender/blenkernel/intern/gpencil_modifier_legacy.cc:688:3 #2 0xdfc511 in gpencil_copy_visible_frames_to_eval(Depsgraph*, Scene*, Object*) /home/brecht/dev/worktree/source/blender/blenkernel/intern/gpencil_modifier_legacy.cc:722:7 #3 0xdfad88 in BKE_gpencil_prepare_eval_data /home/brecht/dev/worktree/source/blender/blenkernel/intern/gpencil_modifier_legacy.cc:795:3 #4 0x19dd0ae in BKE_object_handle_data_update(Depsgraph*, Scene*, Object*) /home/brecht/dev/worktree/source/blender/blenkernel/intern/object_update.cc:183:7 #5 0x19dfaac in BKE_object_eval_uber_data(Depsgraph*, Scene*, Object*) /home/brecht/dev/worktree/source/blender/blenkernel/intern/object_update.cc:315:3 #6 0x59283f2 in blender::deg::DepsgraphNodeBuilder::build_object_data_geometry(Object*)::$_24::operator()(Depsgraph*) const /home/brecht/dev/worktree/source/blender/depsgraph/intern/builder/deg_builder_nodes.cc:1681:34 0x6060000c5d98 is located 24 bytes inside of 64-byte region [0x6060000c5d80,0x6060000c5dc0) freed by thread T197 here: #0 0xa380fd in free (/home/brecht/dev/worktree_debug/bin/blender+0xa380fd) #1 0x59a890c in MEM_lockfree_freeN /home/brecht/dev/worktree/intern/guardedalloc/intern/mallocn_lockfree_impl.c:110:5 #2 0x51a9c72 in BLI_freelinkN /home/brecht/dev/worktree/source/blender/blenlib/intern/listbase.cc:278:3 #3 0xdcf505 in BKE_gpencil_layer_frame_delete /home/brecht/dev/worktree/source/blender/blenkernel/intern/gpencil_legacy.cc:1392:3 #4 0x112183a5 in clear_strokes(Object*, GpencilModifierData*, int) /home/brecht/dev/worktree/source/blender/gpencil_modifiers_legacy/intern/lineart/lineart_ops.cc:74:3 #5 0x11217b9f in lineart_gpencil_bake_single_target(LineartBakeJob*, Object*, int) /home/brecht/dev/worktree/source/blender/gpencil_modifiers_legacy/intern/lineart/lineart_ops.cc:186:9 #6 0x11216230 in lineart_gpencil_bake_startjob(void*, wmJobWorkerStatus*) /home/brecht/dev/worktree/source/blender/gpencil_modifiers_legacy/intern/lineart/lineart_ops.cc:245:11 #7 0x5ac6225 in do_job_thread(void*) /home/brecht/dev/worktree/source/blender/windowmanager/intern/wm_jobs.cc:398:3 #8 0x56c43eb in tslot_thread_start(void*) /home/brecht/dev/worktree/source/blender/blenlib/intern/threads.cc:176:10 #9 0x7ffff7bd7608 in start_thread /build/glibc-wuryBv/glibc-2.31/nptl/pthread_create.c:477:8 ``` What this indicates is that the depsgraph is accessing strokes that were freed in the baking code. One of the potential causes is that the depsgraph was not tagged for these changes, and did not copy the original to the evaluated object before evaluation. And indeed it seems that `clear_strokes` does not directly cause a depsgraph tag. This fix seems to work from a quick test: ```Diff diff --git a/source/blender/gpencil_modifiers_legacy/intern/lineart/lineart_ops.cc b/source/blender/gpencil_modifiers_legacy/intern/lineart/lineart_ops.cc index 5752a085a17..33f6249607e 100644 --- a/source/blender/gpencil_modifiers_legacy/intern/lineart/lineart_ops.cc +++ b/source/blender/gpencil_modifiers_legacy/intern/lineart/lineart_ops.cc @@ -52,26 +52,27 @@ static bool lineart_mod_is_disabled(GpencilModifierData *md) return disabled; } -static void clear_strokes(Object *ob, GpencilModifierData *md, int frame) +static bool clear_strokes(Object *ob, GpencilModifierData *md, int frame) { if (md->type != eGpencilModifierType_Lineart) { - return; + return false; } LineartGpencilModifierData *lmd = (LineartGpencilModifierData *)md; bGPdata *gpd = static_cast<bGPdata *>(ob->data); bGPDlayer *gpl = BKE_gpencil_layer_get_by_name(gpd, lmd->target_layer, 1); if (!gpl) { - return; + return false; } bGPDframe *gpf = BKE_gpencil_layer_frame_find(gpl, frame); if (!gpf) { /* No greasepencil frame found. */ - return; + return false; } BKE_gpencil_layer_frame_delete(gpl, gpf); + return true; } static bool bake_strokes(Object *ob, @@ -183,7 +184,9 @@ static bool lineart_gpencil_bake_single_target(LineartBakeJob *bj, Object *ob, i if (bj->overwrite_frames) { LISTBASE_FOREACH (GpencilModifierData *, md, &ob->greasepencil_modifiers) { if (md->type == eGpencilModifierType_Lineart) { - clear_strokes(ob, md, frame); + if (clear_strokes(ob, md, frame)) { + touched = true; + } } } } ```

Brecht Van Lommel commented

2024-03-19 15:27:58 +01:00

Owner

Posted my fix in #119662.

Posted my fix in #119662.

Brecht Van Lommel commented

2024-03-19 18:00:43 +01:00

Owner

Closing assuming this fix is no longer needed, but feel free to reopen if you disagree. Just for 4.1 the simpler fix seems best.

Closing assuming this fix is no longer needed, but feel free to reopen if you disagree. Just for 4.1 the simpler fix seems best.

Brecht Van Lommel closed this pull request

2024-03-19 18:00:46 +01:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Automated Testing

Blender Asset Bundle

Code Documentation

Dependency Graph

Development Management

Images & Movies

Motion Tracking

Nodes & Physics

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Viewport & EEVEE

Virtual Reality

Asset Browser Project

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

Nodes & Physics

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Viewport & EEVEE

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

3 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#119381

No description provided.