Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@bdero
Copy link
Member

@bdero bdero commented Jun 22, 2023

Fixes uncontrolled crash when kDecal is used (as seen on #43087).

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

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.

@bdero bdero changed the title [Impeller] Add validation forbidding SamplerAddressMode::kDecal on th… [Impeller] Add validation forbidding SamplerAddressMode::kDecal on the OpenGLES backend Jun 22, 2023
@bdero bdero requested review from jonahwilliams and zanderso June 22, 2023 16:30
@bdero bdero self-assigned this Jun 22, 2023
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if we attempt to bind a null Sampler?

if (descriptor.width_address_mode == SamplerAddressMode::kDecal ||
descriptor.height_address_mode == SamplerAddressMode::kDecal ||
descriptor.depth_address_mode == SamplerAddressMode::kDecal) {
VALIDATION_LOG << "SamplerAddressMode::kDecal is not supported by the "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A TODO linking to a GItHub issue would probably be helpful here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done & filed this issue: flutter/flutter#129358

@bdero
Copy link
Member Author

bdero commented Jun 22, 2023

What happens if we attempt to bind a null Sampler?

The command.BindResource call will early return and not add the sampler to the command, and then the GLES backend will fail to find the sampler slot. This triggers a chain of early returns that bubble up to an FML_CHECK that will fail.

@bdero bdero merged commit e58ebef into flutter:main Jun 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 22, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 22, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jun 22, 2023
…129378)

flutter/engine@703c9a1...ed477de

2023-06-22 [email protected] Roll ANGLE from bbcf54bcb738 to 4ed2d403a329 (7 revisions) (flutter/engine#43105)
2023-06-22 [email protected] Workaround a release blocker after libc++ change (flutter/engine#43091)
2023-06-22 [email protected] Roll Skia from 8168c802c391 to 09b36b8ce0db (1 revision) (flutter/engine#43102)
2023-06-22 [email protected] [Impeller] Reland: Correctly compute UVs in texture fill (flutter/engine#43093)
2023-06-22 [email protected] [Impeller] Add validation forbidding SamplerAddressMode::kDecal on the OpenGLES backend (flutter/engine#43094)
2023-06-22 [email protected] Use minor version, ignore patches for CodeQL (flutter/engine#43088)
2023-06-22 [email protected] Print a warning when a message channel is used on the wrong thread. (flutter/engine#42928)
2023-06-22 [email protected] Roll Skia from 3f3e1da4b7eb to 8168c802c391 (4 revisions) (flutter/engine#43096)
2023-06-22 [email protected] [Impeller] default sample count back to 1 (but configure to 4 in defaults). (flutter/engine#43089)
2023-06-22 [email protected] [web] Don't get break type from v8BreakIterator (flutter/engine#43053)
2023-06-22 [email protected] Roll dart to 3.1.0-239.0.dev (flutter/engine#43083)
2023-06-22 [email protected] Revert "[Impeller] dont use concurrent runner to decode images on Android." (flutter/engine#43061)
2023-06-22 [email protected] [Impeller] Add fence waiter trace event. (flutter/engine#43092)
2023-06-22 [email protected] [Impeller] remove Vulkan pipeline cache mutex. (flutter/engine#43085)
2023-06-22 [email protected] Revert "[Impeller] Correctly compute UVs in texture fill" (flutter/engine#43087)
2023-06-22 [email protected] Roll Fuchsia Linux SDK from 7EZeNE4aGd29VfDly... to tcVndpnH_jzGm5LsJ... (flutter/engine#43081)
2023-06-22 [email protected] Roll Skia from 117f57a53215 to 3f3e1da4b7eb (4 revisions) (flutter/engine#43080)
2023-06-22 [email protected] Roll ANGLE from 7658525166a4 to bbcf54bcb738 (1 revision) (flutter/engine#43079)
2023-06-22 [email protected] Roll Skia from 5013b651f8ec to 117f57a53215 (1 revision) (flutter/engine#43078)
2023-06-22 [email protected] Roll Fuchsia Mac SDK from QtQznuUmHMTyORqxJ... to Ylc35wOk0_j0NLzDv... (flutter/engine#43076)
2023-06-22 [email protected] Roll ANGLE from a2b3f9b64670 to 7658525166a4 (1 revision) (flutter/engine#43075)
2023-06-22 [email protected] Roll ANGLE from ac263582dda4 to a2b3f9b64670 (1 revision) (flutter/engine#43074)
2023-06-22 [email protected] Roll Skia from 71047dca9f77 to 5013b651f8ec (4 revisions) (flutter/engine#43073)
2023-06-22 [email protected] [Impeller] Correctly compute UVs in texture fill (flutter/engine#43028)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from 7EZeNE4aGd29 to tcVndpnH_jzG
  fuchsia/sdk/core/mac-amd64 from QtQznuUmHMTy to Ylc35wOk0_j0

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants