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

[Impeller] dont print format strings for blend filter and snapshots. #57105

Merged
auto-submit merged 6 commits into from
Dec 12, 2024
Merged

[Impeller] dont print format strings for blend filter and snapshots. #57105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7bf1104
[Impeller] dont print format strings for blend filter.
Dec 10, 2024
fa56f78
remove extra labeling.
Dec 10, 2024
95ead9c
++
Dec 10, 2024
ee561aa
++
Dec 11, 2024
6e9f5d1
string view everywhere.
Dec 12, 2024
abb6d7e
fix src/dst label order.
Dec 12, 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
2 changes: 1 addition & 1 deletion impeller/entity/contents/contents.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ std::optional<Snapshot> Contents::RenderToSnapshot(
const std::optional<SamplerDescriptor>& sampler_descriptor,
bool msaa_enabled,
int32_t mip_count,
const std::string& label) const {
std::string_view label) const {
auto coverage = GetCoverage(entity);
if (!coverage.has_value()) {
return std::nullopt;
Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/contents/contents.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class Contents {
const std::optional<SamplerDescriptor>& sampler_descriptor = std::nullopt,
bool msaa_enabled = true,
int32_t mip_count = 1,
const std::string& label = "Snapshot") const;
std::string_view label = "Snapshot") const;

//----------------------------------------------------------------------------
/// @brief Return the color source's intrinsic size, if available.
Expand Down
30 changes: 22 additions & 8 deletions impeller/entity/contents/filters/blend_filter_contents.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,24 @@

namespace impeller {

namespace {

#ifdef IMPELLER_DEBUG

#define _IMPELLER_BLEND_MODE_FILTER_NAME_LIST(blend_mode) \
"Blend Filter " #blend_mode,

static constexpr const char* kBlendModeFilterNames[] = {
IMPELLER_FOR_EACH_BLEND_MODE(_IMPELLER_BLEND_MODE_FILTER_NAME_LIST)};

const std::string_view BlendModeToFilterString(BlendMode blend_mode) {
return kBlendModeFilterNames[static_cast<std::underlying_type_t<BlendMode>>(
blend_mode)];
}
#endif // IMPELLER_DEBUG

} // namespace

std::optional<BlendMode> InvertPorterDuffBlend(BlendMode blend_mode) {
switch (blend_mode) {
case BlendMode::kClear:
Expand Down Expand Up @@ -174,8 +192,7 @@ static std::optional<Entity> AdvancedBlend(
std::invoke(pipeline_proc, renderer, options);

#ifdef IMPELLER_DEBUG
pass.SetCommandLabel(
SPrintF("Advanced Blend Filter (%s)", BlendModeToString(blend_mode)));
pass.SetCommandLabel(BlendModeToFilterString(blend_mode));
Copy link
Member

@gaaclarke gaaclarke Dec 11, 2024

Choose a reason for hiding this comment

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

👍

#endif // IMPELLER_DEBUG
pass.SetVertexBuffer(std::move(vtx_buffer));
pass.SetPipeline(pipeline);
Expand Down Expand Up @@ -297,8 +314,7 @@ std::optional<Entity> BlendFilterContents::CreateForegroundAdvancedBlend(
CreateVertexBuffer(vertices, renderer.GetTransientsBuffer());

#ifdef IMPELLER_DEBUG
pass.SetCommandLabel(SPrintF("Foreground Advanced Blend Filter (%s)",
BlendModeToString(blend_mode)));
pass.SetCommandLabel(BlendModeToFilterString(blend_mode));
#endif // IMPELLER_DEBUG
pass.SetVertexBuffer(std::move(vtx_buffer));
auto options = OptionsFromPassAndEntity(pass, entity);
Expand Down Expand Up @@ -450,8 +466,7 @@ std::optional<Entity> BlendFilterContents::CreateForegroundPorterDuffBlend(
CreateVertexBuffer(vertices, renderer.GetTransientsBuffer());

#ifdef IMPELLER_DEBUG
pass.SetCommandLabel(SPrintF("Foreground PorterDuff Blend Filter (%s)",
BlendModeToString(blend_mode)));
pass.SetCommandLabel(BlendModeToFilterString(blend_mode));
#endif // IMPELLER_DEBUG
pass.SetVertexBuffer(std::move(vtx_buffer));
auto options = OptionsFromPassAndEntity(pass, entity);
Expand Down Expand Up @@ -549,8 +564,7 @@ static std::optional<Entity> PipelineBlend(
auto& host_buffer = renderer.GetTransientsBuffer();

#ifdef IMPELLER_DEBUG
pass.SetCommandLabel(
SPrintF("Pipeline Blend Filter (%s)", BlendModeToString(blend_mode)));
pass.SetCommandLabel(BlendModeToFilterString(blend_mode));
#endif // IMPELLER_DEBUG
auto options = OptionsFromPass(pass);
options.primitive_type = PrimitiveType::kTriangleStrip;
Expand Down
5 changes: 3 additions & 2 deletions impeller/entity/contents/filters/filter_contents.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ std::optional<Snapshot> FilterContents::RenderToSnapshot(
const std::optional<SamplerDescriptor>& sampler_descriptor,
bool msaa_enabled,
int32_t mip_count,
const std::string& label) const {
std::string_view label) const {
// Resolve the render instruction (entity) from the filter and render it to a
// snapshot.
if (std::optional<Entity> result =
Expand All @@ -266,7 +266,8 @@ std::optional<Snapshot> FilterContents::RenderToSnapshot(
std::nullopt, // sampler_descriptor
true, // msaa_enabled
/*mip_count=*/mip_count,
label); // label
label // label
);
}

return std::nullopt;
Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/contents/filters/filter_contents.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ class FilterContents : public Contents {
const std::optional<SamplerDescriptor>& sampler_descriptor = std::nullopt,
bool msaa_enabled = true,
int32_t mip_count = 1,
const std::string& label = "Filter Snapshot") const override;
std::string_view label = "Filter Snapshot") const override;

/// @brief Determines the coverage of source pixels that will be needed
/// to produce results for the specified |output_limit| under the
Expand Down
20 changes: 9 additions & 11 deletions impeller/entity/contents/filters/inputs/contents_filter_input.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@
#include <optional>
#include <utility>

#include "impeller/base/strings.h"

namespace impeller {

ContentsFilterInput::ContentsFilterInput(std::shared_ptr<Contents> contents,
Expand All @@ -18,7 +16,7 @@ ContentsFilterInput::ContentsFilterInput(std::shared_ptr<Contents> contents,
ContentsFilterInput::~ContentsFilterInput() = default;

std::optional<Snapshot> ContentsFilterInput::GetSnapshot(
const std::string& label,
std::string_view label,
const ContentContext& renderer,
const Entity& entity,
std::optional<Rect> coverage_limit,
Expand All @@ -27,14 +25,14 @@ std::optional<Snapshot> ContentsFilterInput::GetSnapshot(
coverage_limit = entity.GetContents()->GetCoverageHint();
}
if (!snapshot_.has_value()) {
snapshot_ = contents_->RenderToSnapshot(
renderer, // renderer
entity, // entity
coverage_limit, // coverage_limit
std::nullopt, // sampler_descriptor
msaa_enabled_, // msaa_enabled
/*mip_count=*/mip_count,
SPrintF("Contents to %s Filter Snapshot", label.c_str())); // label
Copy link
Member

@gaaclarke gaaclarke Dec 11, 2024

Choose a reason for hiding this comment

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

Instead of removing this argument, can we just send the argument directly here. That way we'd remove the sprintf but keep valuable debugging information?

Copy link
Contributor Author

@jonahwilliams jonahwilliams Dec 11, 2024

Choose a reason for hiding this comment

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

It always gets sprintf'd with the existing texture label. I just don't think these are useful - you can already see the pipeline in the debugger.

and if we do find out we need them its easy to add back.

Copy link
Member

@gaaclarke gaaclarke Dec 11, 2024

Choose a reason for hiding this comment

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

These are printed out in validation logs too, you won't always have the opportunity to inspect the pipeline. Where ever it is getting sprintf's? We should be able to just take a string verbatim instead of trying to modify it. That's more useful than having no label. If we are trying to debug a customer's issue it won't be easy to add them back in.

Copy link
Contributor Author

@jonahwilliams jonahwilliams Dec 11, 2024

Choose a reason for hiding this comment

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

@gaaclarke do you use this feature? I don't . If it turns out to be useful, we can add it back.

Copy link
Member

@gaaclarke gaaclarke Dec 11, 2024

Choose a reason for hiding this comment

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

Yes, I've benefited from having debug labels when responding to a warning printed out from validation layers. We've had vulkan in a good state for a while I think we've forgotten how useful they can be. Respectfully, I think you are throwing out the baby with the bath water. The expensive part is the sprintf/malloc/free calls, not the actual labelling. I'd rather we throw those out first and remeasure.

Copy link
Contributor Author

@jonahwilliams jonahwilliams Dec 11, 2024

Choose a reason for hiding this comment

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

This is not a baby or bathwater. its a debug label. If we need it, we can add it back.

Copy link
Member

@gaaclarke gaaclarke Dec 11, 2024

Choose a reason for hiding this comment

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

Can we keep the string literals in the source code and conditionally use them? Then the compiler will just rip them out if they aren't used. They aren't going to be easy to bring back. Bringing them back would be recreating something we already had, it will be easier for us to just maintain them. Any patch will bit rot pretty fast.

Copy link
Member

@gaaclarke gaaclarke Dec 11, 2024

Choose a reason for hiding this comment

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

You PR that landed today basically did that, right? It makes it so naming textures is a noop, so can we just keep the literals in the code to make them easy to turn on?

snapshot_ = contents_->RenderToSnapshot(renderer, // renderer
entity, // entity
coverage_limit, // coverage_limit
std::nullopt, // sampler_descriptor
msaa_enabled_, // msaa_enabled
/*mip_count=*/mip_count, //
label //
);
}
return snapshot_;
}
Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/contents/filters/inputs/contents_filter_input.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ class ContentsFilterInput final : public FilterInput {
~ContentsFilterInput() override;

// |FilterInput|
std::optional<Snapshot> GetSnapshot(const std::string& label,
std::optional<Snapshot> GetSnapshot(std::string_view label,
const ContentContext& renderer,
const Entity& entity,
std::optional<Rect> coverage_limit,
Expand Down
17 changes: 8 additions & 9 deletions impeller/entity/contents/filters/inputs/filter_contents_filter_input.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,19 @@ FilterContentsFilterInput::FilterContentsFilterInput(
FilterContentsFilterInput::~FilterContentsFilterInput() = default;

std::optional<Snapshot> FilterContentsFilterInput::GetSnapshot(
const std::string& label,
std::string_view label,
const ContentContext& renderer,
const Entity& entity,
std::optional<Rect> coverage_limit,
int32_t mip_count) const {
if (!snapshot_.has_value()) {
snapshot_ = filter_->RenderToSnapshot(
renderer, // renderer
entity, // entity
coverage_limit, // coverage_limit
std::nullopt, // sampler_descriptor
true, // msaa_enabled
/*mip_count=*/mip_count,
SPrintF("Filter to %s Filter Snapshot", label.c_str())); // label
Copy link
Member

@gaaclarke gaaclarke Dec 11, 2024

Choose a reason for hiding this comment

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

Same here.

snapshot_ = filter_->RenderToSnapshot(renderer, // renderer
entity, // entity
coverage_limit, // coverage_limit
std::nullopt, // sampler_descriptor
true, // msaa_enabled
/*mip_count=*/mip_count, //
label); // label
}
return snapshot_;
}
Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/contents/filters/inputs/filter_contents_filter_input.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ class FilterContentsFilterInput final : public FilterInput {
~FilterContentsFilterInput() override;

// |FilterInput|
std::optional<Snapshot> GetSnapshot(const std::string& label,
std::optional<Snapshot> GetSnapshot(std::string_view label,
const ContentContext& renderer,
const Entity& entity,
std::optional<Rect> coverage_limit,
Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/contents/filters/inputs/filter_input.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class FilterInput {
static FilterInput::Vector Make(std::initializer_list<Variant> inputs);

virtual std::optional<Snapshot> GetSnapshot(
const std::string& label,
std::string_view label,
const ContentContext& renderer,
const Entity& entity,
std::optional<Rect> coverage_limit = std::nullopt,
Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/contents/filters/inputs/placeholder_filter_input.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ PlaceholderFilterInput::PlaceholderFilterInput(Rect coverage_rect)
PlaceholderFilterInput::~PlaceholderFilterInput() = default;

std::optional<Snapshot> PlaceholderFilterInput::GetSnapshot(
const std::string& label,
std::string_view label,
const ContentContext& renderer,
const Entity& entity,
std::optional<Rect> coverage_limit,
Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/contents/filters/inputs/placeholder_filter_input.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class PlaceholderFilterInput final : public FilterInput {
~PlaceholderFilterInput() override;

// |FilterInput|
std::optional<Snapshot> GetSnapshot(const std::string& label,
std::optional<Snapshot> GetSnapshot(std::string_view label,
const ContentContext& renderer,
const Entity& entity,
std::optional<Rect> coverage_limit,
Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/contents/filters/inputs/texture_filter_input.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ TextureFilterInput::TextureFilterInput(std::shared_ptr<Texture> texture,
TextureFilterInput::~TextureFilterInput() = default;

std::optional<Snapshot> TextureFilterInput::GetSnapshot(
const std::string& label,
std::string_view label,
const ContentContext& renderer,
const Entity& entity,
std::optional<Rect> coverage_limit,
Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/contents/filters/inputs/texture_filter_input.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class TextureFilterInput final : public FilterInput {
~TextureFilterInput() override;

// |FilterInput|
std::optional<Snapshot> GetSnapshot(const std::string& label,
std::optional<Snapshot> GetSnapshot(std::string_view label,
const ContentContext& renderer,
const Entity& entity,
std::optional<Rect> coverage_limit,
Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/contents/texture_contents.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ std::optional<Snapshot> TextureContents::RenderToSnapshot(
const std::optional<SamplerDescriptor>& sampler_descriptor,
bool msaa_enabled,
int32_t mip_count,
const std::string& label) const {
std::string_view label) const {
// Passthrough textures that have simple rectangle paths and complete source
// rects.
auto bounds = destination_rect_;
Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/contents/texture_contents.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ class TextureContents final : public Contents {
const std::optional<SamplerDescriptor>& sampler_descriptor = std::nullopt,
bool msaa_enabled = true,
int32_t mip_count = 1,
const std::string& label = "Texture Snapshot") const override;
std::string_view label = "Texture Snapshot") const override;

// |Contents|
bool Render(const ContentContext& renderer,
Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/contents/tiled_texture_contents.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ std::optional<Snapshot> TiledTextureContents::RenderToSnapshot(
const std::optional<SamplerDescriptor>& sampler_descriptor,
bool msaa_enabled,
int32_t mip_count,
const std::string& label) const {
std::string_view label) const {
std::optional<Rect> geometry_coverage = GetGeometry()->GetCoverage({});
if (GetInverseEffectTransform().IsIdentity() &&
GetGeometry()->IsAxisAlignedRect() &&
Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/contents/tiled_texture_contents.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ class TiledTextureContents final : public ColorSourceContents {
const std::optional<SamplerDescriptor>& sampler_descriptor = std::nullopt,
bool msaa_enabled = true,
int32_t mip_count = 1,
const std::string& label = "Tiled Texture Snapshot") const override;
std::string_view label = "Tiled Texture Snapshot") const override;

private:
std::shared_ptr<Texture> CreateFilterTexture(
Expand Down
Loading