Details

Reviewers

Brecht Van Lommel (brecht)
Jeroen Bakker (jbakker)
Clément Foucault (fclem)

Commits

rB8d6a8c543c6b: Cycles X: Use GPUDisplay for non-interactive render

Summary

Cycles X: Use GPUDisplay for non-interactive render [WIP]

This is a part of hi-res image rendering. The goal of this step is to
make it so Cycles takes care of active pass drawing for non-interactive
render, similar to the viewport render. Some top-level overview of the
code side design changes are summarized in T90580.

Benefits and solutions to design limitations which this change brings:

Allows to lazily allocate render result on Blender side.

Solves bottleneck of data transfer and color management, which should solve issues like T52360.

Allows a render engine to take advantage of tiled rendering for memory saving by saving rendered tiles on disk and only keeping display buffer. Leaving display buffer management to an external engine allows the engine to implement logic needed for changing active display pass (which might involve reading from partial tile file).

Allows to see any pass during rendering.

Diff Detail

Repository: rB Blender

Event Timeline

Sergey Sharybin (sergey) requested review of this revision.Jul 27 2021, 3:32 PM

Sergey Sharybin (sergey) created this revision.

Harbormaster completed remote builds in B16066: Diff 40011.Jul 27 2021, 3:32 PM

Alaska (Alaska) awarded a token.Jul 28 2021, 1:10 PM

I would suggest to use the external_engine in drw_engines_enable_editors we could switch between them. drw_engines_enable_* might need some rearranging to do this nicely.

The image engine only draws the image (premul alpha); The overlay engine uses an alpha under to add the checkerboard (overlay_background.c) overlay_edit_uv.c draws the overlays (controls, masks etc.) Sorry for the bad name. will need to change that.

About the context that is tricky there are drivers that don't support sharing data between contexts, although they are getting old and we might want to reduce support for them (IIRC they dont support OpenGL4/vulkan where we want to move to). Currently external engines currently uses the main context as the external_engines runs on the main thread and don't create their own context. So jobs starts from different threads, but drawing still work on the main thread. But this means that an external engine might need to do additional checks to make sure that GLTextures are allocated/used when triggered via external_engine.c. Depending on the backend GLTextures could be created from existing data via the main thread.

@Clément Foucault (fclem) has mentioned looking to remove support for OpenGL3.3. There wasn't an actual reason why (except vulkan and better tested drivers/more mature featureset), but we should take this patch into account in the consideration.

In D12039#312642, @Jeroen Bakker (jbakker) wrote:

I would suggest to use the external_engine in drw_engines_enable_editors we could switch between them. drw_engines_enable_* might need some rearranging to do this nicely.

Makes sense to me as well.
Would need to resolve some assumptions about external_draw_scene_do() only used from viewport, but it can be done relaitvely easily.

The image engine only draws the image (premul alpha); The overlay engine uses an alpha under to add the checkerboard (overlay_background.c) overlay_edit_uv.c draws the overlays (controls, masks etc.) Sorry for the bad name. will need to change that.

Does the overlay engine care about who rendered the image? As in, is it expected to do the alpha-under with checkeboard if the image is drawn by an external engine?

About the context that is tricky there are drivers that don't support sharing data between contexts, although they are getting old and we might want to reduce support for them (IIRC they dont support OpenGL4/vulkan where we want to move to). Currently external engines currently uses the main context as the external_engines runs on the main thread and don't create their own context. So jobs starts from different threads, but drawing still work on the main thread. But this means that an external engine might need to do additional checks to make sure that GLTextures are allocated/used when triggered via external_engine.c. Depending on the backend GLTextures could be created from existing data via the main thread.

In Cycles X we use separate contexts to be able to update texture without synchronizing to the main thread. This is one of the things which enabled much more interactive update.
As per standard, the textures are supposed to be shareable, so if that's not the case, we should get drivers fixed (right? right? ;)

For the creating GLTextures from an existing data: this is something where we rely on CUDA Graphics Interop, which allows to create texture from a CUDA buffer, without transferring data back-n-forth. This is something what we have to keep supported.

On semi-related topic, we can't really rely on draw manager API: any render engine should be able to do same drawing code as we do in Cycles.

In D12039#313076, @Sergey Sharybin (sergey) wrote:

Does the overlay engine care about who rendered the image? As in, is it expected to do the alpha-under with checkeboard if the image is drawn by an external engine?

Overlay engine is independent (does not care). The same code is used to draw the background in the 3d view, node editor and image editor.

In Cycles X we use separate contexts to be able to update texture without synchronizing to the main thread. This is one of the things which enabled much more interactive update.
As per standard, the textures are supposed to be shareable, so if that's not the case, we should get drivers fixed (right? right? ;)

When bumping opengl version, we should get rid of them as well.

On semi-related topic, we can't really rely on draw manager API: any render engine should be able to do same drawing code as we do in Cycles.

Agree. external engines should not know how blender internally works. As for this specific ticket we should open external engines to drawing inside the image editor. Ideally this should be controllable from the ExternalEngine Python API.

I did an initial pass of the design in T90580.

One thing I'm struggling with is a clear separation of what happens where. More specifically: the image_engine is to do nothing in the case when external_engine will user RenderEngine::draw to draw the render result. It is almost like drw_engines_enable_editors need to somehow conditionally enable engines, or the engine/cache init to cross-check something.

Here is how I was trying to move RenderEngine::draw() to the external_engine: P2315. It does work during rendering, but regular image drawing is broken. Possibly because of the depth buffer blitting.

So, how can we fit all those puzzle pieces together in a nice way? :)

I think conditionally enabling engines makes sense. The 3D viewport also conditionally enables workbench, eevee or external engine in drw_engines_enable_from_engine

At least I don't foresee any particular issue with that. Not sure if we have to refresh/free anything when the engine changes, or if any data is automatically freed.

intern/cycles/blender/blender_gpu_display.h
144 ↗	(On Diff #40011)	sued -> used

Minor changes, mainly typos and some comments.
Updating to make an upcoming question more easily to share :)

Harbormaster completed remote builds in B16380: Diff 40629.Aug 12 2021, 12:08 PM

I think conditionally enabling engines makes sense.

Ok, this will avoid some entanglement in the engine code.

Now the biggest remaining unknown to me is how to make background overlay to work with the RenderEngine::draw().
To make things easier to see i've added GPU_matrix_scale_1f(0.5f); prior to draw_render_engine(draw_ctx->evil_C); (in this patch). Then enable transparent film in Cycles and render. This is the image i'm getting

. Note that the gray is coming from the space background (it is not a world color)
If I remove the DRW_draw_pass(psl->image_pass); then I don't see checkerboard at all.

Is it required to draw a pass for checkerboard to work? How can the external engine to be drawn in a pass?

See

P2323 Fix checkboard rendering on D12039

1	diff --git a/source/blender/draw/engines/image/image_engine.c b/source/blender/draw/engines/image/image_engine.c
2	index 9e970b7e897..5ff88fa2e56 100644
3	--- a/source/blender/draw/engines/image/image_engine.c
4	+++ b/source/blender/draw/engines/image/image_engine.c
5	@@ -37,6 +37,7 @@
6	#include "ED_image.h"
7
8	#include "GPU_batch.h"
9	+#include "GPU_debug.h"
10	#include "GPU_matrix.h"
11
12	#include "RE_engine.h"
13	@@ -463,7 +464,11 @@ static void IMAGE_draw_scene(void *ved)
14	GPU_matrix_projection_set(projection_matrix);
15	GPU_matrix_set(view_matrix);
16
17	+ GPU_framebuffer_bind(dfbl->color_only_fb);
18	+ GPU_debug_group_begin("External Engine");
19	draw_render_engine(draw_ctx->evil_C);
20	+ GPU_debug_group_end();
21	+ DRW_state_reset();
22
23	GPU_matrix_pop();
24	GPU_matrix_pop_projection();
25	jeroen@pop-os:~/blender-git/blender$ git status

to fix the checkboard rendering.

The issue is that the depth texture was re-written and the background shader couldn't decide to use the border color or the checker pattern. The fix attaches the color only buffer so the depth buffer cannot be changed by external engines.
The Draw state is reset afterwards we shouldn't trust the gpu state after calling external code.

The debug group allows better GPU debugging it groups the GPU calls that the external engine does.

@Jeroen Bakker (jbakker) Thanks! The snipped gave enough info to understand how to setup the drawing.

Started moving all external engine interaction to the external_engine.c.

NOTE: This is heavily WIP, not ready for the review. Just ensuring a working state is saved somewhere.

I'll cleanup the code and make it ready for review ASAP.

Harbormaster completed remote builds in B16415: Diff 40684.Aug 13 2021, 4:57 PM

Update the patch for the actual review. Will update description shortly.

Note that for the best behavior D12243 and D12246 are to be applied first.

Harbormaster completed remote builds in B16472: Diff 40784.Aug 17 2021, 3:26 PM

Sergey Sharybin (sergey) edited the summary of this revision. (Show Details)Aug 17 2021, 3:28 PM

Sergey Sharybin (sergey) retitled this revision from Cycles X: Use GPUDisplay for non-interactive render [WIP] to Cycles X: Use GPUDisplay for non-interactive render.Aug 17 2021, 3:32 PM

Added some feedback on the external engines. Would like to go over the details with @Clément Foucault (fclem) to make sure we are all on the same track. I notices some potential issues that might already be in master.

source/blender/draw/engines/external/external_engine.c
204	Additional speedup can be get when modify-ing `DRW_draw_render_loop_2d_ex#do_populate_loop`. I don't see this part of this patch, but would improve performance in heavier scenes.
304	Hmm... current structure would still allocate shader passes and shader groups. We could not register the external engine when the prerequisites have not met (draw_manager.c) or invoke this method in the populate_init and don't construct the passes/groups in this case. The second option is mostly how we solve this. Note: DRW_engine_external_use_for_image_editor could also be a good location to block the external engine registration.
391	external engines could alter the state of the GPU. Internal flags could be different than we expect. We might need a DRW_state_reset to make sure we can trust the internal and GPU state. @Clément Foucault (fclem) Is this actually an issue I don't see a guard for v3d, except after calling all the draw_engines.draw_scene. In the viewport we added fixes when using BGL render engines, but I don't see the same when render engines call GL directly. Would add the `GPU_bgl_end()` here also, for case users want to use BGL to do similar tricks.

Sergey Sharybin (sergey) added inline comments.Aug 17 2021, 4:36 PM

source/blender/draw/engines/external/external_engine.c
204	Seems that the optimization you're mentioning can be performed in the master branch by checking the image type. There is even a TODO there ;)
304	Can you please rephrase the comment? `DRW_engine_external_use_for_image_editor` will actually enforce use of `image_engine` if either scene's Render or render's engine is missing.
391	Am i understanding it correctly that I simply need to add `DRW_state_reset();` and `GPU_bgl_end();` at the end of this function?

Jeroen Bakker (jbakker) added inline comments.Aug 17 2021, 4:44 PM

source/blender/draw/engines/external/external_engine.c
391	GPU_bgl_end: yes, DRW_state_reset: not sure, need feedback from Clement.

User GPU_bgl_end() after engine draw, to restore possibly changed state.

Harbormaster completed remote builds in B16482: Diff 40798.Aug 17 2021, 5:22 PM

De-duplicate is-rendering engine check: use RE_ENGINE_RENDERING.

Harbormaster completed remote builds in B16617: Diff 41090.Aug 25 2021, 4:25 PM

Jeroen Bakker (jbakker) mentioned this in D12332: Draw Manager: Optimize populate loop..Aug 27 2021, 3:07 PM

Sergey Sharybin (sergey) mentioned this in D12370: Cycles X: Support GPUDisplay with tiled rendering.Sep 1 2021, 5:14 PM

Sergey Sharybin (sergey) mentioned this in rBb72c1e93247f: Cycles X: Support GPUDisplay with tiled rendering.Sep 2 2021, 12:36 PM

Update against latest cycles-x branch.

Are there any stoppers from landing this patch to the cycles-x branch?
Anything we should mark as a TODO to be resolved prior to merge to master?

Harbormaster completed remote builds in B16776: Diff 41367.Sep 2 2021, 2:52 PM

From my side this looks fine for the cycles-x branch.

This revision is now accepted and ready to land.Sep 2 2021, 5:51 PM

Looks good to me. Patch is quite straightforward.

source/blender/draw/engines/external/external_engine.c
372
382	I would suggest renaming the function to `external_image_space_matrix_set` to make it clear it sets the matrices and to not confuse it with `external_cache_init` (was my first thought).
391	Throw a `DRW_state_reset` in there for good measure. But this is being a little bit paranoid.

Closed by commit rB8d6a8c543c6b: Cycles X: Use GPUDisplay for non-interactive render (authored by Sergey Sharybin (sergey)). · Explain WhySep 7 2021, 10:57 AM

This revision was automatically updated to reflect the committed changes.

Sergey Sharybin (sergey) added a commit: rB8d6a8c543c6b: Cycles X: Use GPUDisplay for non-interactive render.

Cycles X: Use GPUDisplay for non-interactive render
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 41533

intern/cycles/blender/addon/init.py

intern/cycles/blender/addon/engine.py

intern/cycles/blender/blender_gpu_display.cpp

intern/cycles/blender/blender_python.cpp

intern/cycles/blender/blender_session.h

intern/cycles/blender/blender_session.cpp

intern/cycles/integrator/path_trace.cpp

source/blender/draw/engines/eevee/eevee_engine.c

source/blender/draw/engines/external/external_engine.h

source/blender/draw/engines/external/external_engine.c

source/blender/draw/engines/select/select_engine.c

source/blender/draw/engines/workbench/workbench_engine.c

source/blender/draw/intern/DRW_render.h

source/blender/draw/intern/draw_manager.c

source/blender/draw/intern/draw_manager_exec.c

source/blender/makesrna/intern/rna_render.c

source/blender/render/RE_engine.h

source/blender/render/intern/engine.c

source/blender/windowmanager/WM_api.h

source/blender/windowmanager/intern/wm_window.c

Cycles X: Use GPUDisplay for non-interactive renderClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 41533

intern/cycles/blender/addon/__init__.py

intern/cycles/blender/addon/engine.py

intern/cycles/blender/blender_gpu_display.cpp

intern/cycles/blender/blender_python.cpp

intern/cycles/blender/blender_session.h

intern/cycles/blender/blender_session.cpp

intern/cycles/integrator/path_trace.cpp

source/blender/draw/engines/eevee/eevee_engine.c

source/blender/draw/engines/external/external_engine.h

source/blender/draw/engines/external/external_engine.c

source/blender/draw/engines/select/select_engine.c

source/blender/draw/engines/workbench/workbench_engine.c

source/blender/draw/intern/DRW_render.h

source/blender/draw/intern/draw_manager.c

source/blender/draw/intern/draw_manager_exec.c

source/blender/makesrna/intern/rna_render.c

source/blender/render/RE_engine.h

source/blender/render/intern/engine.c

source/blender/windowmanager/WM_api.h

source/blender/windowmanager/intern/wm_window.c

Cycles X: Use GPUDisplay for non-interactive render
ClosedPublic
Actions

Revision Contents
Changeset List

intern/cycles/blender/addon/init.py