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
auto-submit merged 9 commits into from
Mar 27, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e3cbaa7
[Impeller] render empty filled paths without crashing.
Mar 27, 2024
5fbc953
clang tidy
Mar 27, 2024
fc62291
early return if there is no geometry.
Mar 27, 2024
e509527
Update tessellator_unittests.cc
Mar 27, 2024
53af2e8
Update tessellator_unittests.cc
Mar 27, 2024
182998d
fix ndebug condition.
Mar 27, 2024
fbd1b3a
++
Mar 27, 2024
67a668f
++
Mar 27, 2024
59bd52c
remove dchecks
Mar 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions impeller/entity/entity_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2812,6 +2812,31 @@ TEST_P(EntityTest, FailOnValidationError) {
"");
}

TEST_P(EntityTest, CanRenderEmptyPathsWithoutCrashing) {
Copy link
Member

@gaaclarke gaaclarke Mar 27, 2024

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

@jonahwilliams jonahwilliams Mar 27, 2024

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

@jonahwilliams jonahwilliams Mar 27, 2024

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

@jonahwilliams jonahwilliams Mar 27, 2024

Choose a reason for hiding this comment

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

Bug found!

PathBuilder builder = {};
builder.AddRect(Rect::MakeLTRB(0, 0, 0, 0));
Path path = builder.TakePath();

EXPECT_TRUE(path.GetBoundingBox()->IsEmpty());

auto geom = Geometry::MakeFillPath(path);

Entity entity;
RenderTarget target =
GetContentContext()->GetRenderTargetCache()->CreateOffscreen(
*GetContext(), {1, 1}, 1u);
testing::MockRenderPass render_pass(GetContext(), target);
auto position_result =
geom->GetPositionBuffer(*GetContentContext(), entity, render_pass);

auto uv_result =
geom->GetPositionUVBuffer(Rect::MakeLTRB(0, 0, 100, 100), Matrix(),
*GetContentContext(), entity, render_pass);

EXPECT_EQ(position_result.vertex_buffer.vertex_count, 0u);
EXPECT_EQ(uv_result.vertex_buffer.vertex_count, 0u);
}

} // namespace testing
} // namespace impeller

Expand Down
29 changes: 28 additions & 1 deletion impeller/entity/geometry/fill_path_geometry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include "fml/logging.h"
#include "impeller/core/formats.h"
#include "impeller/core/vertex_buffer.h"
#include "impeller/entity/contents/content_context.h"
#include "impeller/entity/geometry/geometry.h"

Expand All @@ -20,8 +21,21 @@ GeometryResult FillPathGeometry::GetPositionBuffer(
const Entity& entity,
RenderPass& pass) const {
auto& host_buffer = renderer.GetTransientsBuffer();
VertexBuffer vertex_buffer;

if (path_.GetBoundingBox()->IsEmpty()) {
return GeometryResult{
.type = PrimitiveType::kTriangle,
.vertex_buffer =
VertexBuffer{
.vertex_buffer = {},
.vertex_count = 0,
.index_type = IndexType::k16bit,
},
.transform = pass.GetOrthographicTransform() * entity.GetTransform(),
};
}

VertexBuffer vertex_buffer;
if constexpr (!ContentContext::kEnableStencilThenCover) {
if (!path_.IsConvex()) {
auto tesselation_result = renderer.GetTessellator()->Tessellate(
Expand Down Expand Up @@ -79,6 +93,19 @@ GeometryResult FillPathGeometry::GetPositionUVBuffer(
RenderPass& pass) const {
using VS = TextureFillVertexShader;

if (path_.GetBoundingBox()->IsEmpty()) {
return GeometryResult{
.type = PrimitiveType::kTriangle,
.vertex_buffer =
VertexBuffer{
.vertex_buffer = {},
.vertex_count = 0,
.index_type = IndexType::k16bit,
},
.transform = pass.GetOrthographicTransform() * entity.GetTransform(),
};
}

auto uv_transform =
texture_coverage.GetNormalizingTransform() * effect_transform;

Expand Down
3 changes: 3 additions & 0 deletions impeller/entity/geometry/geometry_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,14 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include <memory>
#include "flutter/testing/testing.h"
#include "impeller/entity/contents/content_context.h"
#include "impeller/entity/geometry/geometry.h"
#include "impeller/entity/geometry/stroke_path_geometry.h"
#include "impeller/geometry/geometry_asserts.h"
#include "impeller/geometry/path_builder.h"
#include "impeller/renderer/testing/mocks.h"

inline ::testing::AssertionResult SolidVerticesNear(
std::vector<impeller::SolidFillVertexShader::PerVertexData> a,
Expand Down
3 changes: 3 additions & 0 deletions impeller/tessellator/tessellator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ static int ToTessWindingRule(FillType fill_type) {
Tessellator::Result Tessellator::Tessellate(const Path& path,
Scalar tolerance,
const BuilderCallback& callback) {
FML_DCHECK(!path.GetBoundingBox()->IsEmpty());
if (!callback) {
return Result::kInputError;
}
Expand Down Expand Up @@ -178,7 +179,9 @@ Path::Polyline Tessellator::CreateTempPolyline(const Path& path,

std::vector<Point> Tessellator::TessellateConvex(const Path& path,
Scalar tolerance) {
FML_DCHECK(!path.GetBoundingBox()->IsEmpty());
FML_DCHECK(point_buffer_);

std::vector<Point> output;
point_buffer_->clear();
auto polyline =
Expand Down