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

[Impeller] simplify invert colors flag by supporting composed color filters. #46391

Merged
auto-submit merged 9 commits into from
Sep 29, 2023
Merged

[Impeller] simplify invert colors flag by supporting composed color filters. #46391

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ca24ee6
[Impeller] Simplify invert colors.
Sep 28, 2023
bc1dcab
++
Sep 28, 2023
baa08b7
rename to composed
Sep 29, 2023
ccf0ec5
++
Sep 29, 2023
4c15cf3
++
Sep 29, 2023
1b33b1f
++
Sep 29, 2023
dad6ac8
Update dl_dispatcher.cc
Sep 29, 2023
5ff8158
Update dl_dispatcher.cc
Sep 29, 2023
2ca5a11
Update dl_dispatcher.cc
Sep 29, 2023
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
36 changes: 34 additions & 2 deletions impeller/aiks/aiks_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "flutter/testing/testing.h"
#include "impeller/aiks/aiks_playground.h"
#include "impeller/aiks/canvas.h"
#include "impeller/aiks/color_filter.h"
#include "impeller/aiks/image.h"
#include "impeller/aiks/image_filter.h"
#include "impeller/aiks/paint_pass_delegate.h"
Expand Down Expand Up @@ -123,16 +124,47 @@ TEST_P(AiksTest, CanRenderImage) {
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

TEST_P(AiksTest, CanRenderInvertedImage) {
constexpr ColorMatrix kColorInversion = {.array = {

@dnfield dnfield Sep 29, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is now defined in at least three places - it'd be nice if it could live in some common header somewhere.

@jonahwilliams jonahwilliams Sep 29, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We could pull this logic all the way up too painting.dart but then we wont be able to test it with impeller yet. For the purposes of this test though it doesn't really matter what the filter is, just that it composes.

-1.0, 0, 0, 1.0, 0, //
0, -1.0, 0, 1.0, 0, //
0, 0, -1.0, 1.0, 0, //
1.0, 1.0, 1.0, 1.0, 0 //
}};

TEST_P(AiksTest, CanRenderMergedColorFilterImage) {
Canvas canvas;
Paint paint;
auto image = std::make_shared<Image>(CreateTextureForFixture("kalimba.jpg"));
paint.color = Color::Red();
paint.invert_colors = true;
paint.color_filter = std::make_shared<MergedColorFilter>(
ColorFilter::MakeMatrix(kColorInversion),
ColorFilter::MakeBlend(BlendMode::kColorDodge, Color::Aqua()));
canvas.DrawImage(image, Point::MakeXY(100.0, 100.0), paint);
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

TEST_P(AiksTest, CanRenderMergedColorFilter) {
Canvas canvas;
Paint paint;
paint.color = Color::Red();
paint.color_filter = std::make_shared<MergedColorFilter>(
ColorFilter::MakeMatrix(kColorInversion),
ColorFilter::MakeBlend(BlendMode::kColorDodge, Color::Aqua()));
canvas.DrawRect(Rect::MakeLTRB(0, 0, 100, 100), paint);
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

TEST_P(AiksTest, CanRenderMergedColorFilterDrawPaint) {
Canvas canvas;
Paint paint;
paint.color = Color::Red();
paint.color_filter = std::make_shared<MergedColorFilter>(
ColorFilter::MakeMatrix(kColorInversion),
ColorFilter::MakeBlend(BlendMode::kColorDodge, Color::Aqua()));
canvas.DrawPaint(paint);
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

namespace {
bool GenerateMipmap(const std::shared_ptr<Context>& context,
std::shared_ptr<Texture> texture,
Expand Down
1 change: 1 addition & 0 deletions impeller/aiks/canvas.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ void Canvas::DrawPath(const Path& path, const Paint& paint) {
}

void Canvas::DrawPaint(const Paint& paint) {
FML_LOG(ERROR) << "Draw Paint";
Entity entity;
entity.SetTransformation(GetCurrentTransformation());
entity.SetStencilDepth(GetStencilDepth());
Expand Down
36 changes: 36 additions & 0 deletions impeller/aiks/color_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@
// found in the LICENSE file.

#include "impeller/aiks/color_filter.h"

#include <utility>
#include "impeller/entity/contents/filters/color_filter_contents.h"
#include "impeller/entity/contents/filters/filter_contents.h"
#include "impeller/entity/contents/filters/inputs/filter_input.h"
#include "impeller/geometry/color.h"

Expand Down Expand Up @@ -142,4 +145,37 @@ std::shared_ptr<ColorFilter> LinearToSrgbColorFilter::Clone() const {
return std::make_shared<LinearToSrgbColorFilter>(*this);
}

/*******************************************************************************
******* MergedColorFilter
******************************************************************************/

MergedColorFilter::MergedColorFilter(std::shared_ptr<ColorFilter> outer,
std::shared_ptr<ColorFilter> inner)
: outer_(std::move(outer)), inner_(std::move(inner)) {}

MergedColorFilter::~MergedColorFilter() = default;

std::shared_ptr<ColorFilterContents> MergedColorFilter::WrapWithGPUColorFilter(
std::shared_ptr<FilterInput> input,
ColorFilterContents::AbsorbOpacity absorb_opacity) const {
std::shared_ptr<FilterContents> inner = inner_->WrapWithGPUColorFilter(
input, ColorFilterContents::AbsorbOpacity::kNo);
return outer_->WrapWithGPUColorFilter(FilterInput::Make(inner),
absorb_opacity);
}

// |ColorFilter|
ColorFilter::ColorFilterProc MergedColorFilter::GetCPUColorFilterProc() const {
auto inner_proc = inner_->GetCPUColorFilterProc();
auto outer_proc = outer_->GetCPUColorFilterProc();
return [inner_proc, outer_proc](Color color) {
return outer_proc(inner_proc(color));
};
}

// |ColorFilter|
std::shared_ptr<ColorFilter> MergedColorFilter::Clone() const {
return std::make_shared<MergedColorFilter>(outer_, inner_);
}

} // namespace impeller
24 changes: 24 additions & 0 deletions impeller/aiks/color_filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,4 +147,28 @@ class LinearToSrgbColorFilter final : public ColorFilter {
std::shared_ptr<ColorFilter> Clone() const override;
};

/// @brief Applies color filters as f(g(x)), where x is the input color.
class MergedColorFilter final : public ColorFilter {
public:
explicit MergedColorFilter(std::shared_ptr<ColorFilter> outer_,
std::shared_ptr<ColorFilter> inner_);

~MergedColorFilter() override;

// |ColorFilter|
std::shared_ptr<ColorFilterContents> WrapWithGPUColorFilter(
std::shared_ptr<FilterInput> input,
ColorFilterContents::AbsorbOpacity absorb_opacity) const override;

// |ColorFilter|
ColorFilterProc GetCPUColorFilterProc() const override;

// |ColorFilter|
std::shared_ptr<ColorFilter> Clone() const override;

private:
std::shared_ptr<ColorFilter> outer_;
std::shared_ptr<ColorFilter> inner_;
};

} // namespace impeller
24 changes: 0 additions & 24 deletions impeller/aiks/paint.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ std::shared_ptr<Contents> Paint::CreateContentsForGeometry(
std::shared_ptr<Contents> Paint::WithFilters(
std::shared_ptr<Contents> input) const {
input = WithColorFilter(input, ColorFilterContents::AbsorbOpacity::kYes);
input = WithInvertFilter(input);
auto image_filter =
WithImageFilter(input, Matrix(), Entity::RenderingMode::kDirect);
if (image_filter) {
Expand Down Expand Up @@ -121,33 +120,10 @@ std::shared_ptr<Contents> Paint::WithColorFilter(
if (input->ApplyColorFilter(color_filter->GetCPUColorFilterProc())) {
return input;
}

return color_filter->WrapWithGPUColorFilter(FilterInput::Make(input),
absorb_opacity);
}

/// A color matrix which inverts colors.
// clang-format off
constexpr ColorMatrix kColorInversion = {
.array = {
-1.0, 0, 0, 1.0, 0, //
0, -1.0, 0, 1.0, 0, //
0, 0, -1.0, 1.0, 0, //
1.0, 1.0, 1.0, 1.0, 0 //
}
};
// clang-format on

std::shared_ptr<Contents> Paint::WithInvertFilter(
std::shared_ptr<Contents> input) const {
if (!invert_colors) {
return input;
}

return ColorFilterContents::MakeColorMatrix(
{FilterInput::Make(std::move(input))}, kColorInversion);
}

std::shared_ptr<FilterContents> Paint::MaskBlurDescriptor::CreateMaskBlur(
std::shared_ptr<ColorSourceContents> color_source_contents,
const std::shared_ptr<ColorFilter>& color_filter) const {
Expand Down
4 changes: 0 additions & 4 deletions impeller/aiks/paint.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ struct Paint {
Scalar stroke_miter = 4.0;
Style style = Style::kFill;
BlendMode blend_mode = BlendMode::kSourceOver;
bool invert_colors = false;

std::shared_ptr<ImageFilter> image_filter;
std::shared_ptr<ColorFilter> color_filter;
Expand Down Expand Up @@ -108,9 +107,6 @@ struct Paint {
std::shared_ptr<Contents> input,
ColorFilterContents::AbsorbOpacity absorb_opacity =
ColorFilterContents::AbsorbOpacity::kNo) const;

std::shared_ptr<Contents> WithInvertFilter(
std::shared_ptr<Contents> input) const;
};

} // namespace impeller
36 changes: 26 additions & 10 deletions impeller/display_list/dl_dispatcher.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,28 +8,20 @@
#include <cstring>
#include <memory>
#include <optional>
#include <unordered_map>
#include <utility>
#include <vector>

#include "flutter/fml/logging.h"
#include "flutter/fml/trace_event.h"
#include "impeller/aiks/color_filter.h"
#include "impeller/core/formats.h"
#include "impeller/display_list/dl_image_impeller.h"
#include "impeller/display_list/dl_vertices_geometry.h"
#include "impeller/display_list/nine_patch_converter.h"
#include "impeller/display_list/skia_conversions.h"
#include "impeller/entity/contents/conical_gradient_contents.h"
#include "impeller/entity/contents/filters/filter_contents.h"
#include "impeller/entity/contents/filters/inputs/filter_input.h"
#include "impeller/entity/contents/linear_gradient_contents.h"
#include "impeller/entity/contents/radial_gradient_contents.h"
#include "impeller/entity/contents/runtime_effect_contents.h"
#include "impeller/entity/contents/sweep_gradient_contents.h"
#include "impeller/entity/contents/tiled_texture_contents.h"
#include "impeller/entity/entity.h"
#include "impeller/entity/geometry/geometry.h"
#include "impeller/geometry/path.h"
#include "impeller/geometry/path_builder.h"
#include "impeller/geometry/scalar.h"
Expand All @@ -41,6 +33,18 @@

namespace impeller {

/// A color matrix which inverts colors.
// clang-format off
constexpr ColorMatrix kColorInversion = {
.array = {
-1.0, 0, 0, 1.0, 0, //
0, -1.0, 0, 1.0, 0, //
0, 0, -1.0, 1.0, 0, //
1.0, 1.0, 1.0, 1.0, 0 //
}
};
// clang-format on

#define UNIMPLEMENTED \
FML_DLOG(ERROR) << "Unimplemented detail in " << __FUNCTION__;

Expand Down Expand Up @@ -484,12 +488,24 @@ static std::shared_ptr<ColorFilter> ToColorFilter(
// |flutter::DlOpReceiver|
void DlDispatcher::setColorFilter(const flutter::DlColorFilter* filter) {
// Needs https://github.com/flutter/flutter/issues/95434
paint_.color_filter = ToColorFilter(filter);
if (paint_.color_filter) {
auto color_filter = ToColorFilter(filter);
paint_.color_filter = std::make_shared<MergedColorFilter>(
std::move(paint_.color_filter), color_filter);
} else {
paint_.color_filter = ToColorFilter(filter);
}
}

// |flutter::DlOpReceiver|
void DlDispatcher::setInvertColors(bool invert) {
paint_.invert_colors = invert;
if (paint_.color_filter) {
auto invert_filter = ColorFilter::MakeMatrix(kColorInversion);
paint_.color_filter = std::make_shared<MergedColorFilter>(
invert_filter, std::move(paint_.color_filter));
} else {
paint_.color_filter = ColorFilter::MakeMatrix(kColorInversion);
}
Comment on lines +491 to +508

@jonahwilliams jonahwilliams Sep 29, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This sucks but there is no good answer to:

  1. Be resilient to dl dispatch order and
  2. Keep paint as a struct with no private fields.

@bdero bdero Sep 29, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To solve the composing problem, one possibility would be to add a "GetInverted()" to ColorFilter that returns a new composed filter with the color filter inversion appended.

@bdero bdero Sep 29, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(This is one of those cases where I would clone things liberally to prevent bugs -- the only reason these things are smart pointers is for dynamic dispatch, but like so many things in Impeller, these should really just be discriminated unions, but we currently have an aversion for those in C++)

@jonahwilliams jonahwilliams Sep 29, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, this is 2.5 which is "callers need to be aware of both fields" which I guess is just us, but not idea.

}

// |flutter::DlOpReceiver|
Expand Down