Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
95a3554
first draft
Jun 16, 2024
587271b
Merge branch 'main' of github.com:flutter/engine into image_filter_sh…
Jun 20, 2024
a299234
++
Jun 20, 2024
ec1b101
format
Jun 20, 2024
2f9f72d
add basic doc comment.
Jun 20, 2024
1f206da
rename
Jun 20, 2024
0ec37a8
++
Jun 20, 2024
c3d1e0d
add ignore
Jun 20, 2024
c4ce95d
add unit testing.
Jun 24, 2024
b07ef04
fix include.
Jun 24, 2024
216b604
add aiks test.
Jun 24, 2024
e851999
++
Jun 24, 2024
217253b
++
Jun 24, 2024
6ead435
++
Jun 24, 2024
d848d66
8 uint 8s?
Jun 24, 2024
59cecc3
not sure why this works.
Jun 25, 2024
c786ea8
Merge branch 'main' of github.com:flutter/engine into image_filter_sh…
Jul 5, 2024
cda52b0
fixups for entity transform.
Jul 5, 2024
77e290b
++
Jul 5, 2024
5bb8824
Merge branch 'main' into image_filter_shader
Jul 8, 2024
4c55427
Merge branch 'main' into image_filter_shader
Jul 22, 2024
8355d5c
Merge branch 'main' of github.com:flutter/engine into image_filter_sh…
Aug 13, 2024
7772b8c
++
Aug 13, 2024
beac90a
++
Aug 13, 2024
eab80e0
++
Aug 13, 2024
2f9dc47
++
Aug 13, 2024
15378eb
++
Aug 13, 2024
fe023c7
dont reuse color source for filter.
Aug 15, 2024
65e8525
Update fragment_program.cc
Aug 15, 2024
2e00903
Merge branch 'main' into image_filter_shader
Aug 19, 2024
0684931
update for dl testing.
Aug 21, 2024
67cc655
Merge branch 'main' into image_filter_shader
Aug 21, 2024
eba4796
Update impeller_golden_tests_output.txt
Aug 21, 2024
6f824b0
Merge branch 'main' of github.com:flutter/engine into image_filter_sh…
Oct 28, 2024
7d90603
Merge branch 'image_filter_shader' of github.com:jonahwilliams/engine…
Oct 28, 2024
b6a8a23
++
Nov 2, 2024
d61c68c
++
Nov 2, 2024
bc0a436
start adding matrix support.
Nov 7, 2024
f592560
Merge branch 'main' of github.com:flutter/engine into image_filter_sh…
Nov 7, 2024
933a4ce
maybe its ok
Nov 7, 2024
2edae62
Merge branch 'main' into image_filter_shader
Nov 7, 2024
8b8b6ee
Revert "maybe its ok"
Nov 7, 2024
41f333c
Revert "start adding matrix support."
Nov 7, 2024
a876417
partial feedback
Nov 7, 2024
117ed3e
jazz up the shader.
Nov 7, 2024
eaf4ac4
Merge branch 'image_filter_shader' of github.com:jonahwilliams/engine…
Nov 7, 2024
f16d0c6
moar tests
Nov 7, 2024
9dd17af
straggler.
Nov 7, 2024
c6d3248
++
Nov 7, 2024
10e9e59
fix include.
Nov 7, 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
4 changes: 4 additions & 0 deletions ci/licenses_golden/licenses_flutter
Original file line number Diff line number Diff line change
Expand Up @@ -42987,6 +42987,8 @@ ORIGIN: ../../../flutter/impeller/entity/contents/filters/matrix_filter_contents
ORIGIN: ../../../flutter/impeller/entity/contents/filters/matrix_filter_contents.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/entity/contents/filters/morphology_filter_contents.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/entity/contents/filters/morphology_filter_contents.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/entity/contents/filters/runtime_effect_filter_contents.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/entity/contents/filters/runtime_effect_filter_contents.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/entity/contents/filters/srgb_to_linear_filter_contents.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/entity/contents/filters/srgb_to_linear_filter_contents.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/entity/contents/filters/yuv_to_rgb_filter_contents.cc + ../../../flutter/LICENSE
Expand Down Expand Up @@ -45850,6 +45852,8 @@ FILE: ../../../flutter/impeller/entity/contents/filters/matrix_filter_contents.c
FILE: ../../../flutter/impeller/entity/contents/filters/matrix_filter_contents.h
FILE: ../../../flutter/impeller/entity/contents/filters/morphology_filter_contents.cc
FILE: ../../../flutter/impeller/entity/contents/filters/morphology_filter_contents.h
FILE: ../../../flutter/impeller/entity/contents/filters/runtime_effect_filter_contents.cc
FILE: ../../../flutter/impeller/entity/contents/filters/runtime_effect_filter_contents.h
FILE: ../../../flutter/impeller/entity/contents/filters/srgb_to_linear_filter_contents.cc
FILE: ../../../flutter/impeller/entity/contents/filters/srgb_to_linear_filter_contents.h
FILE: ../../../flutter/impeller/entity/contents/filters/yuv_to_rgb_filter_contents.cc
Expand Down
3 changes: 2 additions & 1 deletion display_list/dl_builder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,8 @@ void DisplayListBuilder::onSetImageFilter(const DlImageFilter* filter) {
}
case DlImageFilterType::kCompose:
case DlImageFilterType::kLocalMatrix:
case DlImageFilterType::kColorFilter: {
case DlImageFilterType::kColorFilter:
case DlImageFilterType::kRuntimeEffect: {
Push<SetSharedImageFilterOp>(0, filter);
break;
}
Expand Down
104 changes: 104 additions & 0 deletions display_list/effects/dl_image_filter.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include <utility>

#include "display_list/effects/dl_color_source.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this stale now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need this for the samplers field on the DlRuntimeEffect filter

#include "flutter/display_list/dl_attributes.h"
#include "flutter/display_list/dl_sampling_options.h"
#include "flutter/display_list/dl_tile_mode.h"
Expand Down Expand Up @@ -34,6 +35,7 @@ enum class DlImageFilterType {
kCompose,
kColorFilter,
kLocalMatrix,
kRuntimeEffect,
};

class DlBlurImageFilter;
Expand All @@ -43,6 +45,7 @@ class DlMatrixImageFilter;
class DlLocalMatrixImageFilter;
class DlComposeImageFilter;
class DlColorFilterImageFilter;
class DlRuntimeEffectImageFilter;

class DlImageFilter : public DlAttribute<DlImageFilter, DlImageFilterType> {
public:
Expand Down Expand Up @@ -85,6 +88,12 @@ class DlImageFilter : public DlAttribute<DlImageFilter, DlImageFilterType> {
return nullptr;
}

// Return a DlRuntimeEffectImageFilter pointer to this object iff it is a
// DlRuntimeEffectImageFilter type of ImageFilter, otherwise return nullptr.
virtual const DlRuntimeEffectImageFilter* asRuntimeEffectFilter() const {
return nullptr;
}

// Return a boolean indicating whether the image filtering operation will
// modify transparent black. This is typically used to determine if applying
// the ImageFilter to a temporary saveLayer buffer will turn the surrounding
Expand Down Expand Up @@ -742,6 +751,101 @@ class DlLocalMatrixImageFilter final : public DlImageFilter {
std::shared_ptr<const DlImageFilter> image_filter_;
};

class DlRuntimeEffectImageFilter final : public DlImageFilter {
public:
explicit DlRuntimeEffectImageFilter(
Copy link
Member

Choose a reason for hiding this comment

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

nit: I wish these were in a .cc file. Recompilation time was killing me the other day.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are a few things I'd like to clean up here as well (and the other files in this directory). (There is a .cc file for each, we just don't use it much.)

  • Move more code to the .cc file as @gaaclarke is suggesting.
  • Deskiafying the geometry and other simple objects, naturally.
  • Disallow stack allocation and make them all shared pointers. It's great for unit tests where we don't care about performance, but causes extra copies in the primary code. We can still inline them into the DL records with private constructors, but if any DlOpReceiver code wants to save them "for later", it then needs to deal with reconstructing a shared version. :( Still, the amount of annoyance to deal with stack allocation vs shared_ptr feels like it should be cleaned up. For example, if you ever put it into a DlPaint, we have to re-share it when we should just keep a (shared) copy of the original.
  • Having them be shared pointers means we never construct a NOP filter because it would notice that it was NOP and return a null instead of a constructed object that has an identity (am I useful?) crisis.
  • Have different files for the different types??? This would explode the source files in this directory, but isolate the implementations. There would still be a main "dl_image_filter.h" that included the others as it needs to support the type and asFoo() mechanisms, but we wouldn't have one giant regurgitation of code in the main file.
  • Use more vectors instead of count/ptr pairs.

sk_sp<DlRuntimeEffect> runtime_effect,
std::vector<std::shared_ptr<DlColorSource>> samplers,
std::shared_ptr<std::vector<uint8_t>> uniform_data)
: runtime_effect_(std::move(runtime_effect)),
samplers_(std::move(samplers)),
uniform_data_(std::move(uniform_data)) {}

std::shared_ptr<DlImageFilter> shared() const override {
return std::make_shared<DlRuntimeEffectImageFilter>(
this->runtime_effect_, this->samplers_, this->uniform_data_);
}

static std::shared_ptr<DlImageFilter> Make(
sk_sp<DlRuntimeEffect> runtime_effect,
std::vector<std::shared_ptr<DlColorSource>> samplers,
std::shared_ptr<std::vector<uint8_t>> uniform_data) {
return std::make_shared<DlRuntimeEffectImageFilter>(
std::move(runtime_effect), std::move(samplers),
std::move(uniform_data));
}

DlImageFilterType type() const override {
return DlImageFilterType::kRuntimeEffect;
}
size_t size() const override { return sizeof(*this); }

bool modifies_transparent_black() const override { return false; }

SkRect* map_local_bounds(const SkRect& input_bounds,
SkRect& output_bounds) const override {
output_bounds = input_bounds;
return &output_bounds;
Copy link
Contributor

Choose a reason for hiding this comment

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

This gets tricky. Can they implement a custom image filter that does something like a blur - i.e. expands the bounds of the pixels?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We talked about this offline and discussed that there doesn't seem to be a good way to express all of the general purpose transformations that can be done with an image filter.

}

SkIRect* map_device_bounds(const SkIRect& input_bounds,
const SkMatrix& ctm,
SkIRect& output_bounds) const override {
output_bounds = input_bounds;
return &output_bounds;
}

SkIRect* get_input_device_bounds(const SkIRect& output_bounds,
const SkMatrix& ctm,
SkIRect& input_bounds) const override {
input_bounds = output_bounds;
return &input_bounds;
}

const DlRuntimeEffectImageFilter* asRuntimeEffectFilter() const override {
return this;
}

const sk_sp<DlRuntimeEffect> runtime_effect() const {
return runtime_effect_;
}

const std::vector<std::shared_ptr<DlColorSource>>& samplers() const {
return samplers_;
}

const std::shared_ptr<std::vector<uint8_t>>& uniform_data() const {
return uniform_data_;
}

protected:
bool equals_(const DlImageFilter& other) const override {
FML_DCHECK(other.type() == DlImageFilterType::kRuntimeEffect);
auto that = static_cast<const DlRuntimeEffectImageFilter*>(&other);
if (runtime_effect_ != that->runtime_effect_ ||
samplers_.size() != that->samplers().size() ||
uniform_data_->size() != that->uniform_data()->size()) {
return false;
}
for (auto i = 0u; i < samplers_.size(); i++) {
if (samplers_[i] != that->samplers()[i]) {
return false;
}
}
for (auto i = 0u; i < uniform_data_->size(); i++) {
if (uniform_data_->at(i) != that->uniform_data()->at(i)) {
return false;
}
}
return true;
}

private:
sk_sp<DlRuntimeEffect> runtime_effect_;
std::vector<std::shared_ptr<DlColorSource>> samplers_;
std::shared_ptr<std::vector<uint8_t>> uniform_data_;
};

} // namespace flutter

#endif // FLUTTER_DISPLAY_LIST_EFFECTS_DL_IMAGE_FILTER_H_
84 changes: 84 additions & 0 deletions display_list/effects/dl_image_filter_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
#include "flutter/display_list/utils/dl_comparable.h"
#include "gtest/gtest.h"

#include "include/core/SkMatrix.h"
#include "include/core/SkRect.h"
#include "third_party/skia/include/core/SkBlendMode.h"
#include "third_party/skia/include/core/SkColorFilter.h"
#include "third_party/skia/include/core/SkSamplingOptions.h"
Expand Down Expand Up @@ -823,5 +825,87 @@ TEST(DisplayListImageFilter, LocalImageFilterBounds) {
}
}

TEST(DisplayListImageFilter, RuntimeEffectEquality) {
DlRuntimeEffectImageFilter filter_a(nullptr, {nullptr},
std::make_shared<std::vector<uint8_t>>());
DlRuntimeEffectImageFilter filter_b(nullptr, {nullptr},
std::make_shared<std::vector<uint8_t>>());

EXPECT_EQ(filter_a, filter_b);

DlRuntimeEffectImageFilter filter_c(
nullptr, {nullptr}, std::make_shared<std::vector<uint8_t>>(1));

EXPECT_NE(filter_a, filter_c);
Copy link
Contributor

Choose a reason for hiding this comment

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

Also a version that is different based on samplers? I suppose it would be too hard to test equality if the shader was different - or do we have a RTE test fixture somewhere you could use for testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

TEST(DisplayListImageFilter, RuntimeEffectEqualityWithSamplers) {
auto image_a = std::make_shared<DlImageColorSource>(
nullptr, DlTileMode::kClamp, DlTileMode::kDecal);
auto image_b = std::make_shared<DlImageColorSource>(
nullptr, DlTileMode::kClamp, DlTileMode::kClamp);

DlRuntimeEffectImageFilter filter_a(nullptr, {nullptr, image_a},
std::make_shared<std::vector<uint8_t>>());
DlRuntimeEffectImageFilter filter_b(nullptr, {nullptr, image_a},
std::make_shared<std::vector<uint8_t>>());

EXPECT_EQ(filter_a, filter_b);

DlRuntimeEffectImageFilter filter_c(nullptr, {nullptr, image_b},
std::make_shared<std::vector<uint8_t>>());

EXPECT_NE(filter_a, filter_c);
}

TEST(DisplayListImageFilter, RuntimeEffectMapDeviceBounds) {
DlRuntimeEffectImageFilter filter_a(nullptr, {nullptr},
std::make_shared<std::vector<uint8_t>>());

auto input_bounds = SkIRect::MakeLTRB(0, 0, 100, 100);
SkMatrix identity;
SkIRect output_bounds;
SkIRect* result =
filter_a.map_device_bounds(input_bounds, identity, output_bounds);

EXPECT_NE(result, nullptr);
EXPECT_EQ(output_bounds, input_bounds);
}

TEST(DisplayListImageFilter, RuntimeEffectMapInputBounds) {
DlRuntimeEffectImageFilter filter_a(nullptr, {nullptr},
std::make_shared<std::vector<uint8_t>>());

auto input_bounds = SkRect::MakeLTRB(0, 0, 100, 100);

SkRect output_bounds;
SkRect* result = filter_a.map_local_bounds(input_bounds, output_bounds);

EXPECT_NE(result, nullptr);
EXPECT_EQ(output_bounds, input_bounds);
}

TEST(DisplayListImageFilter, RuntimeEffectGetInputDeviceBounds) {
DlRuntimeEffectImageFilter filter_a(nullptr, {nullptr},
std::make_shared<std::vector<uint8_t>>());

auto output_bounds = SkIRect::MakeLTRB(0, 0, 100, 100);

SkMatrix identity;
SkIRect input_bounds;
SkIRect* result =
filter_a.get_input_device_bounds(output_bounds, identity, input_bounds);

EXPECT_NE(result, nullptr);
EXPECT_EQ(output_bounds, input_bounds);
}

TEST(DisplayListImageFilter, RuntimeEffectModifiesTransparentBlack) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not an issue with this PR, but this engaged a more global thought in me...

This reminds me that the name is a little off because the shader might modify transparent black and that's OK. What this method really is asking/answering is "does it modify transparent black pixels everywhere on the entire plane regardless of whether they were really "involved" in the original operation" which is a question apropos mostly for a CF, but IF needs to answer as well in case it is wrapping a CF. But the name itself doesn't really capture the question being asked. Perhaps ...WithPrejudice() or something...

DlRuntimeEffectImageFilter filter_a(nullptr, {nullptr},
std::make_shared<std::vector<uint8_t>>());

EXPECT_FALSE(filter_a.modifies_transparent_black());
}

} // namespace testing
} // namespace flutter
3 changes: 3 additions & 0 deletions display_list/skia/dl_sk_conversions.cc
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,9 @@ sk_sp<SkImageFilter> ToSk(const DlImageFilter* filter) {
}
return skia_filter->makeWithLocalMatrix(lm_filter->matrix());
}
case DlImageFilterType::kRuntimeEffect:
// UNSUPPORTED.
return nullptr;
}
}

Expand Down
34 changes: 31 additions & 3 deletions impeller/display_list/aiks_dl_runtime_effect_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@

#include <memory>

#include "display_list/effects/dl_color_source.h"
#include "flutter/impeller/display_list/aiks_unittests.h"

#include "flutter/display_list/dl_builder.h"
#include "flutter/display_list/dl_paint.h"
#include "flutter/display_list/effects/dl_color_source.h"
#include "flutter/display_list/effects/dl_image_filter.h"
#include "flutter/display_list/effects/dl_runtime_effect.h"
#include "flutter/impeller/display_list/aiks_unittests.h"

#include "include/core/SkPath.h"
#include "include/core/SkRRect.h"
Expand Down Expand Up @@ -83,5 +84,32 @@ TEST_P(AiksTest, DrawPaintTransformsBounds) {
ASSERT_TRUE(OpenPlaygroundHere(builder.Build()));
}

TEST_P(AiksTest, CanRenderRuntimeEffectFilter) {
auto runtime_stages =
OpenAssetAsRuntimeStage("runtime_stage_filter_example.frag.iplr");

std::shared_ptr<RuntimeStage> runtime_stage =
runtime_stages[PlaygroundBackendToRuntimeStageBackend(GetBackend())];
ASSERT_TRUE(runtime_stage);
ASSERT_TRUE(runtime_stage->IsDirty());

std::vector<std::shared_ptr<DlColorSource>> sampler_inputs = {
nullptr,
};
auto uniform_data = std::make_shared<std::vector<uint8_t>>();
uniform_data->resize(sizeof(Vector2));

DlPaint paint;
paint.setColor(DlColor::kAqua());
paint.setImageFilter(std::make_shared<DlRuntimeEffectImageFilter>(
DlRuntimeEffect::MakeImpeller(runtime_stage), sampler_inputs,
uniform_data));

DisplayListBuilder builder;
builder.DrawRect(SkRect::MakeXYWH(0, 0, 400, 400), paint);

ASSERT_TRUE(OpenPlaygroundHere(builder.Build()));
}

} // namespace testing
} // namespace impeller
2 changes: 1 addition & 1 deletion impeller/display_list/dl_dispatcher.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#include <optional>
#include <vector>

#include "display_list/effects/dl_color_source.h"
#include "display_list/dl_sampling_options.h"
#include "display_list/effects/dl_image_filter.h"
#include "flutter/fml/logging.h"
#include "impeller/core/formats.h"
Expand Down
38 changes: 38 additions & 0 deletions impeller/display_list/image_filter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// found in the LICENSE file.

#include "impeller/display_list/image_filter.h"
#include "display_list/effects/dl_image_filter.h"
#include "fml/logging.h"
#include "impeller/display_list/color_filter.h"
#include "impeller/display_list/skia_conversions.h"
Expand Down Expand Up @@ -102,6 +103,43 @@ std::shared_ptr<FilterContents> WrapInput(const flutter::DlImageFilter* filter,
outer_dl_filter.get(),
FilterInput::Make(WrapInput(inner_dl_filter.get(), input)));
}
case flutter::DlImageFilterType::kRuntimeEffect: {
const flutter::DlRuntimeEffectImageFilter* runtime_filter =
filter->asRuntimeEffectFilter();
FML_DCHECK(runtime_filter);
std::shared_ptr<impeller::RuntimeStage> runtime_stage =
runtime_filter->runtime_effect()->runtime_stage();

std::vector<RuntimeEffectContents::TextureInput> texture_inputs;
size_t index = 0;
for (const std::shared_ptr<flutter::DlColorSource>& sampler :
runtime_filter->samplers()) {
if (index == 0 && sampler == nullptr) {
// Insert placeholder for filter.
texture_inputs.push_back(
{.sampler_descriptor = skia_conversions::ToSamplerDescriptor({}),
.texture = nullptr});
continue;
}
if (sampler == nullptr) {
return nullptr;
}
auto* image = sampler->asImage();
if (!image) {
return nullptr;
}
FML_DCHECK(image->image()->impeller_texture());
index++;
texture_inputs.push_back({
.sampler_descriptor =
skia_conversions::ToSamplerDescriptor(image->sampling()),
.texture = image->image()->impeller_texture(),
});
}
return FilterContents::MakeRuntimeEffect(input, std::move(runtime_stage),
runtime_filter->uniform_data(),
std::move(texture_inputs));
}
}
FML_UNREACHABLE();
}
Expand Down
2 changes: 2 additions & 0 deletions impeller/entity/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,8 @@ impeller_component("entity") {
"contents/filters/matrix_filter_contents.h",
"contents/filters/morphology_filter_contents.cc",
"contents/filters/morphology_filter_contents.h",
"contents/filters/runtime_effect_filter_contents.cc",
"contents/filters/runtime_effect_filter_contents.h",
"contents/filters/srgb_to_linear_filter_contents.cc",
"contents/filters/srgb_to_linear_filter_contents.h",
"contents/filters/yuv_to_rgb_filter_contents.cc",
Expand Down
Loading