Framework wide color linear gradients

[gaaclarke]

This implements wide gamut color support for gradients in the engine.

issue: flutter/flutter#127855
integration test: flutter/flutter#153976

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the CLA.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[gaaclarke]

@jonahwilliams @flar this PR relies on the integration test for verification. Let me know if there is a more direct way in the engine you'd like to see it tested. canvas_test.dart does execute this code.

[jonahwilliams]

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.

[jonahwilliams]

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?

[gaaclarke]

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.

[gaaclarke]

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.

[jonahwilliams]

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?

[flar]

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.

[flar]

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.

[gaaclarke]

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.

[jonahwilliams]

Removing an unnecessary heap allocation does not require an integration test

[gaaclarke]

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.

[flar]

Can we have a helper method, or would that involve unnecessary copying? I thought the compiler optimized the case of returning a vector...??

Another way is to create the vector on the local stack and then hand it into the converter method by reference.

[gaaclarke]

Can we have a helper method, or would that involve unnecessary copying? I thought the compiler optimized the case of returning a vector...??

Another way is to create the vector on the local stack and then hand it into the converter method by reference.

You can't have a vector on a stack. It will always allocate memory on the heap. The size is needed to be known at compilation time in order to put something on the stack (without trickery). I have a proposition in an above thread for using an std::array on the stack.

I think avoiding the copying will require DlColorSource some mechanism to read from the tonic collection directly.

[flar]

The data has to be heap allocated because it is growable, that will happen whether a helper function is used or not. I'm referring to the object itself, but I suppose that the object isn't very big whether it is stack allocated or not. It's a vtable + data pointer + allocated size + used size.

[gaaclarke]

Oh, I see, sorry I thought this was still related to the heap discussion. Yes we could have a helper function and yes C++ would perform the return value optimization.

[flar]

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?

[gaaclarke]

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

[flar]

@jonahwilliams @flar this PR relies on the integration test for verification. Let me know if there is a more direct way in the engine you'd like to see it tested. canvas_test.dart does execute this code.

Are there tests that test ui.Color(...) == DlColor(...)?

[gaaclarke]

@jonahwilliams @flar this PR relies on the integration test for verification. Let me know if there is a more direct way in the engine you'd like to see it tested. canvas_test.dart does execute this code.

Are there tests that test ui.Color(...) == DlColor(...)?

The test Simple gradient, which is implicitly dithered in canvas_test.dart does check ui.Color versus a golden image.

edit: link -

engine/testing/dart/canvas_test.dart

Line 165 in 8d7d5ff

test('Simple gradient, which is implicitly dithered', () async {

[jonahwilliams]

LGTM

@gaaclarke and I looked at this offline. I agree that the refactor can probably wait on this landing in the framework + other testing work. @flar would you be available to pair with @gaaclarke and come up with a way to remove the intermediate allocation?