[skrifa] COLRv1: remove nested traversal optimization and limit total work - #2009
Merged
Conversation
Limit total number of paints processed for a single glyph.
cmyr
approved these changes
Aug 6, 2026
cmyr
left a comment
cmyr
left a comment
Member
There was a problem hiding this comment.
okay i've spent thirty minutes digging into this and do find the patch very hard to follow as a consequence of all the churn, but thanks to my tireless assistant I've left a few comments inline.
| &mut self, | ||
| paint: &Paint<'a>, | ||
| ) -> Result<ResolvedPaint<'a>, PaintError> { | ||
| self.nodes_left = self |
Member
There was a problem hiding this comment.
so we track nodes, but iiuc a 'node' can correspond to quite a large range of possible work. Are we worried about a malicious font where a smaller number of nodes are performing a larger number of operations?
Contributor
Author
There was a problem hiding this comment.
This is a good point, particularly with regard to gradients with variable color stops. It might be worth adding an additional limit for those though I’m not sure what a reasonable value might be. Napkin math: ~4k nodes * 65k stops * 2 values = 4*10^8 IVS lookups for a single glyph.
@behdad I don’t think HB has a limit for these? Should we?
| &resolve_paint(instance, &base_glyph)?, | ||
| instance, | ||
| painter, | ||
| &state.resolve_paint(&base_glyph)?, |
Member
There was a problem hiding this comment.
reviewing with opus and it points out that this error path results in us potentially leaving a clip box on the paint context that is never popped. I'm not sure if this is a concern in practice, but seems worth pointing out?
Contributor
Author
There was a problem hiding this comment.
We can fail at any point during traversal so this isn’t new. Clients that want to avoid partial rendering (or inconsistent graphics state) for broken glyphs will need to render into an offscreen buffer or accumulate the commands into some intermediate data structure.
dfrg
added a commit
that referenced
this pull request
Aug 12, 2026
) Note: patch bumps for font-types and shared-brotli-patch-decoder were already done in the MSRV reduction patch Changes for font-types from font-types-v0.12.2 to 0.12.3 d28db34 [font-types] remove assert from Version16Dot16::new (#2041) 21b17b4 [font-types] add normalize method to bbox (#2043) 8f295ea Lower MSRV to 1.85 and add MSRV CI check 227d12e [chore] Fix some typos (#2005) Changes for read-fonts from read-fonts-v0.42.1 to 0.43.0 39fdc1a [chore] tidy for release (#2045) 78aa50d [read-fonts] fvar: use different normalization path for large number of axes (#2038) 73fd5ce [read-fonts] reduce avar application complexity (#2036) e29b8b5 [read-fonts] avoid panics in classdef intersection (#2028) 4e1f904 [read-fonts] FontAPI: add pure CFF variant to FontKind (#2034) 555ff88 [read-fonts] avoid Device iter subtraction overflow (#2027) 10b5d69 [read-fonts] handle FD Select bounds (#2026) e63756f [read-fonts] Ignore short indexTablesSize when reading EBLC/CBLC index subtables (#2020) 419aa42 fix build issues caused by merging stacked PR a9aa24f [glyph closure] use IntSet is_subset() API 6cd24c9 Add is_subset() method to IntSet. bb6f871 Add is_subset() method to IntSet. e0487b9 [glyph closure] address review comments 950fd5f fix clippy ae4b992 [glyph closure] make fnv feature gated d386422 [glyph closure] add intersected_class_cache e00c732 [closure] Add FnvHashMap, use it as a cache during closure 35ca72a [glyph closure] remove unncessary set allocation 95e1159 [glyph closure] move seen_sequence_indices out of loop 1d3e931 [glyph closure] reduce set allocations for (Chain)ContextFormat1 76c63d8 [closure] reduce set allocations for (Chain)ContextFormat1 40b07af [glyph closure] reduce allocations for (Chain)ContextFormat2 3dfbae6 [glyph closure] add a fn in classdef 7d464b8 [read-fonts] aat: use wrapping to avoid panics on 32-bit (#2017) d0f5d07 [read-fonts] avoid overflow panics in CFF charset (#2016) 8f295ea Lower MSRV to 1.85 and add MSRV CI check f95a2db [read-fonts] type1: remove unbounded recursion (#1993) 227d12e [chore] Fix some typos (#2005) d876963 [read-fonts] limit work in charstring eval (#2000) 40cd87d Remove support for IFT Patch Map Format 1. 8f5f551 [read-fonts] apply imageDataOffset to format 4 sbit offsets (#1994) 746053e fix clippy d394729 [glyph closure] remove unused fn 823c1ea [glyph closure] try to reduce allocations by splitting borrows 8ec1dca [read-fonts] avoid range reset for cmap12/13 iter (#1986) fbe0cb3 [read-fonts] optimize post glyph name iter (#1983) Changes for font-test-data from font-test-data-v0.8.0 to 0.9.0 8f295ea Lower MSRV to 1.85 and add MSRV CI check 40cd87d Remove support for IFT Patch Map Format 1. fbe0cb3 [read-fonts] optimize post glyph name iter (#1983) Changes for write-fonts from write-fonts-v0.51.0 to 0.52.0 8f295ea Lower MSRV to 1.85 and add MSRV CI check 227d12e [chore] Fix some typos (#2005) 40cd87d Remove support for IFT Patch Map Format 1. Changes for skrifa from skrifa-v0.45.1 to 0.46.0 c244623 [skrifa] tthint: ignore fractional bits in parity checks- #2040 #2040 f3edd9d [skrifa] tthint: avoid overflow in line_vector (#2039) 60877df [skrifa] autohint: avoid panic on font with 0 glyphs (#2044) 29ae374 [skrifa] COLRv1: avoid unbalanced stack state on error (#2031) e829e72 [skrifa] VARC: limit number of edges processed (#2032) 9bbb04d [skrifa] COLRv1: remove nested traversal optimization and limit total work (#2009) 487a333 [skrifa] avoid more overflow panics in TT hinting (#2011) 8f295ea Lower MSRV to 1.85 and add MSRV CI check f43403c skrifa/varc: don't panic on a null MultiItemVariationStore offset (#2012) 867fe4d skrifa/varc: bound condition-evaluation recursion (#2014) 5d8cc1d [skrifa] autohint: explicit guards for indexed access (#1988) 38fdf8c [skrifa] autohint: ensure indices fit in u16 (#1989) b7487ff [skrifa] autohint: avoid panic on overflow (#1987) 1041d90 [skrifa] smallvec: don't pretend we do fallible alloc (#1990) fbe0cb3 [read-fonts] optimize post glyph name iter (#1983) Changes for skera from skera-v0.5.1 to 0.6.0 961143e [skera-repacker] use enum type for parents b65f470 [skera-repack] single parent optimization 8f295ea Lower MSRV to 1.85 and add MSRV CI check 4d03189 [skera] add doc for profiling skera 227d12e [chore] Fix some typos (#2005) aeede19 [skera] name: don't panic on out-of-range name record count 37c89dd [skera] move plan creation backc into interation loop 21571e8 [skera] move plan creation out of iterating loop 739ee69 [skera] tweak Benchmark suite 0d5779d [skera-repack] revert previous change to add_parent() 0c818f8 [skera] fix comment a16ef39 [skera-repack] fix fix_virtual_links() in ligatureSubst splitting 3213a55 [skera-rapack] remove unused fn d2b3791 [skera-repack] fix LigastureSubst table splitting 660a4f8 [skera-repack] fix MarkBasePos table splitting aa57058 [skera-repack] make a copy of shared ClassDef before mutation 0d01f50 [skera-repack] make a copy of shared coverage table before mutation ad9d216 [skera] update remap_child() dd3b9cf [skera-repack] fix is_shared() c54a417 [skera] add copy_data option for duplicate_vertex() Changes for shared-brotli-patch-decoder from shared-brotli-patch-decoder-v0.1.4 to 0.1.5 8f295ea Lower MSRV to 1.85 and add MSRV CI check Changes for incremental-font-transfer from incremental-font-transfer-v0.6.0 to 0.7.0 8f295ea Lower MSRV to 1.85 and add MSRV CI check 40cd87d Remove support for IFT Patch Map Format 1.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The first commit replaces the nested traversal that detects the
clip transform* fillpattern with an explicit check, avoiding multiple traversals every time we encounter a clip paint.The second commit limits the total number of paints processed for a single glyph.
There's some unfortunate churn here to enable these changes:
ResolvedPaintso that they can be used from multiple call sites (the optimization check and the normal pass).traverse_with_callbackshave been moved into a newTraversalStatestruct. We need to track visited nodes and this reduces the parameter count while also giving us a nice place to stash the combined resolve + limit check.internal bug ref: b/537783593