Eliminate heap allocations for gradient color conversions

[gaaclarke]

Spawned from flutter/engine#54748

When colors of a gradient are communicated from Dart to C++ they are coming in as arrays of floats which represent the color components of the colors in extended sRGB. When they get stored in the display list they are stored as an array of DlColor. As a temporary measure the floats are converted to DlColor on the heap, then sent into the display list for storage. We can't do it on the stack because the number of colors is unlimited.

I looked into trying to switch the DlColorSource factory methods to take in DlScalar*'s, that way it could be converted to DlColors while we were iterating over them, avoiding the heap allocation. That doesn't work however because there is a second client of those functions, the DlColorSource::shared() methods which does not have DlScalar*, they already have their colors stored as DlColor*. The only way to make that work would have been to refactor all the API's to take in an iterator. That's kind of messy in C++ since it would mean surfacing more logic into templates and headers, or incurring dynamic dispatch cost.

@jonahwilliams proposes we should do a larger refactor where we change the storage of the DlColorSource to instead store its colors as a DlScalar*. That way both clients of the DlColorColorSource::Make* functions can pass in DlScalar*. He noted that storing the colorspace per color may be redundant anyways.

cc @flar

[gaaclarke]

I think potentially the work in flutter/engine#54963 can be continued, we just have to rewrite the storage of the classes to also be DlScalars instead of DlColors.

[gaaclarke]

@flar I'm just reorganizing all the work items I have left for wide gamut framework. If you are interested I can hand this over to you. I have a few other things I could be working on. Let me know if you are interested.

[flar]

I'll take this on, I'm going to have to modify these classes to deal with DlPoint/DlMatrix, etc. anyway...

[gaaclarke]

I looked into this a bit. The flow is like so:

CanvasGradient::initLinear - converts DlScalar[] to DlColor[] (with heap allocation)
  DlColorSource::MakeLinear - calculates size of DlLinearGradientColorSource based on size of DlColor[]
    DlLinearGradientColorSource::DlLinearGradientColorSource - memcpy DlColor[] into this

The options are:

1. Use template iterators - this avoids the extra heap allocation but interrupts a faster memcpy operation, it also complicates the code by moving more code into headers
2. Use virtual iterators - this introduces dynamic dispatch for each color, but removes the heap allocation
3. std::move DlColor[] - This makes the heap allocation not wasted since it gets passed onto the color source, thus removing a memcpy. This changes the logic of DlLinearGradientColorSource a bit, probably for the better since it's weird not having a fixed size instance. The locality of the instance will be broken up though.
4. Switch the storage of DlLinearGradientColorSource to DlScalar - this would be unfortunate to degrade the class to store components instead of instances.
5. Cap the size of supported colors - it's kind of silly to bend over backwards for a feature we don't even want anyways
6. Use a fixed array on the stack unless the number of colors is over a threshold - this would capture most use cases but was already vetoed as an option.
7. Reuse the vector across invocations of CanvasGradient::initLinear - caching values can always lead to weird bugs and I don't know if there is a good owner for this vector.

Looking at all the options I'm still partial to # 6.

[flar]

Another option is to have DlGradient variants that take the DlScalar representation as input and initialize and convert directly into the allocated storage?

[jonahwilliams]

Yeah I would be in favor of that over the other approaches

[gaaclarke]

Okay, did that but used iterators in the .cc file to remove duplication, a blend of 2 things proposed.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.