Fix deadlock happening when using Save Buffers for render

Summary:
Issue was caused by the same tile being written twice to
the EXR file. This was happening because of partial update
of work-in-progress tiles was merging result to the final
render result in order to make color management pipeline
happy.

We need to avoid such a merges and keep memory usage as
low as possible when Save Buffers is enabled.

Now render pipeline will allocate special display buffer
in render layer which will contain combined pass in the
display space. This keeps memory usage as low as we can
do at this moment.

There's one weak thing which is changing color management
settings during rendering would lead to lossy conversion.
This is because render result's display buffer uses color
space from the time when rendering was invoked.

This is actually what was happening in previous release
already actually so not a big issue.

Reviewers: brecht

Reviewed By: brecht

Differential Revision: https://developer.blender.org/D162

8 files changed, 154 insertions, 59 deletions

diff --git a/source/blender/blenkernel/intern/image.c b/source/blender/blenkernel/intern/image.c
index 3d59b719ca0..288443db650 100644
--- a/source/blender/blenkernel/intern/image.c
+++ b/source/blender/blenkernel/intern/image.c
@@ -2773,6 +2773,7 @@ static ImBuf *image_get_render_result(Image *ima, ImageUser *iuser, void **lock_
 	int channels, layer, pass;
 	ImBuf *ibuf;
 	int from_render = (ima->render_slot == ima->last_render_slot);
+	bool byte_buffer_in_display_space = false;
 
 	if (!(iuser && iuser->scene))
 		return NULL;
@@ -2835,6 +2836,13 @@ static ImBuf *image_get_render_result(Image *ima, ImageUser *iuser, void **lock_
 			/* there's no combined pass, is in renderlayer itself */
 			if (pass == 0) {
 				rectf = rl->rectf;
+				if (rectf == NULL) {
+					/* Happens when Save Buffers is enabled.
+					 * Use display buffer stored in the render layer.
+					 */
+					rect = (unsigned int *) rl->display_buffer;
+					byte_buffer_in_display_space = true;
+				}
 			}
 			else {
 				rpass = BLI_findlink(&rl->passes, pass - 1);
@@ -2859,6 +2867,27 @@ static ImBuf *image_get_render_result(Image *ima, ImageUser *iuser, void **lock_
 		image_assign_ibuf(ima, ibuf, IMA_NO_INDEX, 0);
 	}
 
+	/* Set color space settings for a byte buffer.
+	 *
+	 * This is mainly to make it so color management treats byte buffer
+	 * from render result with Save Buffers enabled as final display buffer
+	 * and doesnt' apply any color management on it.
+	 *
+	 * For other cases we need to be sure it stays to default byte buffer space.
+	 */
+	if (ibuf->rect != rect) {
+		if (byte_buffer_in_display_space) {
+			const char *colorspace =
+				IMB_colormanagement_get_display_colorspace_name(&iuser->scene->view_settings,
+			                                                    &iuser->scene->display_settings);
+			IMB_colormanagement_assign_rect_colorspace(ibuf, colorspace);
+		}
+		else {
+			const char *colorspace = IMB_colormanagement_role_colorspace_name_get(COLOR_ROLE_DEFAULT_BYTE);
+			IMB_colormanagement_assign_rect_colorspace(ibuf, colorspace);
+		}
+	}
+
 	/* invalidate color managed buffers if render result changed */
 	BLI_lock_thread(LOCK_COLORMANAGE);
 	if (ibuf->x != rres.rectx || ibuf->y != rres.recty || ibuf->rect_float != rectf) {
diff --git a/source/blender/compositor/operations/COM_ViewerOperation.cpp b/source/blender/compositor/operations/COM_ViewerOperation.cpp
index aa0806944db..f53f2b87f98 100644
--- a/source/blender/compositor/operations/COM_ViewerOperation.cpp
+++ b/source/blender/compositor/operations/COM_ViewerOperation.cpp
@@ -171,7 +171,7 @@ void ViewerOperation::updateImage(rcti *rect)
 {
 	IMB_partial_display_buffer_update(this->m_ibuf, this->m_outputBuffer, NULL, getWidth(), 0, 0,
 	                                  this->m_viewSettings, this->m_displaySettings,
-	                                  rect->xmin, rect->ymin, rect->xmax, rect->ymax);
+	                                  rect->xmin, rect->ymin, rect->xmax, rect->ymax, false);
 
 	this->updateDraw();
 }
diff --git a/source/blender/editors/render/render_internal.c b/source/blender/editors/render/render_internal.c
index b4414cf3f54..deec4225eb9 100644
--- a/source/blender/editors/render/render_internal.c
+++ b/source/blender/editors/render/render_internal.c
@@ -49,6 +49,7 @@
 
 #include "BKE_blender.h"
 #include "BKE_context.h"
+#include "BKE_colortools.h"
 #include "BKE_depsgraph.h"
 #include "BKE_freestyle.h"
 #include "BKE_global.h"
@@ -93,13 +94,39 @@
 /* Render Callbacks */
 static int render_break(void *rjv);
 
+typedef struct RenderJob {
+	Main *main;
+	Scene *scene;
+	Render *re;
+	SceneRenderLayer *srl;
+	struct Object *camera_override;
+	int lay_override;
+	bool v3d_override;
+	short anim, write_still;
+	Image *image;
+	ImageUser iuser;
+	bool image_outdated;
+	short *stop;
+	short *do_update;
+	float *progress;
+	ReportList *reports;
+	int orig_layer;
+	int last_layer;
+	ScrArea *sa;
+	ColorManagedViewSettings view_settings;
+	ColorManagedDisplaySettings display_settings;
+} RenderJob;
+
 /* called inside thread! */
-static void image_buffer_rect_update(Scene *scene, RenderResult *rr, ImBuf *ibuf, ImageUser *iuser, volatile rcti *renrect)
+static void image_buffer_rect_update(RenderJob *rj, RenderResult *rr, ImBuf *ibuf, ImageUser *iuser, volatile rcti *renrect)
 {
+	Scene *scene = rj->scene;
 	float *rectf = NULL;
 	int ymin, ymax, xmin, xmax;
 	int rymin, rxmin;
 	int linear_stride, linear_offset_x, linear_offset_y;
+	ColorManagedViewSettings *view_settings;
+	ColorManagedDisplaySettings *display_settings;
 
 	if (ibuf->userflags & IB_DISPLAY_BUFFER_INVALID) {
 		/* The whole image buffer it so be color managed again anyway. */
@@ -189,10 +216,23 @@ static void image_buffer_rect_update(Scene *scene, RenderResult *rr, ImBuf *ibuf
 		linear_offset_y = 0;
 	}
 
+	if (rr->do_exr_tile) {
+		/* We don't support changing color management settings during rendering
+		 * when using Save Buffers option.
+		 */
+		view_settings = &rj->view_settings;
+		display_settings = &rj->display_settings;
+	}
+	else {
+		view_settings = &scene->view_settings;
+		display_settings = &scene->display_settings;
+	}
+
 	IMB_partial_display_buffer_update(ibuf, rectf, NULL,
 	                                  linear_stride, linear_offset_x, linear_offset_y,
-	                                  &scene->view_settings, &scene->display_settings,
-	                                  rxmin, rymin, rxmin + xmax, rymin + ymax);
+	                                  view_settings, display_settings,
+	                                  rxmin, rymin, rxmin + xmax, rymin + ymax,
+	                                  rr->do_exr_tile);
 }
 
 /* ****************************** render invoking ***************** */
@@ -286,31 +326,11 @@ static int screen_render_exec(bContext *C, wmOperator *op)
 	return OPERATOR_FINISHED;
 }
 
-typedef struct RenderJob {
-	Main *main;
-	Scene *scene;
-	Render *re;
-	SceneRenderLayer *srl;
-	struct Object *camera_override;
-	int lay_override;
-	bool v3d_override;
-	short anim, write_still;
-	Image *image;
-	ImageUser iuser;
-	bool image_outdated;
-	short *stop;
-	short *do_update;
-	float *progress;
-	ReportList *reports;
-	int orig_layer;
-	int last_layer;
-	ScrArea *sa;
-} RenderJob;
-
 static void render_freejob(void *rjv)
 {
 	RenderJob *rj = rjv;
 
+	BKE_color_managed_view_settings_free(&rj->view_settings);
 	MEM_freeN(rj);
 }
 
@@ -516,11 +536,16 @@ static void image_rect_update(void *rjv, RenderResult *rr, volatile rcti *renrec
 	if (ibuf) {
 		/* Don't waste time on CPU side color management if
 		 * image will be displayed using GLSL.
+		 *
+		 * Need to update rect if Save Buffers enabled because in
+		 * this case GLSL doesn't have original float buffer to
+		 * operate with.
 		 */
-		if (ibuf->channels == 1 ||
+		if (rr->do_exr_tile ||
+		    ibuf->channels == 1 ||
 		    U.image_draw_method != IMAGE_DRAW_METHOD_GLSL)
 		{
-			image_buffer_rect_update(rj->scene, rr, ibuf, &rj->iuser, renrect);
+			image_buffer_rect_update(rj, rr, ibuf, &rj->iuser, renrect);
 		}
 
 		/* make jobs timer to send notifier */
@@ -793,6 +818,9 @@ static int screen_render_invoke(bContext *C, wmOperator *op, const wmEvent *even
 	rj->last_layer = 0;
 	rj->sa = sa;
 
+	BKE_color_managed_display_settings_copy(&rj->display_settings, &scene->display_settings);
+	BKE_color_managed_view_settings_copy(&rj->view_settings, &scene->view_settings);
+
 	if (sa) {
 		SpaceImage *sima = sa->spacedata.first;
 		rj->orig_layer = sima->iuser.layer;
diff --git a/source/blender/imbuf/IMB_colormanagement.h b/source/blender/imbuf/IMB_colormanagement.h
index 263216cc5da..262e87bedf5 100644
--- a/source/blender/imbuf/IMB_colormanagement.h
+++ b/source/blender/imbuf/IMB_colormanagement.h
@@ -113,6 +113,9 @@ void IMB_colormanagement_display_settings_from_ctx(const struct bContext *C,
                                                    struct ColorManagedViewSettings **view_settings_r,
                                                    struct ColorManagedDisplaySettings **display_settings_r);
 
+const char *IMB_colormanagement_get_display_colorspace_name(const struct ColorManagedViewSettings *view_settings,
+                                                            const struct ColorManagedDisplaySettings *display_settings);
+
 unsigned char *IMB_display_buffer_acquire(struct ImBuf *ibuf, const struct ColorManagedViewSettings *view_settings,
                                           const struct ColorManagedDisplaySettings *display_settings, void **cache_handle);
 unsigned char *IMB_display_buffer_acquire_ctx(const struct bContext *C, struct ImBuf *ibuf, void **cache_handle);
@@ -153,9 +156,11 @@ void IMB_colormanagement_colorspace_items_add(struct EnumPropertyItem **items, i
 
 /* ** Tile-based buffer management ** */
 void IMB_partial_display_buffer_update(struct ImBuf *ibuf, const float *linear_buffer, const unsigned char *buffer_byte,
-                                       int stride, int offset_x, int offset_y, const struct ColorManagedViewSettings *view_settings,
+                                       int stride, int offset_x, int offset_y,
+                                       const struct ColorManagedViewSettings *view_settings,
                                        const struct ColorManagedDisplaySettings *display_settings,
-                                       int xmin, int ymin, int xmax, int ymax);
+                                       int xmin, int ymin, int xmax, int ymax,
+                                       bool copy_display_to_byte_buffer);
 
 void IMB_partial_display_buffer_update_delayed(struct ImBuf *ibuf, int xmin, int ymin, int xmax, int ymax);
 
diff --git a/source/blender/imbuf/intern/colormanagement.c b/source/blender/imbuf/intern/colormanagement.c
index 6ca3e1ae2a9..e5884f78b75 100644
--- a/source/blender/imbuf/intern/colormanagement.c
+++ b/source/blender/imbuf/intern/colormanagement.c
@@ -745,8 +745,8 @@ void IMB_colormanagement_display_settings_from_ctx(const bContext *C,
 	}
 }
 
-static const char *display_transform_get_colorspace_name(const ColorManagedViewSettings *view_settings,
-                                                         const ColorManagedDisplaySettings *display_settings)
+const char *IMB_colormanagement_get_display_colorspace_name(const ColorManagedViewSettings *view_settings,
+                                                            const ColorManagedDisplaySettings *display_settings)
 {
 	OCIO_ConstConfigRcPtr *config = OCIO_getCurrentConfig();
 
@@ -764,7 +764,7 @@ static const char *display_transform_get_colorspace_name(const ColorManagedViewS
 static ColorSpace *display_transform_get_colorspace(const ColorManagedViewSettings *view_settings,
                                                     const ColorManagedDisplaySettings *display_settings)
 {
-	const char *colorspace_name = display_transform_get_colorspace_name(view_settings, display_settings);
+	const char *colorspace_name = IMB_colormanagement_get_display_colorspace_name(view_settings, display_settings);
 
 	if (colorspace_name)
 		return colormanage_colorspace_get_named(colorspace_name);
@@ -1510,7 +1510,7 @@ static bool is_ibuf_rect_in_display_space(ImBuf *ibuf, const ColorManagedViewSet
 	    view_settings->gamma == 1.0f)
 	{
 		const char *from_colorspace = ibuf->rect_colorspace->name;
-		const char *to_colorspace = display_transform_get_colorspace_name(view_settings, display_settings);
+		const char *to_colorspace = IMB_colormanagement_get_display_colorspace_name(view_settings, display_settings);
 
 		if (to_colorspace && !strcmp(from_colorspace, to_colorspace))
 			return true;
@@ -2034,7 +2034,8 @@ unsigned char *IMB_display_buffer_acquire(ImBuf *ibuf, const ColorManagedViewSet
 			IMB_partial_display_buffer_update(ibuf, ibuf->rect_float, (unsigned char *) ibuf->rect,
 			                                  ibuf->x, 0, 0, applied_view_settings, display_settings,
 			                                  ibuf->invalid_rect.xmin, ibuf->invalid_rect.ymin,
-			                                  ibuf->invalid_rect.xmax, ibuf->invalid_rect.ymax);
+			                                  ibuf->invalid_rect.xmax, ibuf->invalid_rect.ymax,
+			                                  false);
 		}
 
 		BLI_rcti_init(&ibuf->invalid_rect, 0, 0, 0, 0);
@@ -2735,16 +2736,20 @@ static void partial_buffer_update_rect(ImBuf *ibuf, unsigned char *display_buffe
 }
 
 void IMB_partial_display_buffer_update(ImBuf *ibuf, const float *linear_buffer, const unsigned char *byte_buffer,
-                                       int stride, int offset_x, int offset_y, const ColorManagedViewSettings *view_settings,
+                                       int stride, int offset_x, int offset_y,
+                                       const ColorManagedViewSettings *view_settings,
                                        const ColorManagedDisplaySettings *display_settings,
-                                       int xmin, int ymin, int xmax, int ymax)
+                                       int xmin, int ymin, int xmax, int ymax,
+                                       bool copy_display_to_byte_buffer)
 {
+	ColormanageCacheViewSettings cache_view_settings;
+	ColormanageCacheDisplaySettings cache_display_settings;
+	void *cache_handle = NULL;
+	unsigned char *display_buffer = NULL;
+	int buffer_width = ibuf->x;
+
 	if (ibuf->display_buffer_flags) {
-		ColormanageCacheViewSettings cache_view_settings;
-		ColormanageCacheDisplaySettings cache_display_settings;
-		void *cache_handle = NULL;
-		unsigned char *display_buffer = NULL;
-		int view_flag, display_index, buffer_width;
+		int view_flag, display_index;
 
 		colormanage_view_settings_to_cache(ibuf, &cache_view_settings, view_settings);
 		colormanage_display_settings_to_cache(&cache_display_settings, display_settings);
@@ -2753,6 +2758,7 @@ void IMB_partial_display_buffer_update(ImBuf *ibuf, const float *linear_buffer,
 		display_index = cache_display_settings.display - 1;
 
 		BLI_lock_thread(LOCK_COLORMANAGE);
+
 		if ((ibuf->userflags & IB_DISPLAY_BUFFER_INVALID) == 0)
 			display_buffer = colormanage_cache_get(ibuf, &cache_view_settings, &cache_display_settings, &cache_handle);
 
@@ -2767,28 +2773,42 @@ void IMB_partial_display_buffer_update(ImBuf *ibuf, const float *linear_buffer,
 		ibuf->display_buffer_flags[display_index] |= view_flag;
 
 		BLI_unlock_thread(LOCK_COLORMANAGE);
+	}
 
-		if (display_buffer) {
-			ColormanageProcessor *cm_processor = NULL;
-			bool skip_transform = false;
+	if (display_buffer == NULL) {
+		if (copy_display_to_byte_buffer) {
+			display_buffer = (unsigned char *) ibuf->rect;
+		}
+	}
 
-			/* byte buffer is assumed to be in imbuf's rect space, so if byte buffer
-			 * is known we could skip display->linear->display conversion in case
-			 * display color space matches imbuf's rect space
-			 */
-			if (byte_buffer != NULL)
-				skip_transform = is_ibuf_rect_in_display_space(ibuf, view_settings, display_settings);
+	if (display_buffer) {
+		ColormanageProcessor *cm_processor = NULL;
+		bool skip_transform = false;
+
+		/* byte buffer is assumed to be in imbuf's rect space, so if byte buffer
+		 * is known we could skip display->linear->display conversion in case
+		 * display color space matches imbuf's rect space
+		 */
+		if (byte_buffer != NULL)
+			skip_transform = is_ibuf_rect_in_display_space(ibuf, view_settings, display_settings);
 
-			if (!skip_transform)
-				cm_processor = IMB_colormanagement_display_processor_new(view_settings, display_settings);
+		if (!skip_transform)
+			cm_processor = IMB_colormanagement_display_processor_new(view_settings, display_settings);
 
-			partial_buffer_update_rect(ibuf, display_buffer, linear_buffer, byte_buffer, buffer_width, stride,
-			                           offset_x, offset_y, cm_processor, xmin, ymin, xmax, ymax);
+		partial_buffer_update_rect(ibuf, display_buffer, linear_buffer, byte_buffer, buffer_width, stride,
+		                           offset_x, offset_y, cm_processor, xmin, ymin, xmax, ymax);
 
-			if (cm_processor)
-				IMB_colormanagement_processor_free(cm_processor);
+		if (cm_processor)
+			IMB_colormanagement_processor_free(cm_processor);
 
-			IMB_display_buffer_release(cache_handle);
+		IMB_display_buffer_release(cache_handle);
+	}
+
+	if (copy_display_to_byte_buffer && (unsigned char *) ibuf->rect != display_buffer) {
+		int y;
+		for (y = ymin; y < ymax; y++) {
+			int index = y * buffer_width * 4;
+			memcpy(ibuf->rect + index, display_buffer + index, (xmax - xmin) * 4);
 		}
 	}
 }
diff --git a/source/blender/render/extern/include/RE_pipeline.h b/source/blender/render/extern/include/RE_pipeline.h
index 7e8713566e5..4c15ddd6833 100644
--- a/source/blender/render/extern/include/RE_pipeline.h
+++ b/source/blender/render/extern/include/RE_pipeline.h
@@ -91,6 +91,8 @@ typedef struct RenderLayer {
 	float *rectf;		/* 4 float, standard rgba buffer (read not above!) */
 	float *acolrect;	/* 4 float, optional transparent buffer, needs storage for display updates */
 	float *scolrect;	/* 4 float, optional strand buffer, needs storage for display updates */
+	int *display_buffer;	/* 4 char, optional color managed display buffer which is used when
+	                         * Save Buffer is enabled to display combined pass of the screen. */
 	int rectx, recty;
 
 	/* optional saved endresult on disk */
diff --git a/source/blender/render/intern/source/external_engine.c b/source/blender/render/intern/source/external_engine.c
index fa72d5f4057..b9c89f449a7 100644
--- a/source/blender/render/intern/source/external_engine.c
+++ b/source/blender/render/intern/source/external_engine.c
@@ -207,6 +207,11 @@ RenderResult *RE_engine_begin_result(RenderEngine *engine, int x, int y, int w,
 
 	result = render_result_new(re, &disprect, 0, RR_USE_MEM, layername);
 
+	/* Copy EXR tile settings, so pipeline knows whether this is a result
+	 * for Save Buffers enabled rendering.
+	 */
+	result->do_exr_tile = re->result->do_exr_tile;
+
 	/* todo: make this thread safe */
 
 	/* can be NULL if we CLAMP the width or height to 0 */
@@ -263,8 +268,11 @@ void RE_engine_end_result(RenderEngine *engine, RenderResult *result, int cancel
 	}
 
 	if (!cancel || merge_results) {
-		if (re->result->do_exr_tile)
-			render_result_exr_file_merge(re->result, result);
+		if (re->result->do_exr_tile) {
+			if (!cancel) {
+				render_result_exr_file_merge(re->result, result);
+			}
+		}
 		else if (!(re->test_break(re->tbh) && (re->r.scemode & R_BUTS_PREVIEW)))
 			render_result_merge(re->result, result);
 
diff --git a/source/blender/render/intern/source/render_result.c b/source/blender/render/intern/source/render_result.c
index 487de42515d..5e8b030c3fa 100644
--- a/source/blender/render/intern/source/render_result.c
+++ b/source/blender/render/intern/source/render_result.c
@@ -71,6 +71,7 @@ void render_result_free(RenderResult *res)
 		/* acolrect and scolrect are optionally allocated in shade_tile, only free here since it can be used for drawing */
 		if (rl->acolrect) MEM_freeN(rl->acolrect);
 		if (rl->scolrect) MEM_freeN(rl->scolrect);
+		if (rl->display_buffer) MEM_freeN(rl->display_buffer);
 		
 		while (rl->passes.first) {
 			RenderPass *rpass = rl->passes.first;
@@ -501,6 +502,8 @@ RenderResult *render_result_new(Render *re, rcti *partrct, int crop, int savebuf
 		rl->recty = recty;
 		
 		if (rr->do_exr_tile) {
+			rl->display_buffer = MEM_mapallocN(rectx * recty * sizeof(unsigned int), "Combined display space rgba");
+
 			rl->exrhandle = IMB_exr_get_handle();
 
 			IMB_exr_add_channel(rl->exrhandle, rl->name, "Combined.R", 0, 0, NULL);