Ship feature/sdf-text-rendering - #126
Conversation
) Phase 1 of SDF glyph atlas integration: Canvas API: - Add fill_text_sdf(text, x, y, atlas) to Canvas interface - Default implementation falls back to fill_text() for CPU backends SkiaCanvas GPU implementation: - Converts text to codepoints, looks up each in the SdfAtlas - Draws textured quads via drawImageRect with linear filtering - Alpha-channel atlas (kAlpha_8) renders with the current fill color - Scale-independent: one atlas renders crisp at any font size - Glyph positioning respects bearing + advance from SdfGlyph metrics - Text alignment (left/center/right) uses atlas metrics The SdfAtlas infrastructure (Felzenszwalb-Huttenlocher EDT, shelf-based packing, glyph metrics) was built in a prior PR. This wires it into the rendering pipeline. Remaining phases: - Phase 2: SkSL smoothstep shader for sharper edges + outlines - Phase 3: DrawBatcher integration for single-draw-call text - Phase 4: Multi-channel SDF (MSDF) for thin strokes at small sizes - Phase 5: Automatic atlas management in the frame pipeline
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8595900b8a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| else if (c < 0xE0) { cp = (c & 0x1F) << 6 | (text[i+1] & 0x3F); i += 2; } | ||
| else if (c < 0xF0) { cp = (c & 0x0F) << 12 | (text[i+1] & 0x3F) << 6 | (text[i+2] & 0x3F); i += 3; } | ||
| else { cp = (c & 0x07) << 18 | (text[i+1] & 0x3F) << 12 | (text[i+2] & 0x3F) << 6 | (text[i+3] & 0x3F); i += 4; } |
There was a problem hiding this comment.
This decoder reads continuation bytes (text[i+1], text[i+2], text[i+3]) without checking that those indices are still within text.size(). If the input contains truncated or otherwise malformed UTF-8, fill_text_sdf can read past the end of the string and trigger undefined behavior (including crashes under ASAN) during rendering. Guard the remaining length before each multibyte decode path and handle invalid sequences safely (e.g., fallback glyph or fill_text).
Useful? React with 👍 / 👎.
| const SdfGlyph* g = atlas.glyph(cp); | ||
| if (!g) continue; |
There was a problem hiding this comment.
The new API contract says SDF rendering should fall back to fill_text() when the atlas does not contain required glyphs, but this implementation silently skips missing glyphs via continue. With a partial atlas (common in subset builds), characters disappear and alignment/advance become incorrect instead of using the existing text path. Detect any missing glyph and switch to fill_text for the full string.
Useful? React with 👍 / 👎.
1. measure_text uses SkShaper when shaping is enabled (#125 review) - Aligned with fill_text: both now use HarfBuzz for consistent kerning/ligature widths. Fixes draw-vs-measure mismatch for strings like "AV", "ffi". 2. SDF text: UTF-8 bounds checking + missing glyph fallback (#126 review) - Guard multibyte decode against truncated input (no UB on malformed) - Fall back to fill_text() if any glyph is missing from the atlas rather than silently skipping characters 3. Remove duplicate accessibility JNI from gpu_surface_android.cpp (#127 review) - Old code had wrong role mappings (toggle=3, label=2, meter=1) - New code in core/view/platform/android/accessibility_android.cpp uses C++ enum values directly (correct) - Eliminates linker duplicate symbol conflict on Android 4. Add MIDI include path for Android audio target (#128 review) - core/midi/include added to pulp-audio's include directories in PulpAndroid.cmake so oboe_device.cpp can find buffer.hpp and android_midi_fifo.hpp
1. measure_text uses SkShaper when shaping is enabled (#125 review) - Aligned with fill_text: both now use HarfBuzz for consistent kerning/ligature widths. Fixes draw-vs-measure mismatch for strings like "AV", "ffi". 2. SDF text: UTF-8 bounds checking + missing glyph fallback (#126 review) - Guard multibyte decode against truncated input (no UB on malformed) - Fall back to fill_text() if any glyph is missing from the atlas rather than silently skipping characters 3. Remove duplicate accessibility JNI from gpu_surface_android.cpp (#127 review) - Old code had wrong role mappings (toggle=3, label=2, meter=1) - New code in core/view/platform/android/accessibility_android.cpp uses C++ enum values directly (correct) - Eliminates linker duplicate symbol conflict on Android 4. Add MIDI include path for Android audio target (#128 review) - core/midi/include added to pulp-audio's include directories in PulpAndroid.cmake so oboe_device.cpp can find buffer.hpp and android_midi_fifo.hpp
1 participant
Automated by Shipyard