Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[impeller] change input order in ColorFilterContents::MakeBlend #39038

Merged
merged 1 commit into from Jan 20, 2023
Merged

[impeller] change input order in ColorFilterContents::MakeBlend #39038

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions impeller/entity/contents/filters/blend_filter_contents.cc
View file
Open in desktop
Expand Up @@ -159,7 +159,7 @@ static std::optional<Snapshot> PipelineBlend(
using VS = BlendPipeline::VertexShader;
using FS = BlendPipeline::FragmentShader;

auto input_snapshot = inputs[0]->GetSnapshot(renderer, entity);
auto dst_snapshot = inputs[0]->GetSnapshot(renderer, entity);
Copy link
Member Author

Choose a reason for hiding this comment

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

No change here, just renaming a bit for clarity


ContentContext::SubpassCallback callback = [&](const ContentContext& renderer,
RenderPass& pass) {
Expand Down Expand Up @@ -213,7 +213,7 @@ static std::optional<Snapshot> PipelineBlend(
// Draw the first texture using kSource.
options.blend_mode = BlendMode::kSource;
cmd.pipeline = renderer.GetBlendPipeline(options);
if (!add_blend_command(input_snapshot)) {
if (!add_blend_command(dst_snapshot)) {
return true;
}

Expand All @@ -225,8 +225,8 @@ static std::optional<Snapshot> PipelineBlend(

for (auto texture_i = inputs.begin() + 1; texture_i < inputs.end();
texture_i++) {
auto input = texture_i->get()->GetSnapshot(renderer, entity);
if (!add_blend_command(input)) {
auto src_input = texture_i->get()->GetSnapshot(renderer, entity);
if (!add_blend_command(src_input)) {
return true;
}
}
Expand Down Expand Up @@ -264,7 +264,7 @@ static std::optional<Snapshot> PipelineBlend(
.transform = Matrix::MakeTranslation(coverage.origin),
.sampler_descriptor =
inputs[0]->GetSnapshot(renderer, entity)->sampler_descriptor,
.opacity = absorb_opacity ? 1.0f : input_snapshot->opacity};
.opacity = absorb_opacity ? 1.0f : dst_snapshot->opacity};
}

#define BLEND_CASE(mode) \
Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/contents/filters/color_filter_contents.cc
View file
Open in desktop
Expand Up @@ -37,7 +37,7 @@ std::shared_ptr<ColorFilterContents> ColorFilterContents::MakeBlend(
std::shared_ptr<BlendFilterContents> new_blend;
for (auto in_i = inputs.begin() + 1; in_i < inputs.end(); in_i++) {
new_blend = std::make_shared<BlendFilterContents>();
new_blend->SetInputs({*in_i, blend_input});
new_blend->SetInputs({blend_input, *in_i});
Copy link
Member Author

Choose a reason for hiding this comment

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

This was the bug. We start with input 0, and input 1, and but we shifted input 1 and input 0. This was worked around in entity_pass.cc.

Copy link
Member Author

Choose a reason for hiding this comment

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

(This re-arranging doesn't happen for pipeline blends which exit above. We could also change this code to not do the re-arranging for advanced blends with 2 inputs, but that has consistency implications

new_blend->SetBlendMode(blend_mode);
if (in_i < inputs.end() - 1 || foreground_color.has_value()) {
blend_input = FilterInput::Make(
Expand Down
1 change: 1 addition & 0 deletions impeller/entity/contents/filters/color_filter_contents.h
View file
Open in desktop
Expand Up @@ -10,6 +10,7 @@ namespace impeller {

class ColorFilterContents : public FilterContents {
public:
/// @brief the [inputs] are expected to be in the order of dst, src.
static std::shared_ptr<ColorFilterContents> MakeBlend(
BlendMode blend_mode,
FilterInput::Vector inputs,
Expand Down
4 changes: 2 additions & 2 deletions impeller/entity/entity_pass.cc
View file
Open in desktop
Expand Up @@ -531,9 +531,9 @@ bool EntityPass::OnRender(
}

FilterInput::Vector inputs = {
FilterInput::Make(result.entity.GetContents()),
FilterInput::Make(texture,
result.entity.GetTransformation().Invert())};
result.entity.GetTransformation().Invert()),
FilterInput::Make(result.entity.GetContents())};
auto contents =
ColorFilterContents::MakeBlend(result.entity.GetBlendMode(), inputs);
contents->SetCoverageCrop(result.entity.GetCoverage());
Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/entity_unittests.cc
View file
Open in desktop
Expand Up @@ -856,7 +856,7 @@ TEST_P(EntityTest, Filters) {

auto blend1 = ColorFilterContents::MakeBlend(
BlendMode::kScreen,
{fi_bridge, FilterInput::Make(blend0), fi_bridge, fi_bridge});
{FilterInput::Make(blend0), fi_bridge, fi_bridge, fi_bridge});
Copy link
Member Author

Choose a reason for hiding this comment

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

This changes the output of this test. I think that is what we want, the way the previous algorithm worked for 2+ inputs was given the following filter inputs:

N0 N1 N2 N3

First N1 becomes dst and N0 becomes src to produce F1
Then N2 becomes dst and F1 becomes src to produce F2
Then N3 becomes dst and F2 becomes src to produce F3

Now this is changed (and this output looks different), first

N0 becomes dst and N1 becomes src to produce F1
F1 becomes dst and N2 becomes src to produce F2
F2 becomes dst and N3 becomes src to produce F3.

I think the latter is more intuitive initially, but the former also sort of makes sense as a kind of polish notation (or reverse, I don't remember which). But of course this ends up with inconsistencies for the most common 2 input case since that works differently if you're just using the pipeline blends.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we could retain the old behavior if desired with a new constructor, since you can't really express one in terms of the other directly

Copy link
Member

Choose a reason for hiding this comment

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

Since we don't rely on more then 2 inputs anywhere right now (I think the filter graph has bigger dreams, so maybe someday we will), changing the behavior now SGTM. I agree the latter seems more intuitive.


Entity entity;
entity.SetTransformation(Matrix::MakeScale(GetContentScale()) *
Expand Down
1 change: 0 additions & 1 deletion impeller/entity/shaders/blending/advanced_blend.glsl
View file
Open in desktop
Expand Up @@ -46,5 +46,4 @@ void main() {
vec4 blended = vec4(Blend(dst.rgb, src.rgb), 1) * dst.a;

frag_color = mix(dst_sample, blended, src.a);
// frag_color = dst_sample;
}