Bug 232071 - Hardware PCF filtering does not work on iOS 15 when using WebGL2
Summary: Hardware PCF filtering does not work on iOS 15 when using WebGL2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Kyle Piddington
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-10-21 02:41 PDT by Martin
Modified: 2021-11-30 13:35 PST (History)
8 users (show)

See Also:


Attachments
Patch (11.15 KB, patch)
2021-11-01 18:23 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (11.11 KB, patch)
2021-11-09 16:13 PST, Kyle Piddington
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (11.34 KB, patch)
2021-11-09 16:36 PST, Kyle Piddington
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin 2021-10-21 02:41:02 PDT
See https://github.com/playcanvas/engine/issues/3590#issuecomment-945994262

Hardware PCF filtering (compare sampling of depth shadow) does not seem to work on iOS 15.
It works on all other devices and platforms, including Safari 15 on MacOS.

Steps to repro:
http://playcanvas.github.io/#/graphics/shadow-cascades
in Controls, set Count to 1, resolution to around 512, filtering PCF3 (it's easier to click on sliders than slide them)
and notice how pixelated the shadows are on iOS 15 platform.
Comment 1 Radar WebKit Bug Importer 2021-10-21 14:49:03 PDT
<rdar://problem/84524233>
Comment 2 Kenneth Russell 2021-10-22 12:47:04 PDT
Only Apple engineers will be able to triage this effectively but is there any way you can provide a smaller test case?

Although - I think depth comparisons in the direct-to-Metal translator were a known missing feature and might have been implemented upstream. Kyle, do you remember? Searching anglebug.com I couldn't find recent CLs.
Comment 3 Kyle Piddington 2021-10-22 16:39:33 PDT
(In reply to Kenneth Russell from comment #2)
> Only Apple engineers will be able to triage this effectively but is there
> any way you can provide a smaller test case?
> 
> Although - I think depth comparisons in the direct-to-Metal translator were
> a known missing feature and might have been implemented upstream. Kyle, do
> you remember? Searching anglebug.com I couldn't find recent CLs.

I believe the CL you were thinking of was here: https://chromium-review.googlesource.com/c/angle/angle/+/3232818, which just added some workarounds for platforms that didn't correctly implement sample_compare for various options.

Martin, which iOS device are you seeing this regression on? Could you attach a screenshot so I could compare on my own devices?
Comment 4 Kyle Piddington 2021-10-22 16:50:06 PDT
Answered my own question by looking at the forums :)

Perhaps you can answer me a second question then: What pixel format are you using to back the depth buffer? 

on iOS, we have several formats that cannot be filtered on iOS that can on macOS. Are you perhaps using Depth32Float or D32S8 to back your depth buffer? Does changing that to Depth16 improve the issue?


https://developer.apple.com/metal/Metal-Feature-Set-Tables.pdf
Comment 5 Martin 2021-10-28 10:42:48 PDT
Yes, it uses Depth32Float format. I'll test out the 16bit format next week when back from holidays. Is there some way to test for the filtering support of a format?

Thanks!
Comment 6 Martin 2021-11-01 07:18:38 PDT
I've tested this on iPhone XR, OS 15.0.

Originally we used 32bit float depth texture:
    glFormat = gl.DEPTH_COMPONENT;
    glInternalFormat = gl.DEPTH_COMPONENT32F;
    glPixelType = gl.FLOAT;

I now tried 16 bit:
    glFormat = gl.DEPTH_COMPONENT;
    glInternalFormat = gl.DEPTH_COMPONENT16;
    glPixelType = gl.UNSIGNED_INT;

And also 24bit:
    glFormat = gl.DEPTH_COMPONENT;
    glInternalFormat = gl.DEPTH_COMPONENT24;
    glPixelType = gl.UNSIGNED_INT;

They all work as expected on MacOS, but I still get no filtering on iPhone XR.
I also tried Google Pixel 3A and all 3 versions work fine there.
Comment 7 Kyle Piddington 2021-11-01 09:24:34 PDT
Thanks for having a look.

I've found a related issue from the SPIRV-Cross team:
https://github.com/KhronosGroup/SPIRV-Cross/issues/533

It seems by inserting a constexpr sampler into our shadow sampler function, we're able to return PCF functionality to this shader. Right now, I'm double checking if this is a case of state initialization gone wrong, or if we've missed some restriction.
Comment 8 Kyle Piddington 2021-11-01 18:23:11 PDT
Created attachment 443049 [details]
Patch
Comment 9 EWS Watchlist 2021-11-01 18:24:21 PDT
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 10 Kimmo Kinnunen 2021-11-02 00:59:13 PDT
Comment on attachment 443049 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443049&action=review

Some comments that might come up later in the upstream.
I'm not saying if it's 100% required to fix the comments here to land this here now.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/DisplayMtl.mm:-941
> -

Would it be more in line with the project code structure to rename this to supports32BitFloatFiltering and then have the .json query this property?
then move maybe also the conditions `display->supportsMacGPUFamily(1) ||` somehow into this FeatureMtl trigger?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/DisplayMtl.mm:1086
> +    return [mMetalDevice supports32BitFloatFiltering];

I think your compile failure is the case where this one is a "property" name, where as below is a "getter method name".

it looks a bit odd to have one "supports" and the other "is...Supported".

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/DisplayMtl.mm:1090
> +bool DisplayMtl::supportsDepth24Stencil8PixelFormat() const

Would it be more in line with the project structure to add this as a FeaturesMtl, too?

Note: allowSeparatedDepthStencilBuffers is already a related feature query, so this one should be maybe added in such a way that it's clear that both are internally consistent.

FWIW:
If I understand correctly, the current DEPTH24_STENCIL8 emulation implementation is triggered based on `allowSeparatedDepthStencilBuffers`. 
It kind of makes sense today, but it appears funny after supportsDepth24Stencil8PixelFormat is introduced as this would be the more appropriate flag to check in many places.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/TextureMtl.mm:799
> +        !mFormat.getCaps().filterable)
>      {

now you can use `webkit-patch format -g HEAD` to get this formatted according to ANGLE rules.
Comment 11 Kyle Piddington 2021-11-09 16:13:37 PST
Created attachment 443747 [details]
Patch
Comment 12 Kyle Piddington 2021-11-09 16:36:21 PST
Created attachment 443754 [details]
Patch
Comment 13 Kenneth Russell 2021-11-10 11:12:33 PST
Surfacing a conversation we've had on Slack on this topic:

Depth textures in OpenGL ES 3.0 are actually specified as not being filterable; i.e. using GL_LINEAR filtering for the min/mag filter is supposed to make the texture non-renderable. See:

https://github.com/KhronosGroup/WebGL/issues/3353

Martin, is it specifically linear filtering of depth textures that your algorithm relies on? Or is it something else about how depth comparisons are being performed that might be different?
Comment 14 Kenneth Russell 2021-11-10 16:25:50 PST
More feedback based on chats. It seems likely that linear filtering is being allowed because the TEXTURE_COMPARE_MODE is set to COMPARE_REF_TO_TEXTURE. The depth texture would be considered incomplete specifically if TEXTURE_COMPARE_MODE were set to NONE.

https://github.com/KhronosGroup/WebGL/issues/3359

If the proposed patch is applied, what happens to Gregg's test from:
https://github.com/KhronosGroup/WebGL/issues/3353

specifically,
https://jsgist.org/?src=493218139726a53b7b3affbb636148e9

and the d16 linear case?

If that continues to be rejected as non-renderable - then I think this patch is OK. ANGLE must then be enforcing the renderability / filterability requirements at higher levels than the Metal backend.
Comment 15 Martin 2021-11-11 01:32:10 PST
Yes, the PCF depends on LINEAR filtering of the depth map. But as you said, this is only supported when COMPARE_REF_TO_TEXTURE is set - in which case the hardware should do 4 samples, compare them to passed in depth value, and return the interpolated result.

At the moment on iOS15 it seems it only does single sample, or does not interpolate 4 samples.

Other WebGL2 platforms give us this result (source is a very low res shadow map to see the points clearly)
https://user-images.githubusercontent.com/59932779/137777777-6ffa5fdb-5b43-4d27-9b45-6361be4871e3.png

but iOS15 gives us this: 
https://user-images.githubusercontent.com/59932779/137777731-62d53fe4-c274-43ea-b0b6-6d6c8cb5a462.png

These are the settings we use
https://user-images.githubusercontent.com/59932779/137777856-2ffae554-815e-40d0-baaf-21c252984b92.png
Comment 16 Martin 2021-11-11 01:35:22 PST
See the page 28 here: https://www.slideserve.com/myrrh/shadow-mapping-powerpoint-ppt-presentation

it seems like iOS15 is doing GL_NEAREST instead of GL_LINEAR.
Comment 17 Kyle Piddington 2021-11-20 09:37:10 PST
Using Gregg's test here: https://jsgist.org/?src=493218139726a53b7b3affbb636148e9
D16 and D32 Linear sampled filtering still correctly does not render, though sample_compare now correctly filters. I think this patch brings us into spec alignment.
Comment 18 Kenneth Russell 2021-11-22 00:36:07 PST
Comment on attachment 443754 [details]
Patch

Thanks for confirming that Gregg's sample renders correctly on iOS with this change. r+
Comment 19 EWS 2021-11-30 13:35:34 PST
Committed r286323 (244682@main): <https://commits.webkit.org/244682@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 443754 [details].