[HDRP] Physically based depth of field optimizations #5996

pmavridis · 2021-10-12T16:46:15Z

Purpose of this PR

The purpose of this PR is to optimize the performance of the physically based DOF implementation. In particular, DOF regions that are out-of-focus are now computed at lower resolution, while in-focus regions retain the full resolution. For many scenes this results in significant speedup, without any visible reduction in image quality.

It the following image:

green correspond to perfectly focused tiles (CoC < 1 pixel). These are not processed at all by DoF.
blue correspond to "fast tiles" (CoC > 2.5 pixels), these are processed at half res since they are "blurry" by definition.
red corresponds to tiles with both in-focus and out-of-focus regions. These are processed at full res, as before.

So the speedup comes from the green and blue regions.

And here is how the image looks (note that there areno discontinuities in tile boundaries)

The PR also switches to a faster noise function, since the previous blue noise one was causing a measurable perf drop.
(Using defines in the code we can switch between 4 different noise functions, I enabled the fastest one)

Testing status

I have run the graphics tests and updated the reference images related to this mode of DoF (everything looks ok)
I have tested the optimized pb DoF when moving around in the template scene.

Performance numbers

Using these settings:

With optimizations, total pp time: ~3.8 ms
Without the optimizations, total pp time: ~10.3ms

Measured with the Rendering Debugger while in play mode using 7 near/far samples (which is rather high):

Comments to reviewers

Many DoF-related functions where moved from the DoFGather shader to a new DoFGatherUtils file, in order to be shared with the new combine shader. The functions themselves did not change in a significant way.

github-actions · 2021-10-12T16:46:28Z

Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed.
Link to Yamato: https://unity-ci.cds.internal.unity3d.com/project/902/
Search for your PR branch using the search bar at the top, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

HDRP
/jobDefinition/.yamato%2Fall-hdrp.yml%23PR_HDRP_trunk
With changes to HDRP packages, you should also run
/jobDefinition/.yamato%2Fall-lightmapping.yml%23PR_Lightmapping_trunk

SRP Core
You could run ABV on your branch before merging your PR, but it will start A LOT of jobs. Please be responsible about it and run it only when you feel the PR is ready:
/jobDefinition/.yamato%252F_abv.yml%2523all_project_ci_trunk
Be aware that any modifications to the Core package impacts everyone in the Graphics repo so please discuss the PR with your lead.

Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure.

kecho

Let me know what you think of the 3 things.
Feel free to rebute them if you have already tried them, then ill approve the review.
Otherwise fantastic job!

kecho · 2021-10-22T14:57:14Z

...nity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.PostProcess.cs

+                cmd.DispatchCompute(cs, kernel, scaledWidth, scaledHeight, dofParameters.camera.viewCount);
+            }
+
+            if (isScaled)


Wouldnt this be true always? seams like you set parameters.resolution = DepthOfFieldResolution.Half;
Let me know if I missed something

True. It was useful for testing during development, but it should not be there in the final version, so removed.

kecho · 2021-10-22T15:15:35Z

com.unity.render-pipelines.high-definition/Runtime/PostProcessing/Shaders/DoFCombine.compute

+    PositionInputs posInputs = GetPositionInput(float2(dispatchThreadId.xy), _PostProcessScreenSize.zw, uint2(GROUP_RES, GROUP_RES));
+
+    SampleData centerSample;
+    centerSample.color = GetColorSample(posInputs.positionSS, 0);


Have you tried consuming the color exclusively inside the SLOW_INFOCUS_TILE case? If you do it here, color will likely always be sampled. regardless of tile size, which can be costly.
Its possible the compiler is doing the right thing, but i dont trust the compiler :D This could mean extra gains. LMK what you think.

For what I can see the sample is needed for anything but the FAST_DEFOCUS_TILE.

I expect the compiler to do a decent job here, but indeed worth being explicit.

Made this super-explicit in the last version ;)

kecho · 2021-10-22T15:17:00Z

com.unity.render-pipelines.high-definition/Runtime/PostProcessing/Shaders/DoFGather.compute

+    if (ResScale != 1.0 && tileClass != FAST_DEFOCUS_TILE)
+    {
+        // Early exit: these tiles will be computed at full res in the combine pass
+        _OutputTexture[COORD_TEXTURE2D_X(posInputs.positionSS)] = (CTYPE)centerSample.color;


Seams like this is never sampled because its gonna be a slow tile. Thus, means you dont have to write anything to it.
If you want it jsut for debugging purposes, id say just wrap this wtih a debug permutation.
Otherwise writting to this UAV will be expensive. If you dont write to it, this means more gains!

As discussed on slack, we need some extra work to do that, but yes, it's a good idea for future improvement.
I'm leaving it as it is for now.

FrancescoC-unity · 2021-10-25T09:13:51Z

com.unity.render-pipelines.high-definition/Runtime/PostProcessing/Shaders/DoFCombine.compute

+        break;
+    }
+
+    //DebugTiles(posInputs.positionSS, centerSample.CoC, output.xyz);


Do we want to keep this here for development or is it a leftover? If the former I'd add a comment on top

FrancescoC-unity · 2021-10-25T09:16:04Z

com.unity.render-pipelines.high-definition/Runtime/PostProcessing/Shaders/DoFCombine.compute

+    float2 uv = ClampAndScaleUVPostProcessTextureForPoint((posInputs.positionSS + 0.5) * _PostProcessScreenSize.zw);
+
+    CTYPE output = centerSample.color;
+    switch (GetTileClass(posInputs.positionSS, centerSample.CoC))


Is this compiled to a scalar branch?

I assume the tiles are aligned to group size? If so this evaluate always to the same thing for the whole warp. If that's the case and compiler doesn't do that explicitly, the LoadTileData could be scalar load and be faster

I am not sure there'd be a win here since 2 out of 3 cases are really simple, but worth keeping in mind if the cases become complex that this looks like a good candidate for indirect dispatching. (I wouldn't bother now for what it is)

FrancescoC-unity · 2021-10-25T09:19:35Z