[Universal] MSAA store actions optimizations for forward renderer Drawobjects pass #4161

manuele-bonanno · 2021-04-13T08:14:23Z

Purpose of this PR

URP bandwidth usage is currently un-optimal on TBDR architectures.
This PR introduces a new "Store Actions" setting that allows us to improve bandwidth usage on certain passes.

When copying depth or color is enabled, the DrawObjects transparent pass will run as a separate pass after the DrawObjects opaque pass. With MSAA enabled, both opaque and transparent passes will StoreAndResolve color and store depth. These store actions are correct for the opaque pass (since its results will be used by copy color/copy depth and by the transparent pass) but a waste of bandwidth in the transparent pass case since, unless some extra passes which use its results were injected, the results of the transparent pass will not be used anymore in the frame.
With this optimization the transparent pass will now Resolve color and discard (DontCare) depth.

Applying this optimization to all possible scenarios, could potentially break any scenes were new passes are injected, as those could potentially try to use the results of the transparent pass, so we are giving multiple "Store Actions" options (in the URP Asset Advanced options).

Discard: discard results of passes which are not used later in the frame (safe in case no new passes that use those results are injected)
Store: always store, safest but unoptimal behaviour (same as current)
Auto: use the Discard behaviour if no external injected passes are detected, otherwise fallback to Store

On the low level side, we are adding Color store actions and depth store actions properties to ScriptableRenderPass: this will allow more explicit control of store actions for future optimizations (both using the RenderPass API path and the SetRenderTarget one)

Note:
to get fully optimized results on Metal, in terms of resolve store actions being applied correctly, this PR relies on some trunk fixes I made in this PR.

Testing status

Results when applying the Discard policy on iOS:

Opaque pass:

Transparent Pass:

jonuuukas

overall looks good, wrote down few suggestions for overall readability/usability

com.unity.render-pipelines.universal/Editor/UniversalRenderPipelineAssetEditor.cs

com.unity.render-pipelines.universal/Runtime/Passes/ScriptableRenderPass.cs

com.unity.render-pipelines.universal/Runtime/ScriptableRenderer.cs

com.unity.render-pipelines.universal/Runtime/UniversalRenderer.cs

…timal configurations

…ctions and not override them with Store

phi-lira

I'm not sure if this setting should be on the renderer or in the pipeline asset.
@TheoWong-pixel what do you think?

I think from API perspective I get concerned that we expose public API to configure store actions but not to configure load. It feels the API is imcomplete/only being added for this specific case, but my worry is that soon users might find use cases they want to optimize load actions as well. @hkr for awareness/comments.

Otherwise, PR seems mostly good to me, requesting changes and adding additional stakeholders.

phi-lira · 2021-04-15T11:34:27Z

com.unity.render-pipelines.universal/Editor/UniversalRenderPipelineAssetEditor.cs

            public static GUIContent mixedLightingSupportLabel = EditorGUIUtility.TrTextContent("Mixed Lighting", "Makes the render pipeline include mixed-lighting Shader Variants in the build.");
            public static GUIContent debugLevel = EditorGUIUtility.TrTextContent("Debug Level", "Controls the level of debug information generated by the render pipeline. When Profiling is selected, the pipeline provides detailed profiling tags.");
            public static GUIContent shaderVariantLogLevel = EditorGUIUtility.TrTextContent("Shader Variant Log Level", "Controls the level logging in of shader variants information is outputted when a build is performed. Information will appear in the Unity console when the build finishes.");
+            public static GUIContent storeActionsOptimizationText = EditorGUIUtility.TrTextContent("Store Actions", "Sets the store actions policy on mobile GPUs. 'Discard': discard as much as possible, 'Store': always store, 'Auto': Discard if no custom passes are injected, otherwise Store.");


should we say tile based GPUs instead of mobile?

usually load actions are implicitly configured based on what store actions we have. Maybe I could name this setting differently? "Bandwidth optimization policy" or something like that?

@phi-lira all code changes are done as discussed (except the ones we agreed shouldn't be changed).
I wait for documentation and UX reviewers feedback for changing UI/docs related stuff

com.unity.render-pipelines.universal/Runtime/Data/UniversalRenderPipelineAsset.cs

com.unity.render-pipelines.universal/Runtime/Passes/ScriptableRenderPass.cs

phi-lira · 2021-04-15T11:47:13Z

com.unity.render-pipelines.universal/Runtime/Passes/ScriptableRenderPass.cs

+        RenderBufferStoreAction[] m_ColorStoreActions = new RenderBufferStoreAction[] { RenderBufferStoreAction.Store };
+        RenderBufferStoreAction m_DepthStoreAction = RenderBufferStoreAction.Store;


I wonder why not backing the entire attachemnt setup with something like https://docs.unity3d.com/ScriptReference/Rendering.RenderTargetBinding.html

I also find it weird that we provide StoreActions but don't expose LoadAction. If we don't think in terms of API completeness I feel like very soon someone might ask for Load actions as well.

in engine load actions are inferred based on the store actions setup, and this change or every API implementation. The only proper way right now to set load/store actions without having to fight built-in logic is to use the RenderPass API.

So while exposing the load actions here is easy, I am not sure they will have much effect in terms of determining the final store action used by the device. So there would be a lot of work needed to allow that. If it is important can be done as a future task, but from my experience the engine does a good job in figuring out the load actions and the main problems were introduced by wrong stores, so not sure the effort to have an explicit Load action API is worth it (also considering RenderPasses will do that)

I agree having only stores exposed in the API makes it feel limited, but if we want load actions too then it might involve lot of extra work. Future PRs could address it if really needed. WDYT?

Ok. sounds good to me and fair justification.

phi-lira · 2021-04-15T11:48:50Z

com.unity.render-pipelines.universal/Runtime/ScriptableRenderer.cs

+                    if (passColorAttachment != m_ActiveColorAttachments[0] || passDepthAttachment != m_ActiveDepthAttachment || finalClearFlag != ClearFlag.None ||
+                        renderPass.colorStoreAction != m_ActiveColorStoreActions[0] || renderPass.depthStoreAction != m_ActiveDepthStoreAction)


Not for this PR, but maybe for the future we could store the current attachment setup with something like https://docs.unity3d.com/ScriptReference/Rendering.RenderTargetBinding.html then it would make code more readable to compare pass binding with active binding.

May this check introduce redundant render target switches? For example if the active store action is Store and the new action is DontCare then we still have one store in total + a render target switch that implies loads for other attachments.

This seems to be the only place where m_ActiveColorStoreActions is read, but we only read m_ActiveColorStoreActions[0]. Is that on purpose?

this is the non MRT path so there should be only one color attachment (the first check that was already there only checks for color[0]: passColorAttachment != m_ActiveColorAttachments[0])

In general this optimization PR is only for the forward renderer at the moment, so I didn't do anything in MRT/deferred branches

regarding redundant render target switches: I suppose that could happen potentially. It doesn't happen with the current optimizations I did, but if users want to customize further their store actions in some cases it could lead to unoptimal switches I guess. But not sure there is much we can do about it. If we expose more low level customization options this is one of the risks. more power->more responsibility?

I also noticed that in some cases the engine would still do extra target switches compared to URP SetRenderTarget calls. i.e. is opaque would Store depth, and transparent would discard.
Without the store actions comparison check:

Opaque SetRenderTarget is called: Opaque will store all the results

Transparent SetRenderTarget is NOT called (because of no actions check): engine will still load from Opaque, will store the all results (waste of bandwidth)

With this new check introduced the behaviour will be the same in terms of loads but now the transparent pass will discard/resolve everything, no stores. Thanks to the second URP SetRenderTarget call with optimized actions, now happening

phi-lira · 2021-04-15T11:50:21Z

com.unity.render-pipelines.universal/Runtime/ScriptableRenderer.cs

+            m_ActiveColorAttachments[0] = colorAttachment;
+            for (int i = 1; i < m_ActiveColorAttachments.Length; ++i)
+                m_ActiveColorAttachments[i] = 0;
+
+            m_ActiveColorStoreActions[0] = colorStoreAction;
+            m_ActiveDepthStoreAction = depthStoreAction;
+            for (int i = 1; i < m_ActiveColorStoreActions.Length; ++i)
+                m_ActiveColorStoreActions[i] = RenderBufferStoreAction.Store;


Hooking up on the previous idea, if we used RenderTargetBinding we could configure it here once.

com.unity.render-pipelines.universal/Runtime/UniversalRenderer.cs

phi-lira · 2021-04-15T11:56:34Z

com.unity.render-pipelines.universal/Runtime/ScriptableRenderer.cs

            m_ActiveRenderPassQueue.Clear();
+
+            m_StoreActionsOptimizationSetting = UniversalRenderPipeline.asset.storeActionsOptimization;
+            m_UseOptimizedStoreActions = m_StoreActionsOptimizationSetting != StoreActionsOptimization.Store;


StoreActionsOptimization.Store should also remove store optimization from other passes like post-processing imo.

not sure I understand what you mean?
Currently StoreActionsOptimization.Store will remove optimization from all the passes going through the new SetRenderTarget API that explicitely sets the store actions. So any passes using that new interface will be affected. Right now that is only the DrawOpaque and DrawTransparent passes, but it is extensible to other passes in the future including post processing

PostProcessingPass.cs has some optimizations to load and store with SetRenderTarget. F.ex, I think we never store depth after post but some users asked for it. I was asking if these settings should affect those optimizations we already have there.

yes that sounds like a good idea. This PR is focusing on the opaque/transparent passes on top of introducing the new API. But extending the optimization options to other passes in future PRs sounds trivial (should be a matter of making sure that the new explicit actions API is called by the postproc pass)

…by Rider..

phi-lira · 2021-04-22T12:52:23Z

@TheoWong-pixel Qualit Audit will only happen after cutoff and from what I understand this was in the hope to land before. Advice?

phi-lira · 2021-04-22T12:54:33Z

@TheoWong-pixel what is the advance menu?

TheoWong-pixel · 2021-04-22T13:31:20Z

The work on the advance menu is outside of this PR. It is more structured around the UI and how we show advanced settings to users. This should be handled in another PR later.

The location that @manuele-bonanno has placed this setting is fine. :)

aleksandrasdzikia

Looks good on mobile side. Tested URP Examples and Boat attack. Played around with settings, couldn't find any issues on visual correctness as well as performance. It looks that the feature is disabled by default which is expected. Ran on Mali and Adreno devices, had no issues.

erikabar

approving, testing was done by mobile QA

oleks-k

Looks good with the editions!

# Conflicts: # com.unity.render-pipelines.universal/CHANGELOG.md # com.unity.render-pipelines.universal/Runtime/ScriptableRenderer.cs # com.unity.render-pipelines.universal/Runtime/UniversalRenderer.cs

phi-lira · 2021-04-28T07:16:56Z

Failures also reproduced in master / XR related, known issue being fixed.

manuele-bonanno added 6 commits April 7, 2021 08:58

initial store and load action SRP support

b4b6edf

removed depth prepass related temporary changes

5c53798

removing commented out code

6233c18

removing unused variables

1a77378

Added Store Actions settings

03504e3

moved the code that checks for injected passes

a6f59ed

manuele-bonanno requested review from jonuuukas and phi-lira April 13, 2021 08:14

github-actions bot added SRP universal labels Apr 13, 2021

jonuuukas reviewed Apr 13, 2021

View reviewed changes

manuele-bonanno added 6 commits April 13, 2021 11:48

cleanup and addressing reviewer's feedback

af2ecba

Merge branch 'master' into urp/msaa_store_actions_optimization

1a9ce7f

moved the "Store Actions" setting in the Advanced options section

1ee51a6

further depth store action optimization for when MSAA is disabled

8e95a89

Added changelog entry

f2e7ec3

Added documentation

cc6422b

manuele-bonanno requested a review from oleks-k April 13, 2021 13:14

manuele-bonanno marked this pull request as ready for review April 13, 2021 13:14

manuele-bonanno requested review from a team as code owners April 13, 2021 13:14

github-actions bot added the testing label Apr 13, 2021

manuele-bonanno requested a review from a team April 13, 2021 13:15

manuele-bonanno added 3 commits April 14, 2021 10:28

handling the opaque pass depth store actions separately for a more op…

0b2d3a1

…timal configurations

Merge branch 'master' into urp/msaa_store_actions_optimization

701f703

if "Store Actions"=Store, make sure to preserve and StoreAndResolve a…

f07f748

…ctions and not override them with Store

phi-lira suggested changes Apr 15, 2021

View reviewed changes

phi-lira requested review from TheoWong-pixel, UnityPaul and hkr April 15, 2021 12:00

hkr self-requested a review April 19, 2021 14:27

hkr approved these changes Apr 19, 2021

View reviewed changes

aleksandrasdzikia requested review from aleksandrasdzikia and removed request for a team April 19, 2021 23:08

Merge branch 'master' into urp/msaa_store_actions_optimization

887eb25

jonuuukas approved these changes Apr 21, 2021

View reviewed changes

manuele-bonanno added 2 commits April 21, 2021 13:43

removed using System.ComponentModel.Design.Serialization; line added …

6b1315f

…by Rider..

removing files accidentally added by a previous merge

facbf42

phi-lira approved these changes Apr 22, 2021

View reviewed changes

aleksandrasdzikia approved these changes Apr 22, 2021

View reviewed changes

erikabar approved these changes Apr 23, 2021

View reviewed changes

UnityPaul requested review from unity-cchu and removed request for UnityPaul April 23, 2021 14:28

oleks-k added 2 commits April 26, 2021 13:53

Edited the __Store Actions__ description.

507291d

Edited the StoreActionsOptimization API descriptions.

71e9011

oleks-k approved these changes Apr 26, 2021

View reviewed changes

unity-cchu approved these changes Apr 26, 2021

View reviewed changes

manuele-bonanno and others added 4 commits April 26, 2021 14:49

Merge branch 'master' into urp/msaa_store_actions_optimization

b46f66a

# Conflicts: # com.unity.render-pipelines.universal/CHANGELOG.md # com.unity.render-pipelines.universal/Runtime/ScriptableRenderer.cs # com.unity.render-pipelines.universal/Runtime/UniversalRenderer.cs

Merge branch 'master' into urp/msaa_store_actions_optimization

23694e6

editor test fix

39b1394

Merge branch 'master' into urp/msaa_store_actions_optimization

497777f

phi-lira merged commit 1caf37b into master Apr 28, 2021

phi-lira deleted the urp/msaa_store_actions_optimization branch April 28, 2021 07:17

manuele-bonanno mentioned this pull request May 18, 2021

[URP] Render Pass path store actions optimization #4573

Merged

manuele-bonanno mentioned this pull request Jul 26, 2021

added store actions API docs #5221

Merged

thomas-zeng mentioned this pull request Jul 27, 2021

[10.x.x] Backport load/store optimization for forward renderer Drawobjects pass to 10.x #5243

Merged

		RenderBufferStoreAction[] m_ColorStoreActions = new RenderBufferStoreAction[] { RenderBufferStoreAction.Store };
		RenderBufferStoreAction m_DepthStoreAction = RenderBufferStoreAction.Store;

		if (passColorAttachment != m_ActiveColorAttachments[0] \|\| passDepthAttachment != m_ActiveDepthAttachment \|\| finalClearFlag != ClearFlag.None \|\|
		renderPass.colorStoreAction != m_ActiveColorStoreActions[0] \|\| renderPass.depthStoreAction != m_ActiveDepthStoreAction)

[Universal] MSAA store actions optimizations for forward renderer Drawobjects pass #4161

[Universal] MSAA store actions optimizations for forward renderer Drawobjects pass #4161

Uh oh!

Conversation

manuele-bonanno commented Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of this PR

Testing status

Uh oh!

jonuuukas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

phi-lira left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manuele-bonanno Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phi-lira commented Apr 22, 2021

Uh oh!

phi-lira commented Apr 22, 2021

Uh oh!

TheoWong-pixel commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aleksandrasdzikia left a comment

Choose a reason for hiding this comment

Uh oh!

erikabar left a comment

Choose a reason for hiding this comment

Uh oh!

oleks-k left a comment

Choose a reason for hiding this comment

Uh oh!

phi-lira commented Apr 28, 2021

Uh oh!

Reviewers

manuele-bonanno commented Apr 13, 2021 •

edited

Loading

manuele-bonanno Apr 16, 2021 •

edited

Loading

TheoWong-pixel commented Apr 22, 2021 •

edited

Loading