Crash accessing python gpu module in background mode. #59773

New Issue

Bastien Montagne · 2018-12-22T23:59:50+01:00

Bastien Montagne commented

2018-12-22 23:59:50 +01:00

Just run e.g. path/to/blender -b --python-expr "import bpy; bpy.ops.wm.addon_enable(module='space_view3d_math_vis')" (or even enable that add-on in userpref and try to start Blender 2.8 in background mode, I guess).

There are several level to that issue (and several ways to fix it, too, but that’s not my area of code so will live it to the experts ;) ):

math_vis should absolutely not do what it does in first lines of its draw.py, calling gpu.shader.from_builtin() at module level, i.e. during init… tsst. This can be solved in many ways (not even sure it's worth storing in py those shaders? afaik this is cached internally anyway?), as long as it does not do any call outside of functions. Lazy init ftw!
Think gpu module code (gpu_py_shader.c and friends) should ensure it has a valid OpenGL context blabla too? Unless add-ons are expected to do those checks themselves before using it?

Just run e.g. `path/to/blender -b --python-expr "import bpy; bpy.ops.wm.addon_enable(module='space_view3d_math_vis')"` (or even enable that add-on in userpref and try to start Blender 2.8 in background mode, I guess). There are several level to that issue (and several ways to fix it, too, but that’s not my area of code so will live it to the experts ;) ): * math_vis should absolutely **not** do what it does in first lines of its `draw.py`, calling `gpu.shader.from_builtin()` at module level, i.e. during init… tsst. This can be solved in many ways (not even sure it's worth storing in py those shaders? afaik this is cached internally anyway?), as long as it does not do any call outside of functions. Lazy init ftw! * Think gpu module code (`gpu_py_shader.c` and friends) should ensure it has a valid OpenGL context blabla too? Unless add-ons are expected to do those checks themselves before using it?

Campbell Barton was assigned by Bastien Montagne

2018-12-22 23:59:50 +01:00

Bastien Montagne commented

2018-12-22 23:59:50 +01:00

Added subscriber: @mont29

blender-admin commented

2018-12-22 23:59:50 +01:00

blender/blender#59929 was marked as duplicate of this issue

blender-admin commented

2018-12-22 23:59:50 +01:00

blender/blender#59787 was marked as duplicate of this issue

Bastien Montagne commented

2018-12-23 00:02:10 +01:00

Added subscriber: @fclem

Bastien Montagne commented

2018-12-23 00:02:10 +01:00

@ideasman42 assigned to you because math_vis is your add-on, and think you were involved in gpu api work? maybe @fclem or other #opengl_gfx guys are interested too…

math_vis thingy is currently blocking i18n update process, will have to work around that unless it’s fixed soonish. ;)

@ideasman42 assigned to you because math_vis is your add-on, and think you were involved in gpu api work? maybe @fclem or other #opengl_gfx guys are interested too… math_vis thingy is currently blocking i18n update process, will have to work around that unless it’s fixed soonish. ;)

Bastien Montagne commented

2018-12-23 12:49:51 +01:00

Added subscriber: @JacquesLucke

Bastien Montagne commented

2018-12-23 12:49:51 +01:00

@JacquesLucke object_scatter has exact same issue btw.

Bastien Montagne commented

2018-12-23 13:17:54 +01:00

Added subscriber: @AmirShehata

Germano Cavalcante commented

2018-12-24 23:06:44 +01:00

Added subscriber: @mano-wii

Germano Cavalcante commented

2018-12-24 23:06:44 +01:00

I think it would be best if the addons chek out these errors themselves.
But for this, instead of crash it would be better to raise an error message:
P877: patch for #59773

diff --git a/source/blender/gpu/GPU_init_exit.h b/source/blender/gpu/GPU_init_exit.h
index e89c970b7d9..8f1a42c8795 100644
--- a/source/blender/gpu/GPU_init_exit.h
+++ b/source/blender/gpu/GPU_init_exit.h
@@ -38,6 +38,7 @@ extern "C" {
 
 void GPU_init(void);
 void GPU_exit(void);
+bool GPU_is_initialized(void);
 
 #ifdef __cplusplus
 }
diff --git a/source/blender/gpu/intern/gpu_init_exit.c b/source/blender/gpu/intern/gpu_init_exit.c
index 55d0466c929..d21acb188d9 100644
--- a/source/blender/gpu/intern/gpu_init_exit.c
+++ b/source/blender/gpu/intern/gpu_init_exit.c
@@ -73,7 +73,6 @@ void GPU_init(void)
 }
 
 
-
 void GPU_exit(void)
 {
 	if (!G.background) {
@@ -92,3 +91,9 @@ void GPU_exit(void)
 
 	initialized = false;
 }
+
+
+bool GPU_is_initialized(void)
+{
+	return initialized;
+}
diff --git a/source/blender/python/gpu/gpu_py_shader.c b/source/blender/python/gpu/gpu_py_shader.c
index 5f1ea7a33ce..25ffd768a07 100644
--- a/source/blender/python/gpu/gpu_py_shader.c
+++ b/source/blender/python/gpu/gpu_py_shader.c
@@ -29,6 +29,7 @@
 
 #include "BLI_utildefines.h"
 
+#include "GPU_init_exit.h"
 #include "GPU_shader.h"
 #include "GPU_shader_interface.h"
 
@@ -721,6 +722,11 @@ static PyObject *bpygpu_shader_from_builtin(PyObject *UNUSED(self), PyObject *ar
 		return NULL;
 	}
 
+	if (!GPU_is_initialized()) {
+		PyErr_SetString(PyExc_SystemError, "This function can not be called without a GPU context");
+		return NULL;
+	}
+
 	GPUShader *shader = GPU_shader_get_builtin_shader(shader_id);
 
 	return BPyGPUShader_CreatePyObject(shader, true);
diff --git a/source/blender/python/gpu/gpu_py_types.c b/source/blender/python/gpu/gpu_py_types.c
index d9ef0736f8e..b767ee1b56c 100644
--- a/source/blender/python/gpu/gpu_py_types.c
+++ b/source/blender/python/gpu/gpu_py_types.c
@@ -27,6 +27,10 @@
 
 #include <Python.h>
 
+#include "BLI_utildefines.h"
+
+#include "GPU_init_exit.h"
+
 #include "../generic/py_capi_utils.h"
 #include "../generic/python_utildefines.h"
 
@@ -43,12 +47,31 @@ static struct PyModuleDef BPyGPU_types_module_def = {
 	.m_name = "gpu.types",
 };
 
+static PyObject *bpygpu_type_new_error(
+        PyTypeObject *UNUSED(type),
+        PyObject *UNUSED(args),
+        PyObject *UNUSED(kwds))
+{
+	PyErr_SetString(PyExc_SystemError, "This object can not be created without a GPU context");
+	return NULL;
+}
+
 PyObject *BPyInit_gpu_types(void)
 {
 	PyObject *submodule;
 
 	submodule = PyModule_Create(&BPyGPU_types_module_def);
 
+	if (!GPU_is_initialized()) {
+		/* Warning: Only restarting Blender for it to work again. */
+		BPyGPUVertFormat_Type.tp_new = bpygpu_type_new_error;
+		BPyGPUVertBuf_Type.tp_new    = bpygpu_type_new_error;
+		BPyGPUIndexBuf_Type.tp_new   = bpygpu_type_new_error;
+		BPyGPUBatch_Type.tp_new      = bpygpu_type_new_error;
+		BPyGPUOffScreen_Type.tp_new  = bpygpu_type_new_error;
+		BPyGPUShader_Type.tp_new     = bpygpu_type_new_error;
+	}
+
 	if (PyType_Ready(&BPyGPUVertFormat_Type) < 0)
 		return NULL;
 	if (PyType_Ready(&BPyGPUVertBuf_Type) < 0)

I think it would be best if the addons chek out these errors themselves. But for this, instead of crash it would be better to raise an error message: [P877: patch for #59773](https://archive.blender.org/developer/P877.txt) ```diff diff --git a/source/blender/gpu/GPU_init_exit.h b/source/blender/gpu/GPU_init_exit.h index e89c970b7d9..8f1a42c8795 100644 --- a/source/blender/gpu/GPU_init_exit.h +++ b/source/blender/gpu/GPU_init_exit.h @@ -38,6 +38,7 @@ extern "C" { void GPU_init(void); void GPU_exit(void); +bool GPU_is_initialized(void); #ifdef __cplusplus } diff --git a/source/blender/gpu/intern/gpu_init_exit.c b/source/blender/gpu/intern/gpu_init_exit.c index 55d0466c929..d21acb188d9 100644 --- a/source/blender/gpu/intern/gpu_init_exit.c +++ b/source/blender/gpu/intern/gpu_init_exit.c @@ -73,7 +73,6 @@ void GPU_init(void) } - void GPU_exit(void) { if (!G.background) { @@ -92,3 +91,9 @@ void GPU_exit(void) initialized = false; } + + +bool GPU_is_initialized(void) +{ + return initialized; +} diff --git a/source/blender/python/gpu/gpu_py_shader.c b/source/blender/python/gpu/gpu_py_shader.c index 5f1ea7a33ce..25ffd768a07 100644 --- a/source/blender/python/gpu/gpu_py_shader.c +++ b/source/blender/python/gpu/gpu_py_shader.c @@ -29,6 +29,7 @@ #include "BLI_utildefines.h" +#include "GPU_init_exit.h" #include "GPU_shader.h" #include "GPU_shader_interface.h" @@ -721,6 +722,11 @@ static PyObject *bpygpu_shader_from_builtin(PyObject *UNUSED(self), PyObject *ar return NULL; } + if (!GPU_is_initialized()) { + PyErr_SetString(PyExc_SystemError, "This function can not be called without a GPU context"); + return NULL; + } + GPUShader *shader = GPU_shader_get_builtin_shader(shader_id); return BPyGPUShader_CreatePyObject(shader, true); diff --git a/source/blender/python/gpu/gpu_py_types.c b/source/blender/python/gpu/gpu_py_types.c index d9ef0736f8e..b767ee1b56c 100644 --- a/source/blender/python/gpu/gpu_py_types.c +++ b/source/blender/python/gpu/gpu_py_types.c @@ -27,6 +27,10 @@ #include <Python.h> +#include "BLI_utildefines.h" + +#include "GPU_init_exit.h" + #include "../generic/py_capi_utils.h" #include "../generic/python_utildefines.h" @@ -43,12 +47,31 @@ static struct PyModuleDef BPyGPU_types_module_def = { .m_name = "gpu.types", }; +static PyObject *bpygpu_type_new_error( + PyTypeObject *UNUSED(type), + PyObject *UNUSED(args), + PyObject *UNUSED(kwds)) +{ + PyErr_SetString(PyExc_SystemError, "This object can not be created without a GPU context"); + return NULL; +} + PyObject *BPyInit_gpu_types(void) { PyObject *submodule; submodule = PyModule_Create(&BPyGPU_types_module_def); + if (!GPU_is_initialized()) { + /* Warning: Only restarting Blender for it to work again. */ + BPyGPUVertFormat_Type.tp_new = bpygpu_type_new_error; + BPyGPUVertBuf_Type.tp_new = bpygpu_type_new_error; + BPyGPUIndexBuf_Type.tp_new = bpygpu_type_new_error; + BPyGPUBatch_Type.tp_new = bpygpu_type_new_error; + BPyGPUOffScreen_Type.tp_new = bpygpu_type_new_error; + BPyGPUShader_Type.tp_new = bpygpu_type_new_error; + } + if (PyType_Ready(&BPyGPUVertFormat_Type) < 0) return NULL; if (PyType_Ready(&BPyGPUVertBuf_Type) < 0) ```

Amir Shehata commented

2018-12-24 23:36:05 +01:00

animation nodes addon also triggers this crash
Also tested the patch and it works.

animation nodes addon also triggers this crash Also tested the patch and it works.

Bastien Montagne commented

2018-12-25 11:26:02 +01:00

IMHO, first thing first, addons should not call those gpu functions at module level, that means they get called during first eval of the code (first import). This is bad practice in general, unless absolutely mandatory we avoid that kind of extra processing since it makes Blender loading slower.

In fact, not even sure why those global storage of gpu.shader.from_builtin()results are needed at all, afaict those are already cached internally, so call itself should be cheap after first time? But if they are needed, then a lazy init solution should be used instead (like using a dummy class with accessors, or so).

IMHO, first thing first, addons should not call those gpu functions at module level, that means they get called during first eval of the code (first import). This is bad practice in general, unless absolutely mandatory we avoid that kind of extra processing since it makes Blender loading slower. In fact, not even sure why those global storage of `gpu.shader.from_builtin()`results are needed at all, afaict those are already cached internally, so call itself should be cheap after first time? But if they are needed, then a lazy init solution should be used instead (like using a dummy class with accessors, or so).

Jacques Lucke commented

2018-12-25 12:29:30 +01:00

I can fix the Object Scatter and Math Vis addon in that regard (maybe also the UV Export addon).
However, it might take me a couple of days because I'm on vacation and have to work on my Bachelor thesis..
Feel free to assign the addon updates to me.

Will those addons never work in background mode, or is it just the wrong time to initialize? (I'm especially talking about the png export of the Export UV addon, which uses opengl to render the image)

I can fix the Object Scatter and Math Vis addon in that regard (maybe also the UV Export addon). However, it might take me a couple of days because I'm on vacation and have to work on my Bachelor thesis.. Feel free to assign the addon updates to me. Will those addons never work in background mode, or is it just the wrong time to initialize? (I'm especially talking about the png export of the Export UV addon, which uses opengl to render the image)

Germano Cavalcante commented

2018-12-25 15:41:35 +01:00

In #59773#588075, @mont29 wrote:
IMHO, first thing first, addons should not call those gpu functions at module level (...)

Do you recommend adding an assert or raise error if the gpu module is imported at module level?

> In #59773#588075, @mont29 wrote: > IMHO, first thing first, addons should not call those gpu functions at module level (...) Do you recommend adding an assert or raise error if the gpu module is imported at module level?

Bastien Montagne commented

2018-12-26 22:54:31 +01:00

@JacquesLucke Think most of the time they won't work in background mode, no, which would solve most of the crashes already. For cases where OGL context is used in background mode, indeed that patch sounds OK to me, and it's add-on responsibility to handle the issue properly. And no rush here, it can wait for you to get back from holidays! ;)

@mano-wii importing gpu module is fine, what is not fine is calling some of its functions at global level.

@JacquesLucke Think most of the time they won't work in background mode, no, which would solve most of the crashes already. For cases where OGL context is used in background mode, indeed that patch sounds OK to me, and it's add-on responsibility to handle the issue properly. And no rush here, it can wait for you to get back from holidays! ;) @mano-wii *importing* gpu module is fine, what is not fine is calling some of its functions at global level.

Bastien Montagne commented

2018-12-29 15:30:32 +01:00

Added subscribers: @MaTcHo0o, @brecht