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

[Impeller] render empty filled paths without crashing. #51713

Merged
merged 9 commits into from
Mar 27, 2024
Merged

[Impeller] render empty filled paths without crashing. #51713

merged 9 commits into from
Mar 27, 2024

Conversation

@jonahwilliams
Copy link
Contributor

Fixes flutter/flutter#145823

If we try to render a solid filled empty size path, the subpath division code creates an obscene number of vertices that hits overflow errors. Just No-op instead.

gaaclarke
gaaclarke reviewed Mar 27, 2024
View reviewed changes
Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

Code lgtm, I think it may be worth while actually issuing a render. A golden would be pointless but you can make sure you get a non-null result.

impeller/entity/entity_unittests.cc Outdated
"");
}

TEST_P(EntityTest, CanRenderEmptyPathsWithoutCrashing) {
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we should follow through and do a full render? We may have just punted the failure to later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think that is a good idea, might as well make sure we don't crash somewhere else or hit a validation check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

--- Vulkan Debug Report  ----------------------------------------
|                Severity: Error
|                    Type: { Validation }
|                 ID Name: VUID-vkCmdDraw-None-04007
|               ID Number: -1719549157
|       Queue Breadcrumbs: [NONE]
|  CMD Buffer Breadcrumbs: Solid Fill
|         Related Objects: CommandBuffer [140163933725304] [Playground Command Buffer], Pipeline [2301285533516562930] [Pipeline SolidFill Pipeline V#4]
|                 Trigger: Validation Error: [ VUID-vkCmdDraw-None-04007 ] Object 0: handle = 0x7f7a757ae278, name = Playground Command Buffer, type = VK_OBJECT_TYPE_COMMAND_BUFFER; Object 1: handle = 0x1fefcf00000001f2, name = Pipeline SolidFill Pipeline V#4, type = VK_OBJECT_TYPE_PIPELINE; | MessageID = 0x9981c31b | vkCmdDraw: VkPipeline 0x1fefcf00000001f2[Pipeline SolidFill Pipeline V#4] expects that this Command Buffer's vertex binding Index 0 should be set via vkCmdBindVertexBuffers. This is because pVertexBindingDescriptions[0].binding value is 0. The Vulkan spec states: All vertex input bindings accessed via vertex input variables declared in the vertex shader entry point's interface must have either valid or VK_NULL_HANDLE buffers bound (https://vulkan.lunarg.com/doc/view/1.3.243.0/mac/1.3-extensions/vkspec.html#VUID-vkCmdDraw-None-04007)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bug found!

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 27, 2024
@auto-submit auto-submit bot merged commit 5d13a29 into flutter:main Mar 27, 2024
@jonahwilliams jonahwilliams deleted the fix_npe_tessellate branch March 27, 2024 22:34
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 27, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 27, 2024
@engine-flutter-autoroll
5d13a2982 [Impeller] render empty filled paths without crashing. (flu…
402b1f2 
…tter/engine#51713)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 28, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 28, 2024
@engine-flutter-autoroll
5d13a2982 [Impeller] render empty filled paths without crashing. (flu…
552ccff 
…tter/engine#51713)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Mar 28, 2024
@engine-flutter-autoroll
Roll Flutter Engine from c602abdbae16 to 922c7b133bc2 (7 revisions) (#…
aba1621 
…145877)

flutter/engine@c602abd...922c7b1

2024-03-28 [email protected] Roll Fuchsia Linux SDK from Lk8KBU-c97ROj-YHm... to uzI3wnbEGlZ_dtO0Z... (flutter/engine#51737)
2024-03-28 [email protected] Roll Skia from b6160ffc0b96 to 66241be7c81c (16 revisions) (flutter/engine#51733)
2024-03-28 [email protected] [Impeller] Reland: Use the scissor to limit all draws by clip coverage.  (flutter/engine#51731)
2024-03-27 [email protected] [Impeller] adds a `plus` advanced blend for f16 pixel formats (flutter/engine#51589)
2024-03-27 [email protected] Platform channel for predictive back in route transitions on android (flutter/engine#49093)
2024-03-27 [email protected] [Impeller] render empty filled paths without crashing. (flutter/engine#51713)
2024-03-27 [email protected] [Impeller] use optimal depth attachment, remove useless barrier. (flutter/engine#51723)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from Lk8KBU-c97RO to uzI3wnbEGlZ_

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://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@auto-submit auto-submit bot mentioned this pull request Mar 28, 2024
Merged
auto-submit bot added a commit to flutter/flutter that referenced this pull request Mar 28, 2024
@auto-submit
Reverts "Roll Flutter Engine from c602abdbae16 to 922c7b133bc2 (7 rev…
1940955 
…isions) (#145877)" (#145901)

Reverts: #145877
Initiated by: zanderso
Reason for reverting: #145899
Original PR Author: engine-flutter-autoroll

Reviewed By: {fluttergithubbot}

This change reverts the following previous change:

flutter/engine@c602abd...922c7b1

2024-03-28 [email protected] Roll Fuchsia Linux SDK from Lk8KBU-c97ROj-YHm... to uzI3wnbEGlZ_dtO0Z... (flutter/engine#51737)
2024-03-28 [email protected] Roll Skia from b6160ffc0b96 to 66241be7c81c (16 revisions) (flutter/engine#51733)
2024-03-28 [email protected] [Impeller] Reland: Use the scissor to limit all draws by clip coverage.  (flutter/engine#51731)
2024-03-27 [email protected] [Impeller] adds a `plus` advanced blend for f16 pixel formats (flutter/engine#51589)
2024-03-27 [email protected] Platform channel for predictive back in route transitions on android (flutter/engine#49093)
2024-03-27 [email protected] [Impeller] render empty filled paths without crashing. (flutter/engine#51713)
2024-03-27 [email protected] [Impeller] use optimal depth attachment, remove useless barrier. (flutter/engine#51723)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from Lk8KBU-c97RO to uzI3wnbEGlZ_

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://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 28, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 28, 2024
@engine-flutter-autoroll
5d13a2982 [Impeller] render empty filled paths without crashing. (flu…
feb7da2 
…tter/engine#51713)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Mar 28, 2024
@engine-flutter-autoroll
Manual roll Flutter Engine from c602abdbae16 to 9df2d3a0778e (10 revi…
66d1872 
…sions) (#145903)

Manual roll requested by [email protected]

flutter/engine@c602abd...9df2d3a

2024-03-28 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] adds a `plus` advanced blend for f16 pixel formats (#51589)" (flutter/engine#51741)
2024-03-28 [email protected] [Impeller] correct multisample resolve/store configuration for Vulkan. (flutter/engine#51740)
2024-03-28 [email protected] Remove Impeller/OpenGLES from CI branch for Android e2e tests. (flutter/engine#51734)
2024-03-28 [email protected] Roll Fuchsia Linux SDK from Lk8KBU-c97ROj-YHm... to uzI3wnbEGlZ_dtO0Z... (flutter/engine#51737)
2024-03-28 [email protected] Roll Skia from b6160ffc0b96 to 66241be7c81c (16 revisions) (flutter/engine#51733)
2024-03-28 [email protected] [Impeller] Reland: Use the scissor to limit all draws by clip coverage.  (flutter/engine#51731)
2024-03-27 [email protected] [Impeller] adds a `plus` advanced blend for f16 pixel formats (flutter/engine#51589)
2024-03-27 [email protected] Platform channel for predictive back in route transitions on android (flutter/engine#49093)
2024-03-27 [email protected] [Impeller] render empty filled paths without crashing. (flutter/engine#51713)
2024-03-27 [email protected] [Impeller] use optimal depth attachment, remove useless barrier. (flutter/engine#51723)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from Lk8KBU-c97RO to uzI3wnbEGlZ_

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://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@jonahwilliams jonahwilliams added the cp: stable cherry pick to the stable release candidate branch label Jun 5, 2024
@flutteractionsbot
Copy link

Failed to create CP due to merge conflicts.
You will need to create the PR manually. See the cherrypick wiki for more info.

@jonahwilliams
Copy link
Contributor Author

This PR is already in stable

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@gaaclarke gaaclarke gaaclarke left review comments

@matanlurey matanlurey matanlurey approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App cp: stable cherry pick to the stable release candidate branch e: impeller

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Impeller crash on iOS with Material FAB with non zero elevation

4 participants

@jonahwilliams @flutteractionsbot @matanlurey @gaaclarke