[skrifa] Implement an optimized fill_glyph operation

[drott]

In COLRv0/v1 drawing clients may gain a performance benefit from filling
a shape (with a solid or gradient fill) instead of performing a clip +
unbounded fill in sequence.

Allow clients to do that using a fill_glyph() operation that combines
the two.

Implement this using a temporary painter that speculatively traverses
the glyph tree down from a PaintGlyph operation collecting potential
transforms along the way.

New behavior shows clearly in new baselines for tests added in #758 that
nest PaintGlyph tables and cover mixed situations in
which the optimization succeeds for a leaf but fails for the
PaintGlyph at a layer above, and demonstrate recording transforms
along the way.

Fixes #746.

[drott]

I would like to send this for review now. This is still a nested painter-based implementation. I have opted for now not to change signatures of the draw functions to return a Result and thus potentially be able to cancel drawing, as I believe this realizes the performance benefit and we would have yet to discover pathological situations in which there are deeply nested trees behind a PaintGlyph. The benefit the nested painter approach I see in avoiding implementing separate imperative traversal logic - instead - the existing traverse_with_callbacks can be used.

Please let me know if this works for you or if there are strong opinions about a different approach. I am open to revisiting this, but for now, this definitely meets the mark in terms of benchmark results as shared in the chat. An API change to Result<>-returning, cancellable methods would require a roundtrip of deprecation steps and careful chaining it into Skia and then Chrome with a phase of parallel ColorPainter functions along the way. I prefer to save that time atm and focus on implementing bitmap fonts support next.

[dfrg]

Thanks for this!

I’m happy to land with the nested traversal since it works and seems to hit our performance targets. Upon further thought, I’m not too excited about complicating the API with cancellation. If we do encounter deeply nested graphs under a clip glyph, I’d prefer to implement the procedural matching instead which wouldn’t require changing the API.

I noted one request for a change and a couple nits. Otherwise lgtm!

[dfrg]

This should probably be called brush_transform just to further signify that it applies to the brush and not the clip.

Also, optional: make the transform optional :) I suspect this will be identity most of the time.

[drott]

a) Renamed to brush_transform and updated arg name in documentation.
b) Transform turned into Option<Transform> - I wasn't sure if you prefer to have that be expressed in the test results as an empty string - for now I left testing to dump an identity matrix in that case, instead of special casing the None situation, but agree this might be helpful for clients. Let me know if you want changes on the testing side. On the Skia side I am also making this a default matrix in the callbacks to C++.

[dfrg]

Your approach looks good to me. Thanks for making the change.

[dfrg]

I realize we don’t strictly need it here but this will be exposed publicly and having an implementation of MulAssign without Mul feels a bit weird.

[drott]

Added a Mul implementation, moved the logic into that and made the MulAssign implementation based on the former - hope that's what's usually done. Not sure I need the clone() there, but I guess I need some sort of temporary value at least?

[dfrg]

We could also derive Copy for Transform which would remove the need for the clone. I’m good either way.

[drott]

I'll add that.