[Android] choose 24 bit depth buffer.

[jonahwilliams]

Fixes flutter/flutter#158531

Make sure the onscreen framebuffer has a depth attachment that matches the depth24_s8 format expected.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

[gaaclarke]

nice!

[gaaclarke]

Wait, couldn't we have a golden test for this?

[gaaclarke]

I'd like a test for this, putting a request change in until we have that answered.

[jonahwilliams]

we do not have android devices in the engine repo. I can add a unit test that the config contains depth 24.

[gaaclarke]

we do not have android devices in the engine repo. I can add a unit test that the config contains depth 24.

Okay, I see how this worked. That value is just a preference for eglChooseConfig:

EGL_DEPTH_SIZE
Must be followed by a nonnegative integer that indicates the desired depth buffer size, in bits. The smallest depth buffers of at least the specified size is preferred. If the desired size is zero, frame buffer configurations with no depth buffer are preferred. The default value is zero.

Should we instead be specifying a smaller bit depth? Should we have a validation error if we get a frame buffer that doesn't have depth?

[gaaclarke]

A unit test would be nice. Since we can't get a golden test I think we should at least have a validation error if we don't get a depth buffer since we assume we have one in order for impeller to work.

[jonahwilliams]

Part of the problem with golden testing this particular change is that the EGL config is just a request. On the mali devices we requested 8 bit stencil and 0 depth, but Mali doesn't support separate depth+stencil so it gives a D24S8. This happens to match what we need to render exactly and so things just work.

Whereas the Adreno support separate depth and stencil, so when we requested S8 it only gives us S8. Then onscreen clipping appears to not work because of the wonderful world of GL.

So we'd need specific hardware hooked up. Even then...

[gaaclarke]

So we'd need specific hardware hooked up. Even then...

I mean we aren't even using the real opengles drivers. Potentially ANGLE or SwiftShader could duplicate the Adreno behavior. That's way out of scope though for this.

[auto-submit]

auto label is removed for flutter/engine/56553, due to This PR has not met approval requirements for merging. Changes were requested by {gaaclarke}, please make the needed changes and resubmit this PR.
The PR author is a member of flutter-hackers and needs 0 more review(s) in order to merge this PR.

• Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

[jonahwilliams]

@gaaclarke is this a test? does it run... somewhere?

[gaaclarke]

I don't have a good solution for testing this. The following things would work but are out of scope:

1. setting up ANGLE to duplicate Adreno behavior (maybe not possible)
2. setting up SwiftShader to duplicate Adreno behavior (probably possible, but we don't use SwiftShader for opengles golden tests today)
3. Mocking out egl (this is a massive gutting that isn't as valuable as a golden test)

Looks like we might have a good log for this anyways via IMPELLER_LOG_EGL_ERROR

[gaaclarke]

@gaaclarke is this a test? does it run... somewhere?

Not really a test since it isn't asserting that the changed code is correct =)