Details

Reviewers

Brecht Van Lommel (brecht)
William Leeson (leesonw)

Commits

rBcaee36fcc127: Cycles X: Bring back tiles support

Summary

The meaning of tiles has shifted form being a performance tweak to
a memory saving technique (avoid having full-frame buffers stored
in memory during rendering).

This is an initial implementation which brings some crucial building
blocks, such as:

User interface.

The tiling is default to be "Use Auto Tile". In the current version the "auto" part is not implemented, but the idea is to only use tiles when needed.

Internal support for tile manager, render scheduler, path tracer.

Short-term plan is to replace Save Buffers with the new implementation.
In the a-bit-longer term it will also be used for resumable render.

Known limitations:

Cancelling render without adaptive sampling or sample count pass replaces missing tiles with black upon cancel. This is because the stored buffer is all 0-ed, and zero alpha channel means fully opaque pixel in the render buffers.

It will be solved by storing a meta-information for per-tile number of samples (which is also required for resumable render).

Denoising happens for both tile and final result. During rendering it is possible to see seams on tile borders.

It will be solved by introducing idea of tile overscan.

Tiles are not highlighted.

This requires changes in the highlight code on Blender side. It will be worked on separately.

Peak memory usage is not ideal. Need to somehow free up session memory before reading full-frame file. It will be worked on as a follow-up development.

The render result drawing should be done via GPUDisplay, and the pass memory in RenderResult is to be lazily allocated. There are spearate patches for that under review.

Diff Detail

Repository: rB Blender

Event Timeline

Sergey Sharybin (sergey) requested review of this revision.Aug 24 2021, 4:35 PM

Sergey Sharybin (sergey) created this revision.

Harbormaster completed remote builds in B16597: Diff 41057.Aug 24 2021, 4:35 PM

kursad k (kursadk) awarded a token.Aug 24 2021, 11:55 PM

Alaska (Alaska) awarded a token.Aug 25 2021, 4:03 AM

Solve assert in debug mode.

Need to do some initialization of buffer params before they are valid
0-sized parameters.

Harbormaster completed remote builds in B16615: Diff 41086.Aug 25 2021, 3:50 PM

Great work overall.

intern/cycles/integrator/path_trace.h
199	Not sure why this one gets the `_tile` postfix while other functions don't.
203	Would prefer a name like `write_tile_buffer_to_disk`. Since writing can mean multiple things, some indication that this is about writing to disk helps. Also we generally use "buffer" instead of "result" in Cycles.
204	I suggest `process_full_buffer_from_disk`.
intern/cycles/render/buffers.cpp
245 ↗	(On Diff #41086)	There are render passes which are written exactly once, like the depth pass. So for that `pass_info.use_filter` needs to be taken into account. Also not sure how this works with things like cryptomatte.
intern/cycles/render/tile.cpp
48	We'll need to generate a unique name here, otherwise multiple processes will overwrite each other's files.
151	We might as well join these in the constructor instead of storing both.
intern/cycles/render/tile.h
47	In general I think it would be more clear to use "to disk", "from disk", and "on disk" instead of "partial".

This revision now requires changes to proceed.Aug 26 2021, 2:54 PM

Updates to naming and file name generation.

Harbormaster completed remote builds in B16638: Diff 41128.Aug 26 2021, 4:38 PM

Updated with all points other than the passing scaling.

I'm checking on a more future-proof solution which will be needed for resumable render in the future.

intern/cycles/integrator/path_trace.h
199	Got confusing at some point during development, so had to disambiguate. But happy with the naming you've suggested in the review, so will remove the _tile suffix.
intern/cycles/render/buffers.cpp
245 ↗	(On Diff #41086)	Ugh, I forgot about those. Think we should work on a solution which allows to properly resume rendering after cancel. Is "just" we need to store per-tile number of samples in the metadata. I am currently looking into how to implement this in practice.

I decided to test this patch and found a issue.

I know this is a rare use case, but it appears that when a tile is small, and the GPU is used for rendering, and OIDN is active, then some of the tiles will appear black after denoising. It's a minor issue, the tiles have the correct data once the render finishes, they're just black during rendering.

Steps to reproduce:

Create a scene to render.
Change the tile size to something small (16 pixels worked for me)
Enable OIDN denoising for the final render.
Change to GPU compute. (Both CUDA and OptiX produce this issue for me)
Render the image.

Thanks for testing. I'll surely look into reasons why it is happening.

One thing though: in practice you want to keep tile size big. In the Cycles X tile is a memory optimization, not a performance thing: bigger tile better the chance of getting best performance.

In D12309#320279, @Sergey Sharybin (sergey) wrote:

One thing though: in practice you want to keep tile size big. In the Cycles X tile is a memory optimization, not a performance thing: bigger tile better the chance of getting best performance.

Indeed, however I was investigating whether or not the Cycles-X tiles could be used for performance in specific scenarios. Specifically in the Spring demo file with the performance regression noted on blender chat a while ago. I had a feeling that the performance regression was to do with the progressive rendering method of Cycles-X and was wondering if turning the tile size down to match standard Cycles would help resolve this issue. It did not. Although discussion of the performance regression seen in the Spring demo file is not for this patch so I will end the discussion related to that inside this patch here.

Sergey Sharybin (sergey) mentioned this in D12333: Fix missing tiles with GPU render and OIDN denoiser in Cycles X.Aug 27 2021, 11:51 AM

Sergey Sharybin (sergey) mentioned this in rB0c5c22f2ce4a: Fix missing tiles with GPU render and OIDN denoiser in Cycles X.Aug 31 2021, 3:22 PM

Removed bogus render buffer scaling function.

After some trail and error the easiest way to move forward seems to be
to enable sample count pass. This allows to properly read partial tile
results from file and use existing routines for pass accessing and
denoising.

Seems to be acceptable for now, since we do want to make adaptive
sampling default anyway, so is not much of memory penalty, and it does
simplify code a lot.

There are still things to be done on top of this patch, mainly to
only read full buffer after scene database is freed, and support
overscan for adaptive sampling/denoising. But, again, think is better
to look into those more focused after getting ground work reviewed.

Harbormaster completed remote builds in B16729: Diff 41286.Aug 31 2021, 5:11 PM

Update against latest Cycles X branch.

One thing I've realized which is not yet possible with the current design
is to delay reading full frame for until after all view layers are rendered.
But think it will come from a it of re-shuffling of the existing building
blocks.

Let me know if this is something you prefer to have in the first iteration
of review, or whether you'll find it easier to add "complexity" on top of
a reviewed simpler patch.

Harbormaster completed remote builds in B16754: Diff 41321.Sep 1 2021, 11:46 AM

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

I think it's fine to go ahead with this with the limitations, easier to commit and iterate on it further than dealing with these big patches.

intern/cycles/render/tile.cpp
38	Maybe use `cycles-tile-buffer` otherwise it sounds a bit like the file contains one tile.

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

Closed by commit rBcaee36fcc127: Cycles X: Bring back tiles support (authored by Sergey Sharybin (sergey)). · Explain WhySep 7 2021, 11:09 AM

This revision was automatically updated to reflect the committed changes.

Sergey Sharybin (sergey) added a commit: rBcaee36fcc127: Cycles X: Bring back tiles support.

silex (silex) awarded a token.Sep 15 2021, 10:13 PM

Cycles X: Bring back tiles support
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 41534

intern/cycles/blender/addon/properties.py

intern/cycles/blender/addon/ui.py

intern/cycles/blender/blender_session.cpp

intern/cycles/blender/blender_sync.cpp

intern/cycles/integrator/path_trace.h

intern/cycles/integrator/path_trace.cpp

intern/cycles/integrator/render_scheduler.h

intern/cycles/integrator/render_scheduler.cpp

intern/cycles/render/film.h

intern/cycles/render/film.cpp

intern/cycles/render/session.h

intern/cycles/render/session.cpp

intern/cycles/render/tile.h

intern/cycles/render/tile.cpp

intern/cycles/util/util_system.h

intern/cycles/util/util_system.cpp

Cycles X: Bring back tiles supportClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 41534

intern/cycles/blender/addon/properties.py

intern/cycles/blender/addon/ui.py

intern/cycles/blender/blender_session.cpp

intern/cycles/blender/blender_sync.cpp

intern/cycles/integrator/path_trace.h

intern/cycles/integrator/path_trace.cpp

intern/cycles/integrator/render_scheduler.h

intern/cycles/integrator/render_scheduler.cpp

intern/cycles/render/film.h

intern/cycles/render/film.cpp

intern/cycles/render/session.h

intern/cycles/render/session.cpp

intern/cycles/render/tile.h

intern/cycles/render/tile.cpp

intern/cycles/util/util_system.h

intern/cycles/util/util_system.cpp

Cycles X: Bring back tiles support
ClosedPublic
Actions

Revision Contents
Changeset List