Fix #129019: Prevent render engine changes while rendering #129031

YimingWu · 2024-10-15T06:12:12+02:00

YimingWu commented

2024-10-15 06:12:12 +02:00

While animation is being rendered and interface not locked, user could
accidentally change render engine setting which won't make any practical
sense, but will result in a crash. This fix will prevent render engine
changes if blender is rendering stuff.

While animation is being rendered and interface not locked, user could accidentally change render engine setting which won't make any practical sense, but will result in a crash. This fix will prevent render engine changes if blender is rendering stuff.

❤️ 1

YimingWu added the

Module

Render & Cycles

label 2024-10-15 06:12:12 +02:00

YimingWu added 1 commit 2024-10-15 06:12:23 +02:00

Fix #129019 : Prevent render engine changes while rendering dc274f9f07

While animation is being rendered and interface not locked, user could
accidentally change render engine setting which won't make any practical
sense, but will result in a crash. This fix will prevent render engine
changes if blender is rendering stuff.

Miguel Pozo commented

2024-10-30 13:03:03 +01:00

@ChengduLittleA I would add a BKE_report to inform the user they can't switch while rendering.

@fclem Can you take a look at this?

@ChengduLittleA I would add a `BKE_report` to inform the user they can't switch while rendering. @fclem Can you take a look at this?

👍 1

Clément Foucault commented

2024-10-30 14:22:57 +01:00

+1 for the BKE_report.

@Sergey tagging you to double check the logic.

Looks fine to me but I am not sure if this covers all cases (python updates during render?).

Also adding a comment as to why we early return would be nice.

+1 for the `BKE_report`. @Sergey tagging you to double check the logic. Looks fine to me but I am not sure if this covers all cases (python updates during render?). Also adding a comment as to why we early return would be nice.

Sergey Sharybin commented

2024-10-30 17:24:40 +01:00

The question is why crash happens?
The render pipeline is supposed to be decoupled from the original scene after hitting F12. Changes in render settings, or output settings, or passes, or compositor nodes should not be resulting in crash.

The question is *why* crash happens? The render pipeline is supposed to be decoupled from the original scene after hitting F12. Changes in render settings, or output settings, or passes, or compositor nodes should not be resulting in crash.

YimingWu commented

2024-11-02 08:36:29 +01:00

Looks fine to me but I am not sure if this covers all cases (python updates during render?).

Since python goes through RNA so it is also handled.

The question is why crash happens?

Honestly I have no idea. In DRW_system_gpu_render_context_enable() I will get the assert saying you should use DRW_gpu_context_enable() instead, but there's another context re_system_gpu_context needs to be bound/enabled, and DRW_gpu_context_enable() does not take that argument (it uses system_gpu_context instead). Any good ideas? 🤔

I will add a report thingy there.

> Looks fine to me but I am not sure if this covers all cases (python updates during render?). Since python goes through RNA so it is also handled. > The question is why crash happens? Honestly I have no idea. In `DRW_system_gpu_render_context_enable()` I will get the assert saying you should use `DRW_gpu_context_enable()` instead, but there's another context `re_system_gpu_context` needs to be bound/enabled, and `DRW_gpu_context_enable()` does not take that argument (it uses `system_gpu_context` instead). Any good ideas? 🤔 I will add a `report` thingy there.

YimingWu added 2 commits 2024-11-02 08:39:56 +01:00

Merge branch 'main' into fix-129019 e1d159f95b

Add a report 449afbc50f

YimingWu added 1 commit 2024-11-02 08:46:02 +01:00

missing a line b98e489fbe

Clément Foucault commented

2024-11-07 12:34:17 +01:00

Ok looking deeper into this one. The issue is that the main thread is (apparently) destroying the current RenderEngine (calling RE_engine_free then engine_depsgraph_free) which has a gpu context associated with it.

The issue is that this gpu context should not be used on the main thread since it was created for rendering on a different thread.

So I am not sure what to do in this case. It might be enough to bind the main DRW context if the thread is main as the resources that needs to be deleted are shared. This diff fixes the crash. But I get a black viewport for one frame from time to time which is concerning.

diff --git a/source/blender/render/intern/engine.cc b/source/blender/render/intern/engine.cc
index 79081de3cb6..0f36faa971e 100644
--- a/source/blender/render/intern/engine.cc
+++ b/source/blender/render/intern/engine.cc
@@ -137,14 +137,24 @@ static void engine_depsgraph_free(RenderEngine *engine)
     /* Need GPU context since this might free GPU buffers. */
     const bool use_gpu_context = (engine->type->flag & RE_USE_GPU_CONTEXT);
     if (use_gpu_context) {
-      DRW_render_context_enable(engine->re);
+      if (BLI_thread_is_main()) {
+        DRW_gpu_context_enable();
+      }
+      else {
+        DRW_render_context_enable(engine->re);
+      }
     }
 
     DEG_graph_free(engine->depsgraph);
     engine->depsgraph = nullptr;
 
     if (use_gpu_context) {
-      DRW_render_context_disable(engine->re);
+      if (BLI_thread_is_main()) {
+        DRW_gpu_context_disable();
+      }
+      else {
+        DRW_render_context_disable(engine->re);
+      }
     }
   }
 }

But this pauses some concerns:

This can recursive lock if called at the wrong level.
This can mess up the drawing context state.
Why do we even need to free the current renderer depsgraph from the main thread?

BTW I could reproduce with 3.0 so this bug exists from a very long time. So I don't think it should be high prio.

Ok looking deeper into this one. The issue is that the main thread is (apparently) destroying the current `RenderEngine` (calling `RE_engine_free` then `engine_depsgraph_free`) which has a gpu context associated with it. The issue is that this gpu context should not be used on the main thread since it was created for rendering on a different thread. So I am not sure what to do in this case. It might be enough to bind the main DRW context if the thread is main as the resources that needs to be deleted are shared. This diff fixes the crash. But I get a black viewport for one frame from time to time which is concerning. ```diff diff --git a/source/blender/render/intern/engine.cc b/source/blender/render/intern/engine.cc index 79081de3cb6..0f36faa971e 100644 --- a/source/blender/render/intern/engine.cc +++ b/source/blender/render/intern/engine.cc @@ -137,14 +137,24 @@ static void engine_depsgraph_free(RenderEngine *engine) /* Need GPU context since this might free GPU buffers. */ const bool use_gpu_context = (engine->type->flag & RE_USE_GPU_CONTEXT); if (use_gpu_context) { - DRW_render_context_enable(engine->re); + if (BLI_thread_is_main()) { + DRW_gpu_context_enable(); + } + else { + DRW_render_context_enable(engine->re); + } } DEG_graph_free(engine->depsgraph); engine->depsgraph = nullptr; if (use_gpu_context) { - DRW_render_context_disable(engine->re); + if (BLI_thread_is_main()) { + DRW_gpu_context_disable(); + } + else { + DRW_render_context_disable(engine->re); + } } } } ``` But this pauses some concerns: - This can recursive lock if called at the wrong level. - This can mess up the drawing context state. - Why do we even need to free the current renderer depsgraph from the main thread? BTW I could reproduce with 3.0 so this bug exists from a very long time. So I don't think it should be high prio.

👀 1

YimingWu referenced this pull request

2024-11-07 12:54:24 +01:00

Crash when switching from EEVEE to Cycles while rendering animation #129019

Clément Foucault commented

2024-11-11 12:01:02 +01:00

Commited fix as dafa3fb88f

Commited fix as dafa3fb88fb2aad6f1e690e0a8857f17d0fa4ebb

Clément Foucault closed this pull request

2024-11-11 12:01:05 +01:00