[Bf-blender-cvs] [e5f51c684bf] cycles-x: Fix wrong display when rendering multiple view layers in Cycles X

[Sergey Sharybin]

Commit: e5f51c684bf94476864f736aaf6ea25059c345b7
Author: Sergey Sharybin
Date:   Thu Sep 16 14:56:45 2021 +0200
Branches: cycles-x
https://developer.blender.org/rBe5f51c684bf94476864f736aaf6ea25059c345b7

Fix wrong display when rendering multiple view layers in Cycles X

The issue was that the texture used by GPUDisplay was never cleared and
was only used for partial updates.

The actual reasoning of that is a bit more tricky than it looks on a
first glance since the GPUDisplay was re-created together with session
via session_reset so one would think it is weird to have a texture from
previous view layer to appear in the new GPUDisplay. Probably caused by
the fact that OpenGL is the same for all view layers, and texture ID is
happened to be the same.

Anyhow, the way texture is created is considered to have an undefined
behavior by the standard when is attempted to be used without explicit
data assignment.

This change makes it so new texture is always guaranteed to be zeroed
and that the GPU display is cleared when rendering multiple views from
within the same session.

The clearing is done by a GPUDisplay subclass, as the parent does not
know details of how to perform the zeroing. For the CPU the zeroing is
done on a mapped memory, as it makes it easy and does not require any
extra OpenGL requirements.

For the graphics interop things a bit more difficult as attempts to map
texture will make graphics interop to fail out (it really wants to be
an exclusive thing which takes care of the memory mapping). So for the
graphics interop clearing is done when memory is mapped by the interop
implementation.

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

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

M	intern/cycles/blender/blender_gpu_display.cpp
M	intern/cycles/blender/blender_gpu_display.h
M	intern/cycles/device/cuda/graphics_interop.cpp
M	intern/cycles/device/cuda/graphics_interop.h
M	intern/cycles/device/device_graphics_interop.h
M	intern/cycles/integrator/path_trace.cpp
M	intern/cycles/integrator/path_trace.h
M	intern/cycles/render/gpu_display.h
M	intern/cycles/render/session.cpp

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

diff --git a/intern/cycles/blender/blender_gpu_display.cpp b/intern/cycles/blender/blender_gpu_display.cpp
index 66de6727d4d..5d3ae9ea4bc 100644
--- a/intern/cycles/blender/blender_gpu_display.cpp
+++ b/intern/cycles/blender/blender_gpu_display.cpp
@@ -325,6 +325,10 @@ bool BlenderGPUDisplay::do_update_begin(const GPUDisplayParams &params,
     texture_.width = texture_width;
     texture_.height = texture_height;
     glBindTexture(GL_TEXTURE_2D, 0);
+
+    /* Texture did change, and no pixel storage was provided. Tag for an explicit zeroing out to
+     * avoid undefined content. */
+    texture_.need_clear = true;
   }
 
   /* Update PBO dimensions if needed.
@@ -413,6 +417,15 @@ half4 *BlenderGPUDisplay::do_map_texture_buffer()
     LOG(ERROR) << "Error mapping BlenderGPUDisplay pixel buffer object.";
   }
 
+  if (texture_.need_clear) {
+    const int64_t texture_width = texture_.width;
+    const int64_t texture_height = texture_.height;
+    memset(reinterpret_cast<void *>(mapped_rgba_pixels),
+           0,
+           texture_width * texture_height * sizeof(half4));
+    texture_.need_clear = false;
+  }
+
   return mapped_rgba_pixels;
 }
 
@@ -435,6 +448,9 @@ DeviceGraphicsInteropDestination BlenderGPUDisplay::do_graphics_interop_get()
   interop_dst.buffer_height = texture_.buffer_height;
   interop_dst.opengl_pbo_id = texture_.gl_pbo_id;
 
+  interop_dst.need_clear = texture_.need_clear;
+  texture_.need_clear = false;
+
   return interop_dst;
 }
 
@@ -452,11 +468,22 @@ void BlenderGPUDisplay::graphics_interop_deactivate()
  * Drawing.
  */
 
+void BlenderGPUDisplay::clear()
+{
+  texture_.need_clear = true;
+}
+
 void BlenderGPUDisplay::do_draw(const GPUDisplayParams &params)
 {
   /* See do_update_begin() for why no locking is required here. */
   const bool transparent = true;  // TODO(sergey): Derive this from Film.
 
+  if (texture_.need_clear) {
+    /* Texture is requested to be cleared and was not yet cleared.
+     * Do early return which should be equivalent of drawing all-zero texture. */
+    return;
+  }
+
   if (!gl_draw_resources_ensure()) {
     return;
   }
diff --git a/intern/cycles/blender/blender_gpu_display.h b/intern/cycles/blender/blender_gpu_display.h
index 479b63ec4d8..b7eddf0afa7 100644
--- a/intern/cycles/blender/blender_gpu_display.h
+++ b/intern/cycles/blender/blender_gpu_display.h
@@ -16,6 +16,8 @@
 
 #pragma once
 
+#include <atomic>
+
 #include "MEM_guardedalloc.h"
 
 #include "RNA_blender_cpp.h"
@@ -103,6 +105,8 @@ class BlenderGPUDisplay : public GPUDisplay {
   virtual void graphics_interop_activate() override;
   virtual void graphics_interop_deactivate() override;
 
+  virtual void clear() override;
+
  protected:
   virtual bool do_update_begin(const GPUDisplayParams &params,
                                int texture_width,
@@ -177,6 +181,9 @@ class BlenderGPUDisplay : public GPUDisplay {
      * and new data is to be uploaded to the GPU. */
     bool need_update = false;
 
+    /* Content of the texture is to be filled with zeroes. */
+    std::atomic<bool> need_clear = true;
+
     /* Dimensions of the texture in pixels. */
     int width = 0;
     int height = 0;
diff --git a/intern/cycles/device/cuda/graphics_interop.cpp b/intern/cycles/device/cuda/graphics_interop.cpp
index 0bd5ae31631..e8ca8b90eae 100644
--- a/intern/cycles/device/cuda/graphics_interop.cpp
+++ b/intern/cycles/device/cuda/graphics_interop.cpp
@@ -42,6 +42,8 @@ void CUDADeviceGraphicsInterop::set_destination(
 {
   const int64_t new_buffer_area = int64_t(destination.buffer_width) * destination.buffer_height;
 
+  need_clear_ = destination.need_clear;
+
   if (opengl_pbo_id_ == destination.opengl_pbo_id && buffer_area_ == new_buffer_area) {
     return;
   }
@@ -77,6 +79,13 @@ device_ptr CUDADeviceGraphicsInterop::map()
   cuda_device_assert(
       device_, cuGraphicsResourceGetMappedPointer(&cu_buffer, &bytes, cu_graphics_resource_));
 
+  if (need_clear_) {
+    cuda_device_assert(
+        device_, cuMemsetD8Async(static_cast<CUdeviceptr>(cu_buffer), 0, bytes, queue_->stream()));
+
+    need_clear_ = false;
+  }
+
   return static_cast<device_ptr>(cu_buffer);
 }
 
diff --git a/intern/cycles/device/cuda/graphics_interop.h b/intern/cycles/device/cuda/graphics_interop.h
index 447a364a86f..8a70c8aa71d 100644
--- a/intern/cycles/device/cuda/graphics_interop.h
+++ b/intern/cycles/device/cuda/graphics_interop.h
@@ -55,6 +55,9 @@ class CUDADeviceGraphicsInterop : public DeviceGraphicsInterop {
   /* Buffer area in pixels of the corresponding PBO. */
   int64_t buffer_area_ = 0;
 
+  /* The destination was requested to be cleared. */
+  bool need_clear_ = false;
+
   CUgraphicsResource cu_graphics_resource_ = nullptr;
 };
 
diff --git a/intern/cycles/device/device_graphics_interop.h b/intern/cycles/device/device_graphics_interop.h
index 28bbff10c43..671b1c189d7 100644
--- a/intern/cycles/device/device_graphics_interop.h
+++ b/intern/cycles/device/device_graphics_interop.h
@@ -30,6 +30,9 @@ class DeviceGraphicsInteropDestination {
 
   /* OpenGL pixel buffer object. */
   int opengl_pbo_id = 0;
+
+  /* Clear the entire destination before doing partial write to it. */
+  bool need_clear = false;
 };
 
 /* Device-side graphics interoperability support.
diff --git a/intern/cycles/integrator/path_trace.cpp b/intern/cycles/integrator/path_trace.cpp
index 6973a86daac..c69921c8394 100644
--- a/intern/cycles/integrator/path_trace.cpp
+++ b/intern/cycles/integrator/path_trace.cpp
@@ -511,6 +511,11 @@ void PathTrace::set_gpu_display(unique_ptr<GPUDisplay> gpu_display)
   gpu_display_ = move(gpu_display);
 }
 
+void PathTrace::clear_gpu_display()
+{
+  gpu_display_->clear();
+}
+
 void PathTrace::draw()
 {
   if (!gpu_display_) {
diff --git a/intern/cycles/integrator/path_trace.h b/intern/cycles/integrator/path_trace.h
index 3e92a819f31..ecc5204f2ec 100644
--- a/intern/cycles/integrator/path_trace.h
+++ b/intern/cycles/integrator/path_trace.h
@@ -99,6 +99,9 @@ class PathTrace {
   /* Set GPU display which takes care of drawing the render result. */
   void set_gpu_display(unique_ptr<GPUDisplay> gpu_display);
 
+  /* Clear the GPU display by filling it in with all zeroes. */
+  void clear_gpu_display();
+
   /* Perform drawing of the current state of the GPUDisplay. */
   void draw();
 
diff --git a/intern/cycles/render/gpu_display.h b/intern/cycles/render/gpu_display.h
index 9b5ae591455..cbe347895a1 100644
--- a/intern/cycles/render/gpu_display.h
+++ b/intern/cycles/render/gpu_display.h
@@ -158,6 +158,19 @@ class GPUDisplay {
    * Drawing.
    */
 
+  /* Clear the texture by filling it with all zeroes.
+   *
+   * This call might happen in parallel with draw, but can never happen in parallel with the
+   * update.
+   *
+   * The actual zero-ing can be deferred to a later moment. What is important is that after clear
+   * and before pixels update the drawing texture will be fully empty, and that partial update
+   * after clear will write new pixel values for an updating area, leaving everything else zeroed.
+   *
+   * If the GPU display supports graphics interoperability then the zeroing the display is to be
+   * delegated to the device via the `DeviceGraphicsInteropDestination`. */
+  virtual void clear() = 0;
+
   /* Draw the current state of the texture.
    *
    * Returns true if this call did draw an updated state of the texture. */
diff --git a/intern/cycles/render/session.cpp b/intern/cycles/render/session.cpp
index 41a6893b713..643e639149e 100644
--- a/intern/cycles/render/session.cpp
+++ b/intern/cycles/render/session.cpp
@@ -154,6 +154,8 @@ bool Session::ready_to_reset()
 
 void Session::run_main_render_loop()
 {
+  path_trace_->clear_gpu_display();
+
   while (true) {
     RenderWork render_work = run_update_for_next_iteration();