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

Framework wide color linear gradients #54748

Merged
auto-submit merged 7 commits into from
Sep 4, 2024
Merged

Framework wide color linear gradients #54748

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
408f549
Framework wide gamut - implemented linear gradients
gaaclarke Aug 23, 2024
fc754cf
removed todo
gaaclarke Aug 23, 2024
d9c6a96
implemented radial gradient
gaaclarke Aug 23, 2024
976963c
added conical gradient
gaaclarke Aug 23, 2024
5c0887f
added sweep gradient
gaaclarke Aug 23, 2024
0419148
fixed color size
gaaclarke Sep 3, 2024
66b244e
jonah feedback 1, fixed overactive allocation
gaaclarke Sep 3, 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
3 changes: 2 additions & 1 deletion impeller/display_list/skia_conversions.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,8 @@ std::optional<impeller::PixelFormat> ToPixelFormat(SkColorType type) {
void ConvertStops(const flutter::DlGradientColorSourceBase* gradient,
std::vector<Color>& colors,
std::vector<float>& stops) {
FML_DCHECK(gradient->stop_count() >= 2);
FML_DCHECK(gradient->stop_count() >= 2)
<< "stop_count:" << gradient->stop_count();

auto* dl_colors = gradient->colors();
auto* dl_stops = gradient->stops();
Expand Down
29 changes: 22 additions & 7 deletions lib/ui/painting.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4452,6 +4452,21 @@ enum TileMode {
decal,
}

Float32List _encodeWideColorList(List<Color> colors) {
Copy link
Contributor

@flar flar Sep 3, 2024

Choose a reason for hiding this comment

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

I remember talking about preserving the CS, but I'm guessing that gradients are going to want to normalize to a similar CS for lerping - and extendedSRGB is probably the best for fractional combinations?

Copy link
Member Author

@gaaclarke gaaclarke Sep 3, 2024

Choose a reason for hiding this comment

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

We do preserve the colorspace from the users perspective. The conversion to extended srgb only happens when communicating the values to the engine.

final int colorCount = colors.length;
final Float32List result = Float32List(colorCount * 4);
for (int i = 0; i < colorCount; ++i) {
final Color colorXr =
colors[i].withValues(colorSpace: ColorSpace.extendedSRGB);
result[i*4+0] = colorXr.a;
result[i*4+1] = colorXr.r;
result[i*4+2] = colorXr.g;
result[i*4+3] = colorXr.b;
}
return result;
}


Int32List _encodeColorList(List<Color> colors) {
final int colorCount = colors.length;
final Int32List result = Int32List(colorCount);
Expand Down Expand Up @@ -4535,7 +4550,7 @@ base class Gradient extends Shader {
super._() {
_validateColorStops(colors, colorStops);
final Float32List endPointsBuffer = _encodeTwoPoints(from, to);
final Int32List colorsBuffer = _encodeColorList(colors);
final Float32List colorsBuffer = _encodeWideColorList(colors);
final Float32List? colorStopsBuffer = colorStops == null ? null : Float32List.fromList(colorStops);
_constructor();
_initLinear(endPointsBuffer, colorsBuffer, colorStopsBuffer, tileMode.index, matrix4);
Expand Down Expand Up @@ -4588,8 +4603,8 @@ base class Gradient extends Shader {
assert(matrix4 == null || _matrix4IsValid(matrix4)),
super._() {
_validateColorStops(colors, colorStops);
final Int32List colorsBuffer = _encodeColorList(colors);
final Float32List? colorStopsBuffer = colorStops == null ? null : Float32List.fromList(colorStops);
final Float32List colorsBuffer = _encodeWideColorList(colors);

// If focal is null or focal radius is null, this should be treated as a regular radial gradient
// If focal == center and the focal radius is 0.0, it's still a regular radial gradient
Expand Down Expand Up @@ -4647,7 +4662,7 @@ base class Gradient extends Shader {
assert(matrix4 == null || _matrix4IsValid(matrix4)),
super._() {
_validateColorStops(colors, colorStops);
final Int32List colorsBuffer = _encodeColorList(colors);
final Float32List colorsBuffer = _encodeWideColorList(colors);
final Float32List? colorStopsBuffer = colorStops == null ? null : Float32List.fromList(colorStops);
_constructor();
_initSweep(center.dx, center.dy, colorsBuffer, colorStopsBuffer, tileMode.index, startAngle, endAngle, matrix4);
Expand All @@ -4657,14 +4672,14 @@ base class Gradient extends Shader {
external void _constructor();

@Native<Void Function(Pointer<Void>, Handle, Handle, Handle, Int32, Handle)>(symbol: 'Gradient::initLinear')
external void _initLinear(Float32List endPoints, Int32List colors, Float32List? colorStops, int tileMode, Float64List? matrix4);
external void _initLinear(Float32List endPoints, Float32List colors, Float32List? colorStops, int tileMode, Float64List? matrix4);

@Native<Void Function(Pointer<Void>, Double, Double, Double, Handle, Handle, Int32, Handle)>(symbol: 'Gradient::initRadial')
external void _initRadial(
double centerX,
double centerY,
double radius,
Int32List colors,
Float32List colors,
Float32List? colorStops,
int tileMode,
Float64List? matrix4);
Expand All @@ -4677,7 +4692,7 @@ base class Gradient extends Shader {
double endX,
double endY,
double endRadius,
Int32List colors,
Float32List colors,
Float32List? colorStops,
int tileMode,
Float64List? matrix4);
Expand All @@ -4686,7 +4701,7 @@ base class Gradient extends Shader {
external void _initSweep(
double centerX,
double centerY,
Int32List colors,
Float32List colors,
Float32List? colorStops,
int tileMode,
double startAngle,
Expand Down
61 changes: 38 additions & 23 deletions lib/ui/painting/gradient.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ void CanvasGradient::Create(Dart_Handle wrapper) {
}

void CanvasGradient::initLinear(const tonic::Float32List& end_points,
const tonic::Int32List& colors,
const tonic::Float32List& colors,
const tonic::Float32List& color_stops,
DlTileMode tile_mode,
const tonic::Float64List& matrix4) {
FML_DCHECK(end_points.num_elements() == 4);
FML_DCHECK(colors.num_elements() == color_stops.num_elements() ||
FML_DCHECK(colors.num_elements() == (color_stops.num_elements() * 4) ||
color_stops.data() == nullptr);

static_assert(sizeof(SkPoint) == sizeof(float) * 2,
Expand All @@ -46,14 +46,17 @@ void CanvasGradient::initLinear(const tonic::Float32List& end_points,
SkPoint p0 = SkPoint::Make(end_points[0], end_points[1]);
SkPoint p1 = SkPoint::Make(end_points[2], end_points[3]);
std::vector<DlColor> dl_colors;
dl_colors.reserve(colors.num_elements());
for (int i = 0; i < colors.num_elements(); ++i) {
/// TODO(gaaclarke): Make this preserve wide gamut colors.
dl_colors.emplace_back(DlColor(colors[i]));
dl_colors.reserve(color_stops.num_elements());
Copy link
Contributor

@jonahwilliams jonahwilliams Sep 3, 2024

Choose a reason for hiding this comment

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

Can't this go back to a cast now? We're copying the data into the display list again so we shouldn't need to allocate more storage.

Copy link
Contributor

@jonahwilliams jonahwilliams Sep 3, 2024

Choose a reason for hiding this comment

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

Oh, we can't cast it anymore since the color space field.

In that case, can we update this API so we reserve and write the color bytes directly into the DL storage instead of allocating an intermediary?

Copy link
Member Author

@gaaclarke gaaclarke Sep 3, 2024

Choose a reason for hiding this comment

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

In order to have a cast the memory layouts need to be identical. Here we have an array of floats, we need to insert the colorspace field into the memory layout if we wanted to use casts. The casts are super frail. I don't recommend it unless we get some data that supports it's worth the risk.

If we did find it was slowing us down, I would recommend doing the conversion on the stack when the number of colors is less than a certain size to catch most use cases before resorting to an unsafe cast.

Copy link
Member Author

@gaaclarke gaaclarke Sep 3, 2024

Choose a reason for hiding this comment

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

Something like this:

if (num_colors <= 3) {
  std::array<DlColor, 3> dl_colors;
  //...
} else {
  std::vector<DlColor> dl_colors;
  dl_colors.reserve(num_colors);
  //...
}

I don't think it's worth the extra complexity, but I can do it if we want. I know we were bit in the past with too many heap allocations.

Copy link
Contributor

@jonahwilliams jonahwilliams Sep 3, 2024

Choose a reason for hiding this comment

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

All this code is doing is passing the data into the DLBuilder so it can be copied into the display list. Can we copy into the final destination instead of allocating an intermediary?

Copy link
Contributor

@flar flar Sep 4, 2024

Choose a reason for hiding this comment

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

I think I'm gradually absorbing the concern here. Talk of "DL" has me thinking of the display list object itself with recorded operations, but I think the concern here is that DlFooGradientColorSource embeds the data into a contiguous chunk and we are converting from one array to a temp vector of DlColor objects and then into the data co-located with the eventual DlColorSource object.

So, yes, it would be nice if we could copy directly from the constructor/factory argument into the pod allocation without first creating a vector of DlColor objects. I'd be happy to see that added as a future optimization and just stick with making some sort of list of DlColor objects for now to get this capability implemented in its simplest fashion.

We could also consider having the storage for these gradient objects be just an array of floats and a separate common CS. The gradient objects could still get you a single color in a getter by marrying the right 4 floats and the CS into a DlColor object, but there would also be accessors to the common CS value and the array of component float data which the backend might use more efficiently. It would also save on storage size.

Copy link
Contributor

@flar flar Sep 4, 2024

Choose a reason for hiding this comment

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

Also, I will be coming through these classes to de-skia-fy them shortly, so I can implement such optimizations as I do that work. That work should all fall under "should have no impact at all on output" changes and so would hopefully pass up the chain without regressions.

Copy link
Member Author

@gaaclarke gaaclarke Sep 4, 2024

Choose a reason for hiding this comment

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

I'd prefer we use a named factory so that it is clear what order the scalars are supposed to be in the array.

That's what I'm doing I'm refactoring DlColorSource to use the Builder pattern. That way we can pass in DlColor arrays or float arrays.

The logic that I'm asking you to add does not need an intergration test to cover it (in fact it may already be tested)

This PR is adding new functionality that is only tested in an integration test that has to live in another repo. This PR implements the feature while working within the bounds of the current API. I agree with your feedback and it will be addressed. That's why we went through the whole process of brainstorming a solution. However this PR does not add any new heap allocations, they were already there, removing them is going to be a bit of a refactor, one who would be served by having a test as a backstop to make sure we don't break the thing we are actually trying to implement.

We can withhold the approval of this PR until you see the refactor PR if you want. I don't understand why I'm not being trusted here.

Copy link
Contributor

@jonahwilliams jonahwilliams Sep 4, 2024

Choose a reason for hiding this comment

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

Removing an unnecessary heap allocation does not require an integration test

Copy link
Member Author

@gaaclarke gaaclarke Sep 4, 2024

Choose a reason for hiding this comment

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

Moving to the functions to DlScalar* is not going to work because there are 2 clients of DlLinearGradientColorSource::MakeLinear. In one case we have DlScalar*, in another we have DlColor*. Moving to DlScalar would require us to do heap allocations when converting in the other route. The other client is DlColorSource::shared(), I tried removing it and that runs deep coming from DlAttribute.

The only viable solution is to use the iterator pattern if we want to eliminate heap allocations in both cases. That would require moving more logic to templates or a dynamic dispatch, which would possibly eliminate any benefit of removing the tiny heap allocation.

I think the best bet is to eliminate the heap in most cases but have an overflow for gradients with large amounts of colors.

for (int i = 0; i < colors.num_elements(); i += 4) {
DlScalar a = colors[i + 0];
DlScalar r = colors[i + 1];
DlScalar g = colors[i + 2];
DlScalar b = colors[i + 3];
dl_colors.emplace_back(DlColor(a, r, g, b, DlColorSpace::kExtendedSRGB));
}

dl_shader_ = DlColorSource::MakeLinear(
p0, p1, colors.num_elements(), dl_colors.data(), color_stops.data(),
p0, p1, colors.num_elements() / 4, dl_colors.data(), color_stops.data(),
tile_mode, has_matrix ? &sk_matrix : nullptr);
// Just a sanity check, all gradient shaders should be thread-safe
FML_DCHECK(dl_shader_->isUIThreadSafe());
Expand All @@ -62,11 +65,11 @@ void CanvasGradient::initLinear(const tonic::Float32List& end_points,
void CanvasGradient::initRadial(double center_x,
double center_y,
double radius,
const tonic::Int32List& colors,
const tonic::Float32List& colors,
const tonic::Float32List& color_stops,
DlTileMode tile_mode,
const tonic::Float64List& matrix4) {
FML_DCHECK(colors.num_elements() == color_stops.num_elements() ||
FML_DCHECK(colors.num_elements() == (color_stops.num_elements() * 4) ||
color_stops.data() == nullptr);

static_assert(sizeof(SkColor) == sizeof(int32_t),
Expand All @@ -80,27 +83,31 @@ void CanvasGradient::initRadial(double center_x,

std::vector<DlColor> dl_colors;
dl_colors.reserve(colors.num_elements());
for (int i = 0; i < colors.num_elements(); ++i) {
dl_colors.emplace_back(DlColor(colors[i]));
for (int i = 0; i < colors.num_elements(); i += 4) {
DlScalar a = colors[i + 0];
DlScalar r = colors[i + 1];
DlScalar g = colors[i + 2];
DlScalar b = colors[i + 3];
dl_colors.emplace_back(DlColor(a, r, g, b, DlColorSpace::kExtendedSRGB));
}

dl_shader_ = DlColorSource::MakeRadial(
SkPoint::Make(SafeNarrow(center_x), SafeNarrow(center_y)),
SafeNarrow(radius), colors.num_elements(), dl_colors.data(),
SafeNarrow(radius), colors.num_elements() / 4, dl_colors.data(),
color_stops.data(), tile_mode, has_matrix ? &sk_matrix : nullptr);
// Just a sanity check, all gradient shaders should be thread-safe
FML_DCHECK(dl_shader_->isUIThreadSafe());
}

void CanvasGradient::initSweep(double center_x,
double center_y,
const tonic::Int32List& colors,
const tonic::Float32List& colors,
const tonic::Float32List& color_stops,
DlTileMode tile_mode,
double start_angle,
double end_angle,
const tonic::Float64List& matrix4) {
FML_DCHECK(colors.num_elements() == color_stops.num_elements() ||
FML_DCHECK(colors.num_elements() == (color_stops.num_elements() * 4) ||
color_stops.data() == nullptr);

static_assert(sizeof(SkColor) == sizeof(int32_t),
Expand All @@ -114,16 +121,20 @@ void CanvasGradient::initSweep(double center_x,

std::vector<DlColor> dl_colors;
dl_colors.reserve(colors.num_elements());
for (int i = 0; i < colors.num_elements(); ++i) {
dl_colors.emplace_back(DlColor(colors[i]));
for (int i = 0; i < colors.num_elements(); i += 4) {
DlScalar a = colors[i + 0];
DlScalar r = colors[i + 1];
DlScalar g = colors[i + 2];
DlScalar b = colors[i + 3];
dl_colors.emplace_back(DlColor(a, r, g, b, DlColorSpace::kExtendedSRGB));
}

dl_shader_ = DlColorSource::MakeSweep(
SkPoint::Make(SafeNarrow(center_x), SafeNarrow(center_y)),
SafeNarrow(start_angle) * 180.0f / static_cast<float>(M_PI),
SafeNarrow(end_angle) * 180.0f / static_cast<float>(M_PI),
colors.num_elements(), dl_colors.data(), color_stops.data(), tile_mode,
has_matrix ? &sk_matrix : nullptr);
colors.num_elements() / 4, dl_colors.data(), color_stops.data(),
tile_mode, has_matrix ? &sk_matrix : nullptr);
// Just a sanity check, all gradient shaders should be thread-safe
FML_DCHECK(dl_shader_->isUIThreadSafe());
}
Expand All @@ -134,11 +145,11 @@ void CanvasGradient::initTwoPointConical(double start_x,
double end_x,
double end_y,
double end_radius,
const tonic::Int32List& colors,
const tonic::Float32List& colors,
const tonic::Float32List& color_stops,
DlTileMode tile_mode,
const tonic::Float64List& matrix4) {
FML_DCHECK(colors.num_elements() == color_stops.num_elements() ||
FML_DCHECK(colors.num_elements() == (color_stops.num_elements() * 4) ||
color_stops.data() == nullptr);

static_assert(sizeof(SkColor) == sizeof(int32_t),
Expand All @@ -152,15 +163,19 @@ void CanvasGradient::initTwoPointConical(double start_x,

std::vector<DlColor> dl_colors;
dl_colors.reserve(colors.num_elements());
for (int i = 0; i < colors.num_elements(); ++i) {
dl_colors.emplace_back(DlColor(colors[i]));
for (int i = 0; i < colors.num_elements(); i += 4) {
DlScalar a = colors[i + 0];
DlScalar r = colors[i + 1];
DlScalar g = colors[i + 2];
DlScalar b = colors[i + 3];
dl_colors.emplace_back(DlColor(a, r, g, b, DlColorSpace::kExtendedSRGB));
}

dl_shader_ = DlColorSource::MakeConical(
SkPoint::Make(SafeNarrow(start_x), SafeNarrow(start_y)),
SafeNarrow(start_radius),
SkPoint::Make(SafeNarrow(end_x), SafeNarrow(end_y)),
SafeNarrow(end_radius), colors.num_elements(), dl_colors.data(),
SafeNarrow(end_radius), colors.num_elements() / 4, dl_colors.data(),
color_stops.data(), tile_mode, has_matrix ? &sk_matrix : nullptr);
// Just a sanity check, all gradient shaders should be thread-safe
FML_DCHECK(dl_shader_->isUIThreadSafe());
Expand Down
8 changes: 4 additions & 4 deletions lib/ui/painting/gradient.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,22 +21,22 @@ class CanvasGradient : public Shader {
static void Create(Dart_Handle wrapper);

void initLinear(const tonic::Float32List& end_points,
const tonic::Int32List& colors,
const tonic::Float32List& colors,
const tonic::Float32List& color_stops,
DlTileMode tile_mode,
const tonic::Float64List& matrix4);

void initRadial(double center_x,
double center_y,
double radius,
const tonic::Int32List& colors,
const tonic::Float32List& colors,
const tonic::Float32List& color_stops,
DlTileMode tile_mode,
const tonic::Float64List& matrix4);

void initSweep(double center_x,
double center_y,
const tonic::Int32List& colors,
const tonic::Float32List& colors,
const tonic::Float32List& color_stops,
DlTileMode tile_mode,
double start_angle,
Expand All @@ -49,7 +49,7 @@ class CanvasGradient : public Shader {
double end_x,
double end_y,
double end_radius,
const tonic::Int32List& colors,
const tonic::Float32List& colors,
const tonic::Float32List& color_stops,
DlTileMode tile_mode,
const tonic::Float64List& matrix4);
Expand Down