[Bf-blender-cvs] [2a70c8e] opensubdiv-modifier: Fix accidental crashes when changing subdiv level

Sergey Sharybin noreply at git.blender.org
Mon May 12 20:09:04 CEST 2014


Commit: 2a70c8ebe0b9ff53107875ce5b2b0c3719f81396
Author: Sergey Sharybin
Date:   Sun May 11 19:27:54 2014 +0200
https://developer.blender.org/rB2a70c8ebe0b9ff53107875ce5b2b0c3719f81396

Fix accidental crashes when changing subdiv level

Issue was caused by freeing GLMesh from non-main thread.
Since this might invoke OpenGL calls, better to free the
mesh from main thread instead.

===================================================================

M	intern/opensubdiv/opensubdiv_capi.cc
M	intern/opensubdiv/opensubdiv_capi.h
M	intern/opensubdiv/opensubdiv_gpu_capi.cc
M	source/blender/blenkernel/intern/CCGSubSurf.c

===================================================================

diff --git a/intern/opensubdiv/opensubdiv_capi.cc b/intern/opensubdiv/opensubdiv_capi.cc
index 3e7787c..1ba85bd 100644
--- a/intern/opensubdiv/opensubdiv_capi.cc
+++ b/intern/opensubdiv/opensubdiv_capi.cc
@@ -69,7 +69,7 @@
 
 #include "MEM_guardedalloc.h"
 
-/* **************** Types declaration **************** */
+// **************** Types declaration ****************
 
 struct OpenSubdiv_ComputeControllerDescr;
 
@@ -258,8 +258,7 @@ struct OpenSubdiv_GLMesh *openSubdiv_createOsdGLMeshFromEvaluator(
 	int num_vertex_elements = 3;
 	int num_varying_elements = 0;
 
-	/* Trick to avoid passing multi-argument template to a macro. */
-	OsdGLMeshInterface *mesh;
+	OsdGLMeshInterface *mesh = NULL;
 
 	switch (controller_type) {
 #define CHECK_CONTROLLER_TYPE(type, class, controller_class) \
@@ -313,10 +312,15 @@ struct OpenSubdiv_GLMesh *openSubdiv_createOsdGLMeshFromEvaluator(
 #undef CHECK_CONTROLLER_TYPE
 	}
 
+	if (mesh == NULL) {
+		return NULL;
+	}
+
 	OpenSubdiv_GLMesh *gl_mesh =
 		(OpenSubdiv_GLMesh *) OBJECT_GUARDED_NEW(OpenSubdiv_GLMesh);
 	gl_mesh->controller_type = controller_type;
 	gl_mesh->descriptor = (OpenSubdiv_GLMeshDescr *) mesh;
+	gl_mesh->level = level;
 
 	return gl_mesh;
 }
diff --git a/intern/opensubdiv/opensubdiv_capi.h b/intern/opensubdiv/opensubdiv_capi.h
index e5a68b7..3491273 100644
--- a/intern/opensubdiv/opensubdiv_capi.h
+++ b/intern/opensubdiv/opensubdiv_capi.h
@@ -39,6 +39,7 @@ struct OpenSubdiv_GLMeshDescr;
 typedef struct OpenSubdiv_GLMesh {
 	int controller_type;
 	OpenSubdiv_GLMeshDescr *descriptor;
+	int level;
 } OpenSubdiv_GLMesh;
 #endif
 
@@ -78,4 +79,4 @@ void openSubdiv_cleanup(void);
 }
 #endif
 
-#endif  /* __OPENSUBDIV_CAPI_H__ */
+#endif  // __OPENSUBDIV_CAPI_H__
diff --git a/intern/opensubdiv/opensubdiv_gpu_capi.cc b/intern/opensubdiv/opensubdiv_gpu_capi.cc
index 0df6614..680944e 100644
--- a/intern/opensubdiv/opensubdiv_gpu_capi.cc
+++ b/intern/opensubdiv/opensubdiv_gpu_capi.cc
@@ -39,7 +39,7 @@
 #include <opensubdiv/osd/cudaGLVertexBuffer.h>
 #include <opensubdiv/osd/glDrawRegistry.h>
 
-/* **************** Types declaration **************** */
+// **************** Types declaration ****************
 
 using OpenSubdiv::OsdDrawContext;
 using OpenSubdiv::OsdGLDrawRegistry;
@@ -54,9 +54,8 @@ protected:
 	virtual SourceConfigType *_CreateDrawSourceConfig(DescType const &desc);
 };
 
-/* TODO(sergey): Fine for tests, but ideally need to be stored
- * in some sort of object draw context.
- */
+// TODO(sergey): Fine for tests, but ideally need to be stored
+// in some sort of object draw context.
 static GLuint g_transformUB = 0,
               g_transformBinding = 0,
               g_tessellationUB = 0,
@@ -203,7 +202,7 @@ EffectDrawRegistry::_CreateDrawSourceConfig(DescType const &desc)
 		sconfig->fragmentShader.AddDefine("PRIM_TRI");
     }
 
-	/* TODO(sergey): Currently unsupported, but good to have for the reference. */
+	// TODO(sergey): Currently unsupported, but good to have for the reference.
 #if 0
 	if (screenSpaceTess) {
 		sconfig->commonShader.AddDefine("OSD_ENABLE_SCREENSPACE_TESSELLATION");
@@ -271,7 +270,8 @@ EffectDrawRegistry::_CreateDrawConfig(DescType const &desc,
 }
 
 static GLuint bindProgram(OsdGLMeshInterface *mesh,
-                          OsdDrawContext::PatchArray const &patch)
+                          OsdDrawContext::PatchArray const &patch,
+                          int level)
 {
 	EffectDrawRegistry::ConfigType *config =
 		g_effectRegistry.GetDrawConfig(patch.GetDescriptor());
@@ -298,7 +298,7 @@ static GLuint bindProgram(OsdGLMeshInterface *mesh,
 		float TessLevel;
 	} tessellationData;
 
-	tessellationData.TessLevel = static_cast<float>(1 << /*g_tessLevel*/1);
+	tessellationData.TessLevel = static_cast<float>(1 << level);
 
 	if (! g_tessellationUB) {
 		glGenBuffers(1, &g_tessellationUB);
@@ -427,7 +427,7 @@ void openSubdiv_osdGLMeshDisplay(OpenSubdiv_GLMesh *gl_mesh)
 				glPatchParameteri(GL_PATCH_VERTICES, desc.GetNumControlVertices());
 		}
 
-		GLuint program = bindProgram(mesh, patch);
+		GLuint program = bindProgram(mesh, patch, gl_mesh->level);
 		GLuint diffuseColor = glGetUniformLocation(program, "diffuseColor");
 
 		glProgramUniform4f(program, diffuseColor, 0.4f, 0.4f, 0.8f, 1);
diff --git a/source/blender/blenkernel/intern/CCGSubSurf.c b/source/blender/blenkernel/intern/CCGSubSurf.c
index 1e06a67..7d6d783 100644
--- a/source/blender/blenkernel/intern/CCGSubSurf.c
+++ b/source/blender/blenkernel/intern/CCGSubSurf.c
@@ -462,6 +462,7 @@ struct CCGSubSurf {
 #ifdef WITH_OPENSUBDIV
 	struct OpenSubdiv_EvaluatorDescr *osd_evaluator;
 	struct OpenSubdiv_GLMesh *osd_mesh;
+	bool osd_mesh_invalid;
 	unsigned int osd_vao;
 	bool skip_grids;
 	short osd_compute;
@@ -922,6 +923,7 @@ CCGSubSurf *ccgSubSurf_new(CCGMeshIFC *ifc, int subdivLevels, CCGAllocatorIFC *a
 #ifdef WITH_OPENSUBDIV
 		ss->osd_evaluator = NULL;
 		ss->osd_mesh = NULL;
+		ss->osd_mesh_invalid = false;
 		ss->osd_vao = 0;
 		ss->skip_grids = false;
 		ss->osd_compute = 0;
@@ -940,6 +942,7 @@ void ccgSubSurf_free(CCGSubSurf *ss)
 		openSubdiv_deleteEvaluatorDescr(ss->osd_evaluator);
 	}
 	if (ss->osd_mesh != NULL) {
+		/* TODO(sergey): Make sure free happens form the main thread! */
 		openSubdiv_deleteOsdGLMesh(ss->osd_mesh);
 	}
 	if (ss->osd_vao != 0) {
@@ -2327,6 +2330,12 @@ void ccgSubSurf_prepareGLMesh(CCGSubSurf *ss)
 		glGenVertexArrays(1, &ss->osd_vao);
 	}
 
+	if (ss->osd_mesh_invalid) {
+		openSubdiv_deleteOsdGLMesh(ss->osd_mesh);
+		ss->osd_mesh = NULL;
+		ss->osd_mesh_invalid = false;
+	}
+
 	if (ss->osd_mesh == NULL) {
 		ss->osd_mesh = openSubdiv_createOsdGLMeshFromEvaluator(
 			ss->osd_evaluator,
@@ -2558,10 +2567,11 @@ static bool opensubdiv_ensureEvaluator(CCGSubSurf *ss)
 
 			/* We would also need to re-create gl mesh from sratch
 			 * if the topology changes.
+			 * Here we only tag for free, actual free should happen
+			 * from the main thread.
 			 */
 			if (ss->osd_mesh) {
-				openSubdiv_deleteOsdGLMesh(ss->osd_mesh);
-				ss->osd_mesh = NULL;
+				ss->osd_mesh_invalid = true;
 			}
 		}
 	}




More information about the Bf-blender-cvs mailing list