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

[Impeller] Match Skia gradient clamping behavior (and document). #44825

Merged
merged 6 commits into from
Sep 27, 2023
Merged

[Impeller] Match Skia gradient clamping behavior (and document). #44825

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
e37122b
[Impeller] match Skia gradient clamping behavior (and document)
Aug 17, 2023
9a33cf4
clean up conversion utilities
Aug 18, 2023
2889fc0
fix doc comment.
Aug 18, 2023
1d06b5a
Merge branch 'main' of github.com:flutter/engine into match_skia_grad…
Sep 27, 2023
a2684bd
update docs
Sep 27, 2023
6efecf2
split up tests.
Sep 27, 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
32 changes: 4 additions & 28 deletions impeller/display_list/dl_dispatcher.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,30 +271,6 @@ static std::vector<Color> ToColors(const flutter::DlColor colors[], int count) {
return result;
}

// Convert display list colors + stops into impeller colors and stops, taking
// care to ensure that the stops always start with 0.0 and end with 1.0.
template <typename T>
static void ConvertStops(T* gradient,
std::vector<Color>* colors,
std::vector<float>* stops) {
FML_DCHECK(gradient->stop_count() >= 2);

auto* dl_colors = gradient->colors();
auto* dl_stops = gradient->stops();
if (dl_stops[0] != 0.0) {
colors->emplace_back(skia_conversions::ToColor(dl_colors[0]));
stops->emplace_back(0);
}
for (auto i = 0; i < gradient->stop_count(); i++) {
colors->emplace_back(skia_conversions::ToColor(dl_colors[i]));
stops->emplace_back(dl_stops[i]);
}
if (stops->back() != 1.0) {
colors->emplace_back(colors->back());
stops->emplace_back(1.0);
}
}

static std::optional<ColorSource::Type> ToColorSourceType(
flutter::DlColorSourceType type) {
switch (type) {
Expand Down Expand Up @@ -351,7 +327,7 @@ void DlDispatcher::setColorSource(const flutter::DlColorSource* source) {
auto end_point = skia_conversions::ToPoint(linear->end_point());
std::vector<Color> colors;
std::vector<float> stops;
ConvertStops(linear, &colors, &stops);
skia_conversions::ConvertStops(linear, colors, stops);

auto tile_mode = ToTileMode(linear->tile_mode());
auto matrix = ToMatrix(linear->matrix());
Expand All @@ -372,7 +348,7 @@ void DlDispatcher::setColorSource(const flutter::DlColorSource* source) {
SkScalar focus_radius = conical_gradient->start_radius();
std::vector<Color> colors;
std::vector<float> stops;
ConvertStops(conical_gradient, &colors, &stops);
skia_conversions::ConvertStops(conical_gradient, colors, stops);

auto tile_mode = ToTileMode(conical_gradient->tile_mode());
auto matrix = ToMatrix(conical_gradient->matrix());
Expand All @@ -390,7 +366,7 @@ void DlDispatcher::setColorSource(const flutter::DlColorSource* source) {
auto radius = radialGradient->radius();
std::vector<Color> colors;
std::vector<float> stops;
ConvertStops(radialGradient, &colors, &stops);
skia_conversions::ConvertStops(radialGradient, colors, stops);

auto tile_mode = ToTileMode(radialGradient->tile_mode());
auto matrix = ToMatrix(radialGradient->matrix());
Expand All @@ -409,7 +385,7 @@ void DlDispatcher::setColorSource(const flutter::DlColorSource* source) {
auto end_angle = Degrees(sweepGradient->end());
std::vector<Color> colors;
std::vector<float> stops;
ConvertStops(sweepGradient, &colors, &stops);
skia_conversions::ConvertStops(sweepGradient, colors, stops);

auto tile_mode = ToTileMode(sweepGradient->tile_mode());
auto matrix = ToMatrix(sweepGradient->matrix());
Expand Down
24 changes: 24 additions & 0 deletions impeller/display_list/skia_conversions.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,5 +189,29 @@ std::optional<impeller::PixelFormat> ToPixelFormat(SkColorType type) {
return std::nullopt;
}

void ConvertStops(const flutter::DlGradientColorSourceBase* gradient,
std::vector<Color>& colors,
std::vector<float>& stops) {
FML_DCHECK(gradient->stop_count() >= 2);

auto* dl_colors = gradient->colors();
auto* dl_stops = gradient->stops();
if (dl_stops[0] != 0.0) {
colors.emplace_back(skia_conversions::ToColor(dl_colors[0]));
stops.emplace_back(0);
}
for (auto i = 0; i < gradient->stop_count(); i++) {
colors.emplace_back(skia_conversions::ToColor(dl_colors[i]));
stops.emplace_back(std::clamp(dl_stops[i], 0.0f, 1.0f));
}
if (dl_stops[gradient->stop_count() - 1] != 1.0) {
colors.emplace_back(colors.back());
stops.emplace_back(1.0);
}
for (auto i = 1; i < gradient->stop_count(); i++) {
stops[i] = std::clamp(stops[i], stops[i - 1], stops[i]);
}
}

} // namespace skia_conversions
} // namespace impeller
19 changes: 19 additions & 0 deletions impeller/display_list/skia_conversions.h
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this approach generally and the docstring LGTM!

One question though, does that mean we could enforce (with DCHECKs?) deeper in the stack that stops/colors are valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not, because we've never enforced this so adding more enforcement is breaking.

Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#pragma once

#include "display_list/dl_color.h"
#include "display_list/effects/dl_color_source.h"
#include "impeller/core/formats.h"
#include "impeller/geometry/color.h"
#include "impeller/geometry/path.h"
Expand Down Expand Up @@ -47,5 +48,23 @@ Path PathDataFromTextBlob(const sk_sp<SkTextBlob>& blob,

std::optional<impeller::PixelFormat> ToPixelFormat(SkColorType type);

/// @brief Convert display list colors + stops into impeller colors and stops,
/// taking care to ensure that the stops monotonically increase from 0.0 to 1.0.
///
/// The general process is:
/// * Ensure that the first gradient stop value is 0.0. If not, insert a new
/// stop with a value of 0.0 and use the first gradient color as this new
/// stops color.
/// * Ensure the last gradient stop value is 1.0. If not, insert a new stop
/// with a value of 1.0 and use the last gradient color as this stops color.
/// * Clamp all gradient values between the values of 0.0 and 1.0.
/// * For all stop values, ensure that the values are monotonically increasing
/// by clamping each value to a minimum of the previous stop value and itself.
/// For example, with stop values of 0.0, 0.5, 0.4, 1.0, we would clamp such
Copy link
Member

Choose a reason for hiding this comment

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

This bit is surprising to me. I would have expected it to be sorted but this works too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still WIP and confirming :)

Copy link
Contributor

Choose a reason for hiding this comment

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

What did you end up finding out here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found the w3c notes which basically confirm this behavior is approximately correct

/// that the values were 0.0, 0.5, 0.5, 1.0.
void ConvertStops(const flutter::DlGradientColorSourceBase* gradient,
std::vector<Color>& colors,
std::vector<float>& stops);

} // namespace skia_conversions
} // namespace impeller
133 changes: 133 additions & 0 deletions impeller/display_list/skia_conversions_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "display_list/dl_color.h"
#include "display_list/dl_tile_mode.h"
#include "flutter/testing/testing.h"
#include "impeller/display_list/skia_conversions.h"
#include "impeller/geometry/scalar.h"
Expand All @@ -23,5 +25,136 @@ TEST(SkiaConversionsTest, ToColor) {
ASSERT_TRUE(ScalarNearlyEqual(converted_color.blue, 0x20 * (1.0f / 255)));
}

TEST(SkiaConversionsTest, GradientStopConversion) {
// Typical gradient.
std::vector<flutter::DlColor> colors = {flutter::DlColor::kBlue(),
flutter::DlColor::kRed(),
flutter::DlColor::kGreen()};
std::vector<float> stops = {0.0, 0.5, 1.0};
const auto gradient =
flutter::DlColorSource::MakeLinear(SkPoint::Make(0, 0), //
SkPoint::Make(1.0, 1.0), //
3, //
colors.data(), //
stops.data(), //
flutter::DlTileMode::kClamp, //
nullptr //
);

std::vector<Color> converted_colors;
std::vector<Scalar> converted_stops;
skia_conversions::ConvertStops(gradient.get(), converted_colors,
converted_stops);

ASSERT_TRUE(ScalarNearlyEqual(converted_stops[0], 0.0f));
ASSERT_TRUE(ScalarNearlyEqual(converted_stops[1], 0.5f));
ASSERT_TRUE(ScalarNearlyEqual(converted_stops[2], 1.0f));
}

TEST(SkiaConversionsTest, GradientMissing0) {
std::vector<flutter::DlColor> colors = {flutter::DlColor::kBlue(),
flutter::DlColor::kRed()};
std::vector<float> stops = {0.5, 1.0};
const auto gradient =
flutter::DlColorSource::MakeLinear(SkPoint::Make(0, 0), //
SkPoint::Make(1.0, 1.0), //
2, //
colors.data(), //
stops.data(), //
flutter::DlTileMode::kClamp, //
nullptr //
);

std::vector<Color> converted_colors;
std::vector<Scalar> converted_stops;
skia_conversions::ConvertStops(gradient.get(), converted_colors,
converted_stops);

// First color is inserted as blue.
ASSERT_TRUE(ScalarNearlyEqual(converted_colors[0].blue, 1.0f));
ASSERT_TRUE(ScalarNearlyEqual(converted_stops[0], 0.0f));
ASSERT_TRUE(ScalarNearlyEqual(converted_stops[1], 0.5f));
ASSERT_TRUE(ScalarNearlyEqual(converted_stops[2], 1.0f));
}

TEST(SkiaConversionsTest, GradientMissingLastValue) {
std::vector<flutter::DlColor> colors = {flutter::DlColor::kBlue(),
flutter::DlColor::kRed()};
std::vector<float> stops = {0.0, .5};
const auto gradient =
flutter::DlColorSource::MakeLinear(SkPoint::Make(0, 0), //
SkPoint::Make(1.0, 1.0), //
2, //
colors.data(), //
stops.data(), //
flutter::DlTileMode::kClamp, //
nullptr //
);

std::vector<Color> converted_colors;
std::vector<Scalar> converted_stops;
skia_conversions::ConvertStops(gradient.get(), converted_colors,
converted_stops);

// Last color is inserted as red.
ASSERT_TRUE(ScalarNearlyEqual(converted_colors[2].red, 1.0f));
ASSERT_TRUE(ScalarNearlyEqual(converted_stops[0], 0.0f));
ASSERT_TRUE(ScalarNearlyEqual(converted_stops[1], 0.5f));
ASSERT_TRUE(ScalarNearlyEqual(converted_stops[2], 1.0f));
}

TEST(SkiaConversionsTest, GradientStopGreaterThan1) {
std::vector<flutter::DlColor> colors = {flutter::DlColor::kBlue(),
flutter::DlColor::kGreen(),
flutter::DlColor::kRed()};
std::vector<float> stops = {0.0, 100, 1.0};
const auto gradient =
flutter::DlColorSource::MakeLinear(SkPoint::Make(0, 0), //
SkPoint::Make(1.0, 1.0), //
3, //
colors.data(), //
stops.data(), //
flutter::DlTileMode::kClamp, //
nullptr //
);

std::vector<Color> converted_colors;
std::vector<Scalar> converted_stops;
skia_conversions::ConvertStops(gradient.get(), converted_colors,
converted_stops);

// Value is clamped to 1.0
ASSERT_TRUE(ScalarNearlyEqual(converted_stops[0], 0.0f));
ASSERT_TRUE(ScalarNearlyEqual(converted_stops[1], 1.0f));
ASSERT_TRUE(ScalarNearlyEqual(converted_stops[2], 1.0f));
}

TEST(SkiaConversionsTest, GradientConversionNonMonotonic) {
std::vector<flutter::DlColor> colors = {
flutter::DlColor::kBlue(), flutter::DlColor::kGreen(),
flutter::DlColor::kGreen(), flutter::DlColor::kRed()};
std::vector<float> stops = {0.0, 0.5, 0.4, 1.0};
const auto gradient =
flutter::DlColorSource::MakeLinear(SkPoint::Make(0, 0), //
SkPoint::Make(1.0, 1.0), //
4, //
colors.data(), //
stops.data(), //
flutter::DlTileMode::kClamp, //
nullptr //
);

std::vector<Color> converted_colors;
std::vector<Scalar> converted_stops;
skia_conversions::ConvertStops(gradient.get(), converted_colors,
converted_stops);

// Value is clamped to 0.5
ASSERT_TRUE(ScalarNearlyEqual(converted_stops[0], 0.0f));
ASSERT_TRUE(ScalarNearlyEqual(converted_stops[1], 0.5f));
ASSERT_TRUE(ScalarNearlyEqual(converted_stops[2], 0.5f));
ASSERT_TRUE(ScalarNearlyEqual(converted_stops[3], 1.0f));
}

} // namespace testing
} // namespace impeller
18 changes: 15 additions & 3 deletions lib/ui/painting.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4191,7 +4191,11 @@ base class Gradient extends Shader {
/// If `colorStops` is provided, `colorStops[i]` is a number from 0.0 to 1.0
/// that specifies where `color[i]` begins in the gradient. If `colorStops` is
/// not provided, then only two stops, at 0.0 and 1.0, are implied (and
/// `color` must therefore only have two entries).
/// `color` must therefore only have two entries). Stop values less than 0.0
/// will be rounded up to 0.0 and stop values greater than 1.0 will be rounded
/// down to 1.0. Each stop value must be greater than or equal to the previous
/// stop value. Stop values that do not meet this criteria will be rounded up
/// to the previous stop value.
///
/// The behavior before `from` and after `to` is described by the `tileMode`
/// argument. For details, see the [TileMode] enum.
Expand Down Expand Up @@ -4233,7 +4237,11 @@ base class Gradient extends Shader {
/// If `colorStops` is provided, `colorStops[i]` is a number from 0.0 to 1.0
/// that specifies where `color[i]` begins in the gradient. If `colorStops` is
/// not provided, then only two stops, at 0.0 and 1.0, are implied (and
/// `color` must therefore only have two entries).
/// `color` must therefore only have two entries). Stop values less than 0.0
Copy link
Member

Choose a reason for hiding this comment

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

The docstring in the C++ header indicates that the second color is optional and the first will be repeated. Perhaps add that here too. Unless the framework has additional checks in place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That isn't a valid gradient according to our docs, but w3c says this should degrade to a solid color fill. I'd rather punt on this behavior.

Copy link
Member

Choose a reason for hiding this comment

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

👍🏽

/// will be rounded up to 0.0 and stop values greater than 1.0 will be rounded
/// down to 1.0. Each stop value must be greater than or equal to the previous
/// stop value. Stop values that do not meet this criteria will be rounded up
/// to the previous stop value.
///
/// The behavior before and after the radius is described by the `tileMode`
/// argument. For details, see the [TileMode] enum.
Expand Down Expand Up @@ -4295,7 +4303,11 @@ base class Gradient extends Shader {
/// If `colorStops` is provided, `colorStops[i]` is a number from 0.0 to 1.0
/// that specifies where `color[i]` begins in the gradient. If `colorStops` is
/// not provided, then only two stops, at 0.0 and 1.0, are implied (and
/// `color` must therefore only have two entries).
/// `color` must therefore only have two entries). Stop values less than 0.0
/// will be rounded up to 0.0 and stop values greater than 1.0 will be rounded
/// down to 1.0. Each stop value must be greater than or equal to the previous
/// stop value. Stop values that do not meet this criteria will be rounded up
/// to the previous stop value.
///
/// The behavior before `startAngle` and after `endAngle` is described by the
/// `tileMode` argument. For details, see the [TileMode] enum.
Expand Down