[Bf-blender-cvs] SVN commit: /data/svn/bf-blender [52235] trunk/blender/source: Image thread safe improvements

Sergey Sharybin sergey.vfx at gmail.com
Thu Nov 15 17:00:00 CET 2012


Revision: 52235
          http://projects.blender.org/scm/viewvc.php?view=rev&root=bf-blender&revision=52235
Author:   nazgul
Date:     2012-11-15 15:59:58 +0000 (Thu, 15 Nov 2012)
Log Message:
-----------
Image thread safe improvements

This commit makes BKE_image_acquire_ibuf referencing result, which means once
some area requested for image buffer, it'll be guaranteed this buffer wouldn't
be freed by image signal.

To de-reference buffer BKE_image_release_ibuf should now always be used.

To make referencing working correct we can not rely on result of
image_get_ibuf_threadsafe called outside from thread lock. This is so because
we need to guarantee getting image buffer from list of loaded buffers and it's
referencing happens atomic. Without lock here it is possible that between call
of image_get_ibuf_threadsafe and referencing the buffer IMA_SIGNAL_FREE would
be called. Image signal handling too is blocking now to prevent such a
situation.

Threads are locking by spinlock, which are faster than mutexes. There were some
slowdown reports in the past about render slowdown when using OSX on Xeon CPU.
It shouldn't happen with spin locks, but more tests on different hardware would
be really welcome. So far can not see speed regressions on own computers.

This commit also removes BKE_image_get_ibuf, because it was not so intuitive
when get_ibuf and acquire_ibuf should be used.

Thanks to Ton and Brecht for discussion/review :)

Modified Paths:
--------------
    trunk/blender/source/blender/blenkernel/BKE_image.h
    trunk/blender/source/blender/blenkernel/intern/blender.c
    trunk/blender/source/blender/blenkernel/intern/brush.c
    trunk/blender/source/blender/blenkernel/intern/image.c
    trunk/blender/source/blender/blenlib/BLI_threads.h
    trunk/blender/source/blender/blenlib/intern/threads.c
    trunk/blender/source/blender/blenloader/intern/readfile.c
    trunk/blender/source/blender/collada/ImageExporter.cpp
    trunk/blender/source/blender/compositor/nodes/COM_ImageNode.cpp
    trunk/blender/source/blender/compositor/operations/COM_ImageOperation.cpp
    trunk/blender/source/blender/compositor/operations/COM_ViewerBaseOperation.cpp
    trunk/blender/source/blender/editors/include/ED_image.h
    trunk/blender/source/blender/editors/object/object_bake.c
    trunk/blender/source/blender/editors/object/object_edit.c
    trunk/blender/source/blender/editors/render/render_internal.c
    trunk/blender/source/blender/editors/render/render_opengl.c
    trunk/blender/source/blender/editors/render/render_preview.c
    trunk/blender/source/blender/editors/sculpt_paint/paint_image.c
    trunk/blender/source/blender/editors/space_image/image_buttons.c
    trunk/blender/source/blender/editors/space_image/image_draw.c
    trunk/blender/source/blender/editors/space_image/image_edit.c
    trunk/blender/source/blender/editors/space_image/image_ops.c
    trunk/blender/source/blender/editors/space_image/space_image.c
    trunk/blender/source/blender/editors/space_info/info_ops.c
    trunk/blender/source/blender/editors/space_node/drawnode.c
    trunk/blender/source/blender/editors/space_node/node_view.c
    trunk/blender/source/blender/editors/space_view3d/drawmesh.c
    trunk/blender/source/blender/editors/space_view3d/drawobject.c
    trunk/blender/source/blender/editors/space_view3d/view3d_draw.c
    trunk/blender/source/blender/gpu/intern/gpu_draw.c
    trunk/blender/source/blender/gpu/intern/gpu_material.c
    trunk/blender/source/blender/makesrna/intern/rna_image.c
    trunk/blender/source/blender/makesrna/intern/rna_image_api.c
    trunk/blender/source/blender/makesrna/intern/rna_space.c
    trunk/blender/source/blender/nodes/composite/nodes/node_composite_image.c
    trunk/blender/source/blender/nodes/composite/nodes/node_composite_splitViewer.c
    trunk/blender/source/blender/nodes/composite/nodes/node_composite_viewer.c
    trunk/blender/source/blender/nodes/shader/nodes/node_shader_tex_environment.c
    trunk/blender/source/blender/nodes/shader/nodes/node_shader_tex_image.c
    trunk/blender/source/blender/nodes/shader/nodes/node_shader_texture.c
    trunk/blender/source/blender/nodes/texture/nodes/node_texture_image.c
    trunk/blender/source/blender/render/intern/source/envmap.c
    trunk/blender/source/blender/render/intern/source/imagetexture.c
    trunk/blender/source/blender/render/intern/source/pipeline.c
    trunk/blender/source/blender/render/intern/source/render_texture.c
    trunk/blender/source/blender/render/intern/source/rendercore.c
    trunk/blender/source/blender/render/intern/source/voxeldata.c
    trunk/blender/source/blender/windowmanager/intern/wm_playanim.c
    trunk/blender/source/creator/creator.c
    trunk/blender/source/gameengine/GamePlayer/ghost/GPG_ghost.cpp
    trunk/blender/source/gameengine/Ketsji/BL_Texture.cpp

Modified: trunk/blender/source/blender/blenkernel/BKE_image.h
===================================================================
--- trunk/blender/source/blender/blenkernel/BKE_image.h	2012-11-15 15:37:58 UTC (rev 52234)
+++ trunk/blender/source/blender/blenkernel/BKE_image.h	2012-11-15 15:59:58 UTC (rev 52235)
@@ -48,6 +48,9 @@
 
 #define IMA_MAX_SPACE       64
 
+void   BKE_images_init(void);
+void   BKE_images_exit(void);
+
 /* call from library */
 void    BKE_image_free(struct Image *me);
 
@@ -133,14 +136,13 @@
 #define IMA_CHAN_FLAG_RGB   2
 #define IMA_CHAN_FLAG_ALPHA 4
 
-/* depending Image type, and (optional) ImageUser setting it returns ibuf */
-/* always call to make signals work */
-struct ImBuf *BKE_image_get_ibuf(struct Image *ima, struct ImageUser *iuser);
+/* checks whether there's an image buffer for given image and user */
+int BKE_image_has_ibuf(struct Image *ima, struct ImageUser *iuser);
 
 /* same as above, but can be used to retrieve images being rendered in
  * a thread safe way, always call both acquire and release */
 struct ImBuf *BKE_image_acquire_ibuf(struct Image *ima, struct ImageUser *iuser, void **lock_r);
-void BKE_image_release_ibuf(struct Image *ima, void *lock);
+void BKE_image_release_ibuf(struct Image *ima, struct ImBuf *ibuf, void *lock);
 
 /* returns a new image or NULL if it can't load */
 struct Image *BKE_image_load(const char *filepath);

Modified: trunk/blender/source/blender/blenkernel/intern/blender.c
===================================================================
--- trunk/blender/source/blender/blenkernel/intern/blender.c	2012-11-15 15:37:58 UTC (rev 52234)
+++ trunk/blender/source/blender/blenkernel/intern/blender.c	2012-11-15 15:59:58 UTC (rev 52235)
@@ -70,6 +70,7 @@
 #include "BKE_displist.h"
 #include "BKE_global.h"
 #include "BKE_idprop.h"
+#include "BKE_image.h"
 #include "BKE_ipo.h"
 #include "BKE_library.h"
 #include "BKE_main.h"
@@ -113,6 +114,7 @@
 	BKE_spacetypes_free();      /* after free main, it uses space callbacks */
 	
 	IMB_exit();
+	BKE_images_exit();
 
 	BLI_callback_global_finalize();
 

Modified: trunk/blender/source/blender/blenkernel/intern/brush.c
===================================================================
--- trunk/blender/source/blender/blenkernel/intern/brush.c	2012-11-15 15:37:58 UTC (rev 52234)
+++ trunk/blender/source/blender/blenkernel/intern/brush.c	2012-11-15 15:59:58 UTC (rev 52235)
@@ -1287,8 +1287,6 @@
 
 		texcache = MEM_callocN(sizeof(int) * side * side, "Brush texture cache");
 
-		BKE_image_get_ibuf(mtex->tex->ima, NULL);
-		
 		/*do normalized cannonical view coords for texture*/
 		for (y = -1.0, iy = 0; iy < side; iy++, y += step) {
 			for (x = -1.0, ix = 0; ix < side; ix++, x += step) {

Modified: trunk/blender/source/blender/blenkernel/intern/image.c
===================================================================
--- trunk/blender/source/blender/blenkernel/intern/image.c	2012-11-15 15:37:58 UTC (rev 52234)
+++ trunk/blender/source/blender/blenkernel/intern/image.c	2012-11-15 15:59:58 UTC (rev 52235)
@@ -100,6 +100,8 @@
 
 #include "WM_api.h"
 
+static SpinLock image_spin;
+
 /* max int, to indicate we don't store sequences in ibuf */
 #define IMA_NO_INDEX    0x7FEFEFEF
 
@@ -108,6 +110,16 @@
 #define IMA_INDEX_FRAME(index)          (index >> 10)
 #define IMA_INDEX_PASS(index)           (index & ~1023)
 
+void BKE_images_init(void)
+{
+	BLI_spin_init(&image_spin);
+}
+
+void BKE_images_exit(void)
+{
+	BLI_spin_end(&image_spin);
+}
+
 /* ******** IMAGE PROCESSING ************* */
 
 static void de_interlace_ng(struct ImBuf *ibuf) /* neogeo fields */
@@ -168,13 +180,14 @@
 
 void BKE_image_de_interlace(Image *ima, int odd)
 {
-	ImBuf *ibuf = BKE_image_get_ibuf(ima, NULL);
+	ImBuf *ibuf = BKE_image_acquire_ibuf(ima, NULL, NULL);
 	if (ibuf) {
 		if (odd)
 			de_interlace_st(ibuf);
 		else
 			de_interlace_ng(ibuf);
 	}
+	BKE_image_release_ibuf(ima, ibuf, NULL);
 }
 
 /* ***************** ALLOC & FREE, DATA MANAGING *************** */
@@ -260,8 +273,9 @@
 	/* this function is intended to be thread safe. with IMA_NO_INDEX this
 	 * should be OK, but when iterating over the list this is more tricky
 	 * */
-	if (index == IMA_NO_INDEX)
+	if (index == IMA_NO_INDEX) {
 		return ima->ibufs.first;
+	}
 	else {
 		ImBuf *ibuf;
 
@@ -269,9 +283,9 @@
 		for (ibuf = ima->ibufs.first; ibuf; ibuf = ibuf->next)
 			if (ibuf->index == index)
 				return ibuf;
+	}
 
-		return NULL;
-	}
+	return NULL;
 }
 
 /* no ima->ibuf anymore, but listbase */
@@ -534,7 +548,7 @@
 		ibuf->userflags |= IB_BITMAPDIRTY;
 	}
 
-	BKE_image_release_ibuf(image, lock);
+	BKE_image_release_ibuf(image, ibuf, lock);
 
 	return (ibuf != NULL);
 }
@@ -2081,6 +2095,8 @@
 	if (ima == NULL)
 		return;
 
+	BLI_spin_lock(&image_spin);
+
 	switch (signal) {
 		case IMA_SIGNAL_FREE:
 			image_free_buffers(ima);
@@ -2167,6 +2183,8 @@
 			}
 		}
 	}
+
+	BLI_spin_unlock(&image_spin);
 }
 
 /* if layer or pass changes, we need an index for the imbufs list */
@@ -2320,7 +2338,7 @@
 
 	if (ibuf) {
 #ifdef WITH_OPENEXR
-		/* handle multilayer case, don't assign ibuf. will be handled in BKE_image_get_ibuf */
+		/* handle multilayer case, don't assign ibuf. will be handled in BKE_image_acquire_ibuf */
 		if (ibuf->ftype == OPENEXR && ibuf->userdata) {
 			image_create_multilayer(ima, ibuf, frame);
 			ima->type = IMA_TYPE_MULTILAYER;
@@ -2482,7 +2500,7 @@
 	}
 
 	if (ibuf) {
-		/* handle multilayer case, don't assign ibuf. will be handled in BKE_image_get_ibuf */
+		/* handle multilayer case, don't assign ibuf. will be handled in BKE_image_acquire_ibuf */
 		if (ibuf->ftype == OPENEXR && ibuf->userdata) {
 			image_create_multilayer(ima, ibuf, cfra);
 			ima->type = IMA_TYPE_MULTILAYER;
@@ -2751,38 +2769,32 @@
 		 * a big bottleneck */
 	}
 
-	*frame_r = frame;
-	*index_r = index;
+	if (frame_r)
+		*frame_r = frame;
 
+	if (index_r)
+		*index_r = index;
+
 	return ibuf;
 }
 
-/* Checks optional ImageUser and verifies/creates ImBuf. */
-/* use this one if you want to get a render result in progress,
- * if not, use BKE_image_get_ibuf which doesn't require a release */
-ImBuf *BKE_image_acquire_ibuf(Image *ima, ImageUser *iuser, void **lock_r)
+/* Checks optional ImageUser and verifies/creates ImBuf.
+ *
+ * not thread-safe, so callee should worry about thread locks
+ */
+static ImBuf *image_acquire_ibuf(Image *ima, ImageUser *iuser, void **lock_r)
 {
 	ImBuf *ibuf = NULL;
 	float color[] = {0, 0, 0, 1};
 	int frame = 0, index = 0;
 
-	/* This function is intended to be thread-safe. It postpones the mutex lock
-	 * until it needs to load the image, if the image is already there it
-	 * should just get the pointer and return. The reason is that a lot of mutex
-	 * locks appears to be very slow on certain multicore macs, causing a render
-	 * with image textures to actually slow down as more threads are used.
-	 *
-	 * Note that all the image loading functions should also make sure they do
-	 * things in a threadsafe way for image_get_ibuf_threadsafe to work correct.
-	 * That means, the last two steps must be, 1) add the ibuf to the list and
-	 * 2) set ima/iuser->ok to 0 to IMA_OK_LOADED */
-
 	if (lock_r)
 		*lock_r = NULL;
 
 	/* quick reject tests */
 	if (ima == NULL)
 		return NULL;
+
 	if (iuser) {
 		if (iuser->ok == 0)
 			return NULL;
@@ -2790,95 +2802,71 @@
 	else if (ima->ok == 0)
 		return NULL;
 
-	/* try to get the ibuf without locking */
 	ibuf = image_get_ibuf_threadsafe(ima, iuser, &frame, &index);
 
 	if (ibuf == NULL) {
-		/* couldn't get ibuf and image is not ok, so let's lock and try to
-		 * load the image */
-		BLI_lock_thread(LOCK_IMAGE);
-
-		/* need to check ok flag and loading ibuf again, because the situation
-		 * might have changed in the meantime */
-		if (iuser) {
-			if (iuser->ok == 0) {
-				BLI_unlock_thread(LOCK_IMAGE);
-				return NULL;
-			}
+		/* we are sure we have to load the ibuf, using source and type */
+		if (ima->source == IMA_SRC_MOVIE) {
+			/* source is from single file, use flipbook to store ibuf */
+			ibuf = image_load_movie_file(ima, iuser, frame);
 		}
-		else if (ima->ok == 0) {
-			BLI_unlock_thread(LOCK_IMAGE);
-			return NULL;
-		}
-
-		ibuf = image_get_ibuf_threadsafe(ima, iuser, &frame, &index);
-
-		if (ibuf == NULL) {
-			/* we are sure we have to load the ibuf, using source and type */
-			if (ima->source == IMA_SRC_MOVIE) {
-				/* source is from single file, use flipbook to store ibuf */
-				ibuf = image_load_movie_file(ima, iuser, frame);
+		else if (ima->source == IMA_SRC_SEQUENCE) {
+			if (ima->type == IMA_TYPE_IMAGE) {
+				/* regular files, ibufs in flipbook, allows saving */
+				ibuf = image_load_sequence_file(ima, iuser, frame);
 			}
-			else if (ima->source == IMA_SRC_SEQUENCE) {
-				if (ima->type == IMA_TYPE_IMAGE) {
-					/* regular files, ibufs in flipbook, allows saving */
-					ibuf = image_load_sequence_file(ima, iuser, frame);
-				}
-				/* no else; on load the ima type can change */
-				if (ima->type == IMA_TYPE_MULTILAYER) {
-					/* only 1 layer/pass stored in imbufs, no exrhandle anim storage, no saving */
-					ibuf = image_load_sequence_multilayer(ima, iuser, frame);
-				}
+			/* no else; on load the ima type can change */
+			if (ima->type == IMA_TYPE_MULTILAYER) {
+				/* only 1 layer/pass stored in imbufs, no exrhandle anim storage, no saving */
+				ibuf = image_load_sequence_multilayer(ima, iuser, frame);
 			}
-			else if (ima->source == IMA_SRC_FILE) {
+		}
+		else if (ima->source == IMA_SRC_FILE) {
 
-				if (ima->type == IMA_TYPE_IMAGE)
-					ibuf = image_load_image_file(ima, iuser, frame);  /* cfra only for '#', this global is OK */
-				/* no else; on load the ima type can change */
-				if (ima->type == IMA_TYPE_MULTILAYER)
-					/* keeps render result, stores ibufs in listbase, allows saving */
-					ibuf = image_get_ibuf_multilayer(ima, iuser);
+			if (ima->type == IMA_TYPE_IMAGE)
+				ibuf = image_load_image_file(ima, iuser, frame);  /* cfra only for '#', this global is OK */
+			/* no else; on load the ima type can change */
+			if (ima->type == IMA_TYPE_MULTILAYER)
+				/* keeps render result, stores ibufs in listbase, allows saving */
+				ibuf = image_get_ibuf_multilayer(ima, iuser);
 
+		}
+		else if (ima->source == IMA_SRC_GENERATED) {
+			/* generated is: ibuf is allocated dynamically */
+			/* UV testgrid or black or solid etc */
+			if (ima->gen_x == 0) ima->gen_x = 1024;
+			if (ima->gen_y == 0) ima->gen_y = 1024;

@@ Diff output truncated at 10240 characters. @@



More information about the Bf-blender-cvs mailing list