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
flutter/engine@703c9a1...ed477de

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