perf: Improve font drawing performance#978
perf: Improve font drawing performance#978daveallie merged 7 commits intocrosspoint-reader:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactored GfxRenderer's character rendering to use a template-based approach via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
jpirnay
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/GfxRenderer/GfxRenderer.cpplib/GfxRenderer/GfxRenderer.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cppcheck
- GitHub Check: build
🔇 Additional comments (5)
lib/GfxRenderer/GfxRenderer.h (1)
39-40: LGTM!The signature change from
const int*toint*aligns with the new template-based implementation. SincerenderCharis a private method, this change has no external API impact.lib/GfxRenderer/GfxRenderer.cpp (4)
50-154: Good use of templates for compile-time optimization.The
if constexprapproach withTextRotationenum effectively eliminates runtime branching for rotation handling. Hoisting theis2Bitcheck outside the pixel loops (line 90 vs. inner loop) is the key optimization that achieves the reported 11-22% performance improvement.
200-226: LGTM!The
drawTextfunction correctly delegates to the template-based rendering path. The removal ofconstfromyPosaligns with the signature change, though the value remains unmodified in the normal rendering path.
874-901: Clean refactoring of rotated text rendering.Replacing the inline rendering logic with a call to
renderCharImpl<TextRotation::Rotated90CW>consolidates the implementation and ensures both normal and rotated paths share the same glyph rendering logic.
1006-1009: LGTM!Clean delegation to the template implementation for the normal rendering path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/GfxRenderer/GfxRenderer.cpp`:
- Line 109: The expression computing bmpVal is ambiguous due to operator
precedence: change the calculation so the 2-bit value is masked first and then
subtracted from 3; e.g., compute the extracted bits as (byte >> bit_index) & 0x3
(or parenthesize directly: 3 - ((byte >> bit_index) & 0x3)) and assign bmpVal
from that to make the intent explicit in GfxRenderer.cpp (variable bmpVal).
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/GfxRenderer/GfxRenderer.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: cppcheck
🔇 Additional comments (3)
lib/GfxRenderer/GfxRenderer.cpp (3)
212-216: LGTM: mutable y cursor aligns with new renderChar signature.
919-930: LGTM: rotated path now shares the optimized glyph renderer.
994-1005: No review notes for this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/GfxRenderer/GfxRenderer.cpp`:
- Around line 1029-1032: The implementation of renderChar has a mismatched
parameter type for y; update the function signature in GfxRenderer::renderChar
to use const int* y (matching the header declaration) so it matches the declared
interface and the call to renderCharImpl<TextRotation::None>(*this, renderMode,
fontFamily, cp, x, y, pixelState, style); ensure the parameter name and const
qualifier exactly match the header's declaration for renderChar to avoid
linkage/compilation errors.
- Around line 80-90: renderCharImpl currently reads the glyph bitmap directly
via fontData->bitmap (using offset) which bypasses compressed-font handling;
replace that logic by calling renderer.getGlyphBitmap(fontData, glyph), check
for a null return and return early if null, and then use the returned pointer
for subsequent rendering so compressed fonts (fontData->groups) are handled
correctly.
## Summary * ``renderChar`` checked ``is2Bit`` on every pixel inside the inner loop, even though the value is constant for the lifetime of a single glyph * Moved the branch above both loops so each path (2-bit antialiased / 1-bit monochrome) runs without a per-pixel conditional * Eliminates redundant work in the two inner loops that render font glyphs to the frame buffer, targeting ``renderChar`` and ``drawTextRotated90CW`` in ``GfxRenderer.cpp`` ## Additional Context * Measured on device using a dedicated framebuffer benchmark (no display refresh). 100 repetitions of "The quick brown fox jumps". | Test | Before | After | Change | |-----------------|-----------------|-----------------|---------| | drawText UI12 | 1,337 µs/call | 1,024 µs/call | −23%| | drawText Bookerly14 | 2.174 µs / call | 1,847 µs/call | −15% | --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? _**PARTIALLY**_ Claude did the analysis and wrote the benchmarks
## Summary * ``renderChar`` checked ``is2Bit`` on every pixel inside the inner loop, even though the value is constant for the lifetime of a single glyph * Moved the branch above both loops so each path (2-bit antialiased / 1-bit monochrome) runs without a per-pixel conditional * Eliminates redundant work in the two inner loops that render font glyphs to the frame buffer, targeting ``renderChar`` and ``drawTextRotated90CW`` in ``GfxRenderer.cpp`` ## Additional Context * Measured on device using a dedicated framebuffer benchmark (no display refresh). 100 repetitions of "The quick brown fox jumps". | Test | Before | After | Change | |-----------------|-----------------|-----------------|---------| | drawText UI12 | 1,337 µs/call | 1,024 µs/call | −23%| | drawText Bookerly14 | 2.174 µs / call | 1,847 µs/call | −15% | --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? _**PARTIALLY**_ Claude did the analysis and wrote the benchmarks
4 participants
Summary
renderCharcheckedis2Biton every pixel inside the inner loop, even though the value is constant for the lifetime of a single glyphrenderCharanddrawTextRotated90CWinGfxRenderer.cppAdditional Context
AI Usage
While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it
helps set the right context for reviewers.
Did you use AI tools to help write this code? PARTIALLY Claude did the analysis and wrote the benchmarks