applib/graphics: add a minimal BiDi engine - #1972
Conversation
672aac0 to
569a334
Compare
gmarull
left a comment
There was a problem hiding this comment.
Automated correctness review of the BiDi engine. The engine itself held up well: fuzzing bidi_next_run() / bidi_reverse_run() over 300k random mixed-script strings under ASan/UBSan found no memory-safety issues and no failure to make progress, and W1/W4-W7/N1/N2/L4 produce correct output for brackets, 12:34, 50%, -5, 3.5, the Arabic comma and AN-vs-EN. The findings below are mostly in how text_layout.c consumes it.
|
|
||
| // L4: mirrored glyphs such as brackets face the other way inside | ||
| // an RTL run. | ||
| rcp = bidi_mirror_codepoint(rcp); |
There was a problem hiding this comment.
Regression: flag emoji inside an RTL run are drawn unfolded, so the measured width no longer matches what is drawn.
prv_class() classifies regional indicators as BidiClassON, so in "مرحبا 🇸🇦 وداعا" the neutrals resolve RTL on both sides and the whole string comes out as a single RTL run (checked against the engine directly).
Pass 1 measures that run through prv_shape_pair() -> emoji_shape_pair(), which folds the RI pair into one FLAG_CODEPOINT advance and sets skip_ligature_member. This draw loop reads codepoints straight out of rtl_buffer and never calls prv_shape_pair(), so the two regional indicators are rendered individually (missing/wildcard glyphs) and walked_width_px advances by both — everything after them on the line is shifted.
On main this worked: the old splitter treated an RI as neither punctuation nor RTL, so it broke the segment and the flag was drawn by the LTR branch, which does fold. The RI pairing added in bidi_reverse_run() preserves the pair's byte order but does not make this loop fold it.
There was a problem hiding this comment.
Fixed — the RTL draw loop now folds through emoji_shape_pair() and consumes the second indicator, matching pass 1 and the LTR branch.
Your read of the cause is exactly right, and I had this wrong: I had assumed the LTR branch did not fold either, so I treated it as pre-existing. It does fold (line 97), the RTL branch did not, and classifying regional indicators as ON is what moved the flag into the RTL run in the first place. Confirmed "مرحبا 🇸🇦 وداعا" resolves to a single RTL run here.
The pair ordering fix already in bidi_reverse_run() keeps the two indicators in order through the reversal, so folding after it produces the right flag rather than a mirrored country code.
| // Pass 2: Reorder the runs for the paragraph direction (UAX 9 L2) | ||
| // In an RTL paragraph the visual order of the runs is reversed so the | ||
| // first logical run appears on the right, where reading starts. | ||
| if (line_is_rtl && num_segments > 1) { |
There was a problem hiding this comment.
L2 reordering is skipped entirely for LTR paragraphs, so a number after an RTL run lands on the wrong side.
In an LTR paragraph an EN that follows an RTL run sits at level 2 nested inside the level-1 RTL region, and L2 moves it to the left of that region. Gating the reorder on line_is_rtl never does that.
Concrete case — "Total مرحبا 123" splits into L("Total "), R("مرحبا "), L("123") and renders as the glyph sequence
T o t a l ␣ ␣ ا ب ح ر م 1 2 3
i.e. two adjacent spaces, and the digits fused to the wrong side of the Arabic. Expected is Total 123 ابحرم.
This is inside the PR's stated scope — only "embedding levels above two" is listed as out of scope, and this is level 2. RTL paragraphs are fine, because there L and EN both resolve to is_rtl == false and bidi_next_run() merges them into one run, which is what makes the whole-array reversal correct in that direction.
There was a problem hiding this comment.
Fixed properly — runs now carry an embedding level (UAX 9 I1/I2, 0-2) instead of a direction bit, and pass 2 is real L2: from the highest level present down to the lowest odd level, reversing each contiguous group at that level or above.
Your scope point was the one that settled it. I had tried a cheaper version first that reconstructed the level-1 region by bridging any LTR run held between two RTL runs. It fixed your case and broke the ordinary one, because a bool cannot tell level-0 Latin from a level-2 number:
A مرحبا 12 وداعا Z -> A اعادو 12 ابحرم Z fixed
A مرحبا DEF وداعا Z -> A اعادو DEF ابحرم Z broken, was correct
With levels both hold:
Total مرحبا 123 -> [Total |L0] [مرحبا |L1] [123|L2] -> Total 123 ابحرم
A مرحبا DEF وداعا Z -> [A |L0] [مرحبا|L1] [ DEF |L0] [وداعا|L1] [ Z|L0]
This changes bidi_next_run() to report uint8_t *run_level. The PR text now lists I1/I2 and L2 as implemented.
| utf8_t *line_start = (utf8_t *)line->start; | ||
| utf8_t *line_end = (utf8_t *)text_box_params->utf8_bounds->end; | ||
| utf8_t *ptr = line_start; | ||
| const bool line_is_rtl = bidi_paragraph_is_rtl(line_start, line_end); |
There was a problem hiding this comment.
P2/P3 is resolved per visual line rather than per paragraph.
line_start is line->start, so for wrapped text this picks the first strong character on this line, not in the paragraph. An Arabic paragraph that wraps so that line 2 begins with an embedded English word gets line_is_rtl == false for that line: its runs are not reordered, and prv_line_justify() (same call at line 1388) left-aligns it while line 1 is right-aligned — the base direction flips mid-paragraph.
P2 is defined over the paragraph, so the direction wants resolving once from the paragraph start and carrying on Line / TextBoxParams. Pre-existing in shape (prv_utf8_starts_with_rtl() had the same problem), but bidi_paragraph_is_rtl() is the natural place to fix it now.
There was a problem hiding this comment.
Fixed. Paragraph direction now resolves from the start of the paragraph rather than the start of the visual line, at both call sites.
Rather than carry it on Line / TextBoxParams, prv_paragraph_start() scans back to the previous \n and hands that to bidi_paragraph_is_rtl(), which already stops at class B. \n cannot appear as a UTF-8 continuation byte so the scan is a plain byte walk, once per line. Same result as plumbing it through, without the struct change.
You are right that it was pre-existing in shape — prv_utf8_starts_with_rtl() had the same scope — but it is a small fix here and the alignment flip mid-paragraph is worth not shipping.
| return true; | ||
| } | ||
|
|
||
| bool bidi_is_needed(const utf8_t *start, const utf8_t *end) { |
There was a problem hiding this comment.
The gate is narrower than the class table, so some scripts prv_class() calls strong-RTL never reach the engine.
The byte scan covers U+0590-U+06FF only, but prv_class() returns BidiClassR for U+0700-U+08FF (Syriac, Thaana, NKo, Samaritan, Mandaic, Arabic Ext-A/B) and for the presentation forms U+FB1D-U+FDFF / U+FE70-U+FEFC.
Text made only of those — including pre-shaped Arabic presentation forms, which some servers send — returns false here, so it renders in logical order and is left-aligned by prv_line_justify(), even though the engine has full class data for it.
Either widen the scan (lead bytes 0xDC-0xDF for U+0700-U+07FF, plus the three-byte prefixes for U+0800-U+08FF and U+FB1D and up), or drop the unreachable R classifications so the two agree.
There was a problem hiding this comment.
Fixed by widening the scan rather than dropping the classifications, so the gate and the class table now agree in the direction that keeps the data useful.
Lead bytes added: 0xD7-0xDF for U+05C0-U+07FF, 0xE0 with 0xA0-0xA3 for U+0800-U+08FF, and 0xEF with 0xAC-0xB7 / 0xB9-0xBB for the presentation forms. Continuation bytes are 0x80-0xBF so none of them can match a lead tested here and the byte-at-a-time scan stays safe.
Dropping the R classifications was the other option you offered, but the presentation forms are the case that decided it: pre-shaped Arabic is real input, arabic_shape_text() emits U+FE70-U+FEFC itself, and classifying those as L would be wrong rather than merely unreachable. Verified Syriac, Thaana, NKo, Arabic Extended-A and both presentation-form blocks now pass the gate.
| //! Resolve the direction of the span starting at @p pos and report where it | ||
| //! ends. A span is one strong character, a number with its weak neighbours, or | ||
| //! a stretch of neutrals resolved together. | ||
| static bool prv_resolve_span(const utf8_t *line_start, utf8_t *pos, const utf8_t *end, |
There was a problem hiding this comment.
Neutral resolution re-scans the line for every span, which is super-linear on the render path.
Each neutral span calls prv_prev_side() (backward scan to line_start) and prv_next_side() (forward scan to end), and every EN either of them meets calls prv_number_follows_ltr(), which scans backward to line_start again.
Measured on an M-series host at -O2, splitting one 122-byte Arabic-plus-digit-groups line into runs costs ~80 us, vs ~6 us for a more typical mixed line. Scaled to a 64-80 MHz Cortex-M4 that is single-digit milliseconds per line, repeated on every render of the text. MAX_BIDI_SEGMENTS 16 caps the number of bidi_next_run() calls but not the rescans inside each one.
Carrying the last-strong direction forward while walking the line once would make this linear.
There was a problem hiding this comment.
The complexity claim is right and I can reproduce the shape of it. Measuring run splitting alone on an M-series host at -O2, us per byte across a doubling series:
shape n=2 n=4 n=8 n=16 us/byte
arabic + digits 0.41 0.91 1.92 3.92 flat 0.016 linear
digits + punct 0.40 1.29 4.52 15.53 0.020->0.149 super-linear
The digits + punct shape roughly doubles per doubling, so prv_prev_side() / prv_number_follows_ltr() walking back to line_start is the cost, as you describe.
I could not reproduce the magnitude. My worst constructed case is 15.5 us at 104 bytes against your 80 us at 122 bytes, about 5x apart. Could you share the line you measured? I would rather fix this against the shape that actually hurts than against my guess at it.
One thing that got worse, not better: resolving embedding levels for the L2 fix means every EN span now calls prv_number_follows_ltr() inside prv_resolve_span(), not only during neutral resolution. That took the same worst case from 10.9 us to 15.5 us. So this is more worth doing than when you raised it.
Carrying the last strong class forward is the right fix and it needs state across bidi_next_run() calls, so it changes the signature again. I would rather do that as a follow-up than reopen the interface a second time in this PR, but say the word if you would prefer it here.
There was a problem hiding this comment.
The shape is a Latin word followed by a list of numbers, with Arabic anywhere later in the paragraph so the line passes the gate:
"a " + "1 " * 55 + "مرحبا" 122 B 36.6 us
"a " + "1 " * 27 + "مرحبا" 66 B 9.8 us
"a " + "1 " * 14 + "مرحبا" 40 B 3.1 us
"a " + "1 " * 7 + "مرحبا" 26 B 1.1 us
"Score: 10 11 12 … 44 مرحبا" 122 B 21 us
M-series, -O2, run splitting only, on the current head. ~4x per doubling. Every space is a neutral whose prv_prev_side() and prv_next_side() both land on an EN, and each of those calls prv_number_follows_ltr(), which walks back over the whole prefix to the a. It is one run, so MAX_BIDI_SEGMENTS does not bound it, and a line is bounded only by the display width.
Carrying the last strong forward is the right fix and I am fine with it as a tracked follow-up: at ~120 B per line that is low single-digit milliseconds on an M4 per render, not a hang. One thing to weigh: a single-pass resolver with per-line state is also the natural place to do N0 (see the final-pass review), so it may be worth doing the two together rather than fitting N0 into the scanning design first.
There was a problem hiding this comment.
Thanks for the shape — reproduced it. Agree it goes to the follow-up, and agree N0 belongs in the same single-pass rewrite rather than being fitted into the scanning design first.
gmarull
left a comment
There was a problem hiding this comment.
Automated correctness review of the BiDi engine. The engine itself held up well: fuzzing bidi_next_run() / bidi_reverse_run() over 300k random mixed-script strings under ASan/UBSan found no memory-safety issues and no failure to make progress, and W1/W4-W7/N1/N2/L4 produce correct output for brackets, 12:34, 50%, -5, 3.5, the Arabic comma and AN-vs-EN. The findings below are mostly in how text_layout.c consumes it.
569a334 to
63cfe7e
Compare
gmarull
left a comment
There was a problem hiding this comment.
Final pass, on top of the first round. The four fixes hold up: I re-verified the RI folding, the level-based L2, the paragraph-start P2/P3 and the widened gate. The branch also rebases cleanly onto current main and the text layout, iterator and image-fixture tests all pass there.
This time the engine was checked differentially: bidi_next_run() + the L2 reorder from text_layout.c + bidi_reverse_run(), compared against the unicode-bidi reference (via python-bidi) and against Unicode's BidiCharacterTest.txt. Hebrew mixed with Latin, numbers, separators, terminators and neutrals: 0 mismatches in 80k random lines. The deviations are all in three places, below, and the remaining findings go with them.
1. N0 is absent, and that regresses main for a common shape
A Latin word followed by a parenthetical inside an RTL paragraph draws both parentheses as opening ones:
رسالة من John (Work) → (John (Work نم ةلاسر
مرحبا iPhone (15) وداعا → اعادو (iPhone (15 ابحرم
הודעה מ John (Work) → (John (Work מ העדוה
مرحبا (hello) world وداعا → اعادو hello) world) ابحرم
( sits between L and L and resolves L; ) sits between L and R (or eos) and takes the paragraph direction, so it is reversed to the far side and mirrored. On main the old splitter never split on punctuation, so John (Work) was one LTR segment and came out right. Contact and app names with a parenthetical are the practical case, so I don't think the "N1/N2 already give both halves the same direction in the cases that arise in practice" argument survives; in BidiCharacterTest.txt 14.5k of 91.5k bracket cases fail. N0 restricted to the ten pairs already in the mirror table would cover it. If it goes to the follow-up instead, the claim in the description and commit message should go now.
2. W2 is missing: Arabic letters are classed R, not AL
After an Arabic letter a European number becomes an Arabic number, so terminators and +/- do not bind to it:
الحرارة 25% → 25% ةرارحلا spec / phone: %25 ةرارحلا
من 10-12 مساء → ءاسم 10-12 نم spec / phone: ءاسم 12-10 نم
الرصيد $50 → $50 ديصرلا spec / phone: 50$ ديصرلا
This is inherited from main's weak-LTR digit rule and the PR preserves it deliberately, but it is what the phone renders differently for the primary script. Hebrew is unaffected (R, not AL).
3. W4 is applied together with W5
A separator after a terminator joins the number, so 50%-60% is one run; per spec W4 runs first and the - is a neutral. 16 of 40k random Hebrew lines, so minor, but it falls out of the same patch.
Patch for 2 and 3
Attached below. It adds BidiClassAL, replaces prv_number_follows_ltr()'s scan with prv_prev_strong() (feeds both W2 and W7), gives prv_scan_number() an after_arabic flag plus an after_digit flag for the W4/W5 order, and steps over a terminator run in one go in the neutral stretch (the extra back-scan would otherwise make مرحبا %%%…1 quadratic). Verified: 0 mismatches across 200k random lines on every alphabet, ASan/UBSan clean on 60k corrupted-UTF-8 inputs, +124 B on cortex-m4 -Os. Only number_keeps_its_terminator needed changing (it encoded the non-standard case; it is retargeted to Hebrew and W2 / W4-order tests are added).
bidi-w2-w4w5.patch (apply with git apply on the PR head)
diff --git a/src/fw/applib/graphics/bidi.c b/src/fw/applib/graphics/bidi.c
index 618df2feb..3624c53c7 100644
--- a/src/fw/applib/graphics/bidi.c
+++ b/src/fw/applib/graphics/bidi.c
@@ -17,6 +17,7 @@
typedef enum {
BidiClassL, // Strong left-to-right
BidiClassR, // Strong right-to-left
+ BidiClassAL, // Strong right-to-left Arabic letter, turns a following EN into AN
BidiClassEN, // European number
BidiClassAN, // Arabic-Indic number
BidiClassES, // European separator, binds two European numbers
@@ -126,17 +127,24 @@ static BidiClass prv_class(Codepoint cp) {
if (cp == 0x0609 || cp == 0x060A || cp == 0x066A) { // Per mille, per ten thousand, percent
return BidiClassET;
}
- return BidiClassR;
+ return BidiClassAL;
}
if (cp >= 0x0700 && cp <= 0x08FF) { // Syriac, Thaana, NKo, Arabic Extended-A/B
// These blocks carry Arabic number signs among their letters.
if (cp == 0x0890 || cp == 0x0891 || cp == 0x08E2) {
return BidiClassAN;
}
- return BidiClassR;
+ // NKo, Samaritan and Mandaic are R; the rest are Arabic-script letters.
+ if (cp >= 0x07C0 && cp <= 0x085F) {
+ return BidiClassR;
+ }
+ return BidiClassAL;
}
- if ((cp >= 0xFB1D && cp <= 0xFDFF) || (cp >= 0xFE70 && cp <= 0xFEFC)) {
- return BidiClassR; // Hebrew and Arabic presentation forms
+ if (cp >= 0xFB1D && cp <= 0xFB4F) {
+ return BidiClassR; // Hebrew presentation forms
+ }
+ if ((cp >= 0xFB50 && cp <= 0xFDFF) || (cp >= 0xFE70 && cp <= 0xFEFC)) {
+ return BidiClassAL; // Arabic presentation forms
}
switch (cp) {
case 0x00A0: // No-break space
@@ -185,18 +193,20 @@ static BidiClass prv_class_at(const utf8_t *pos, const utf8_t *end, utf8_t **nex
//! Direction a class contributes when a neutral looks at it (N1). Numbers act
//! as right-to-left for this purpose even though they are laid out the other way.
static bool prv_side_is_rtl(BidiClass cls) {
- return (cls == BidiClassR) || (cls == BidiClassEN) || (cls == BidiClassAN);
+ return (cls == BidiClassR) || (cls == BidiClassAL) || (cls == BidiClassEN) ||
+ (cls == BidiClassAN);
}
static bool prv_class_has_side(BidiClass cls) {
return (cls == BidiClassL) || prv_side_is_rtl(cls);
}
-//! W7: a European number takes the direction of the strong character before it.
-//! Returns true when that character is left-to-right, so the number stops acting
-//! as right-to-left towards the neutrals around it.
-static bool prv_number_follows_ltr(const utf8_t *line_start, const utf8_t *pos,
- const utf8_t *end, bool para_is_rtl) {
+//! Strong class before @p pos (L, R or AL), or the class the start of the
+//! paragraph stands for when nothing strong precedes it. Feeds W2, which turns
+//! a European number after an Arabic letter into an Arabic number, and W7,
+//! which gives one after Latin text that text's direction.
+static BidiClass prv_prev_strong(const utf8_t *line_start, const utf8_t *pos,
+ const utf8_t *end, bool para_is_rtl) {
utf8_t *cur = (utf8_t *)pos;
while (cur > line_start) {
cur = utf8_get_previous((utf8_t *)line_start, cur);
@@ -208,15 +218,29 @@ static bool prv_number_follows_ltr(const utf8_t *line_start, const utf8_t *pos,
if (next == NULL) {
break;
}
- if (cls == BidiClassL) {
- return true;
+ if ((cls == BidiClassL) || (cls == BidiClassR) || (cls == BidiClassAL)) {
+ return cls;
}
- if ((cls == BidiClassR) || (cls == BidiClassB)) {
- return false;
+ if (cls == BidiClassB) {
+ break;
}
}
- // Nothing strong precedes it, so the paragraph direction decides.
- return !para_is_rtl;
+ return para_is_rtl ? BidiClassR : BidiClassL;
+}
+
+//! W7: true when the strong character before a European number is left-to-right,
+//! so the number stops acting as right-to-left towards the neutrals around it.
+static bool prv_number_follows_ltr(const utf8_t *line_start, const utf8_t *pos,
+ const utf8_t *end, bool para_is_rtl) {
+ return prv_prev_strong(line_start, pos, end, para_is_rtl) == BidiClassL;
+}
+
+//! W2: true when the strong character before a European number is an Arabic
+//! letter, which makes it an Arabic number: separators still bind (W4) but
+//! terminators (W5) and the plus and minus signs no longer do.
+static bool prv_number_follows_arabic(const utf8_t *line_start, const utf8_t *pos,
+ const utf8_t *end, bool para_is_rtl) {
+ return prv_prev_strong(line_start, pos, end, para_is_rtl) == BidiClassAL;
}
//! Direction the class at @p pos contributes to a neighbouring neutral, with W7
@@ -283,30 +307,43 @@ static utf8_t *prv_skip_terminators(utf8_t *pos, const utf8_t *end) {
}
//! Consume a number along with the separators and terminators that bind to it
-//! (UAX 9 W4-W6). @p pos must point at a number of class @p num_cls.
-static utf8_t *prv_scan_number(utf8_t *pos, const utf8_t *end, BidiClass num_cls) {
+//! (UAX 9 W4-W6). @p pos must point at a digit of class @p num_cls. After an
+//! Arabic letter (@p after_arabic) W2 makes European digits Arabic numbers, so
+//! both kinds of digit join and the Arabic-number rules apply.
+static utf8_t *prv_scan_number(utf8_t *pos, const utf8_t *end, BidiClass num_cls,
+ bool after_arabic) {
+ const bool european = (num_cls == BidiClassEN) && !after_arabic;
utf8_t *cur = pos;
+ bool after_digit = true;
while (cur < end && *cur != '\0') {
utf8_t *next = NULL;
BidiClass cls = prv_class_at(cur, end, &next);
if (next == NULL) {
break;
}
- if (cls == num_cls || cls == BidiClassNSM) {
+ const bool is_digit = (cls == num_cls) ||
+ (after_arabic && ((cls == BidiClassEN) || (cls == BidiClassAN)));
+ if (is_digit || cls == BidiClassNSM) {
+ after_digit = true;
cur = next;
continue;
}
- // W4: a separator surrounded by numbers of the same class joins them.
- if ((cls == BidiClassCS) || (cls == BidiClassES && num_cls == BidiClassEN)) {
+ // W4: a separator surrounded by numbers of the same class joins them. W4
+ // runs before W5, so a separator that follows a terminator does not.
+ if ((cls == BidiClassCS) || (cls == BidiClassES && european)) {
utf8_t *after = NULL;
- if (prv_class_at(next, end, &after) == num_cls && after != NULL) {
+ const BidiClass after_cls = prv_class_at(next, end, &after);
+ const bool after_is_digit = (after_cls == num_cls) ||
+ (after_arabic && ((after_cls == BidiClassEN) || (after_cls == BidiClassAN)));
+ if (after_digit && after_is_digit && after != NULL) {
cur = after;
continue;
}
break;
}
// W5: terminators next to a European number join it.
- if (cls == BidiClassET && num_cls == BidiClassEN) {
+ if (cls == BidiClassET && european) {
+ after_digit = false;
cur = next;
continue;
}
@@ -369,21 +406,24 @@ static bool prv_resolve_span(const utf8_t *line_start, utf8_t *pos, const utf8_t
*span_end = next;
return true;
case BidiClassR:
+ case BidiClassAL:
*span_level = 1;
*span_end = next;
return true;
case BidiClassAN:
// Arabic numbers sit one level inside their surroundings either way.
*span_level = 2;
- *span_end = prv_scan_number(pos, end, cls);
+ *span_end = prv_scan_number(pos, end, cls,
+ prv_number_follows_arabic(line_start, pos, end, para_is_rtl));
return true;
- case BidiClassEN:
+ case BidiClassEN: {
// W7 already turned a number after Latin text into L, so it belongs at
// the paragraph's own level rather than nested inside an RTL region.
- *span_level = prv_number_follows_ltr(line_start, pos, end, para_is_rtl) ?
- prv_level_for_dir(false, para_is_rtl) : 2;
- *span_end = prv_scan_number(pos, end, cls);
+ const BidiClass strong = prv_prev_strong(line_start, pos, end, para_is_rtl);
+ *span_level = (strong == BidiClassL) ? prv_level_for_dir(false, para_is_rtl) : 2;
+ *span_end = prv_scan_number(pos, end, cls, strong == BidiClassAL);
return true;
+ }
case BidiClassB:
// A paragraph separator stands on its own at the paragraph direction.
*span_level = prv_level_for_dir(para_is_rtl, para_is_rtl);
@@ -393,14 +433,16 @@ static bool prv_resolve_span(const utf8_t *line_start, utf8_t *pos, const utf8_t
break;
}
- // A terminator run directly ahead of a European number belongs to it (W5).
+ // A terminator run directly ahead of a European number belongs to it (W5),
+ // unless W2 made that number Arabic.
if (cls == BidiClassET) {
utf8_t *number = prv_skip_terminators(pos, end);
utf8_t *after = NULL;
- if (prv_class_at(number, end, &after) == BidiClassEN && after != NULL) {
+ if (prv_class_at(number, end, &after) == BidiClassEN && after != NULL &&
+ !prv_number_follows_arabic(line_start, number, end, para_is_rtl)) {
*span_level = prv_number_follows_ltr(line_start, number, end, para_is_rtl) ?
prv_level_for_dir(false, para_is_rtl) : 2;
- *span_end = prv_scan_number(number, end, BidiClassEN);
+ *span_end = prv_scan_number(number, end, BidiClassEN, false);
return true;
}
}
@@ -417,9 +459,12 @@ static bool prv_resolve_span(const utf8_t *line_start, utf8_t *pos, const utf8_t
if (stretch_cls == BidiClassET) {
utf8_t *number = prv_skip_terminators(stretch_end, end);
utf8_t *after = NULL;
- if (prv_class_at(number, end, &after) == BidiClassEN && after != NULL) {
+ if (prv_class_at(number, end, &after) == BidiClassEN && after != NULL &&
+ !prv_number_follows_arabic(line_start, number, end, para_is_rtl)) {
break;
}
+ // The whole terminator run is neutral, so step over it at once.
+ stretch_next = number;
}
stretch_end = stretch_next;
}
@@ -494,7 +539,7 @@ bool bidi_paragraph_is_rtl(const utf8_t *start, const utf8_t *end) {
if (cls == BidiClassL) {
return false;
}
- if (cls == BidiClassR) {
+ if ((cls == BidiClassR) || (cls == BidiClassAL)) {
return true;
}
ptr = next;
diff --git a/tests/fw/test_bidi.c b/tests/fw/test_bidi.c
index 786ebf507..6bf2a00fe 100644
--- a/tests/fw/test_bidi.c
+++ b/tests/fw/test_bidi.c
@@ -252,12 +252,56 @@ void test_bidi__number_keeps_its_separator(void) {
}
void test_bidi__number_keeps_its_terminator(void) {
+ // Hebrew is R rather than AL, so the digits stay European and W5 binds the
+ // percent sign to them.
static const ExpectedRun expected[] = {
{ 0, 3, true },
{ 3, 3, false }, // "50%"
{ 6, 3, true },
};
- prv_assert_runs("م 50% م", expected, 3);
+ prv_assert_runs("ם 50% ם", expected, 3);
+}
+
+void test_bidi__number_after_arabic_letter_is_arabic(void) {
+ // W2: after an Arabic letter a European number becomes an Arabic number, so
+ // a terminator (W5) or the plus and minus signs (W4) no longer bind to it and
+ // "50%" is drawn as "%50", the way the phone shows it.
+ static const ExpectedRun percent[] = {
+ { 0, 3, true },
+ { 3, 2, false }, // "50"
+ { 5, 4, true }, // "% م"
+ };
+ prv_assert_runs("م 50% م", percent, 3);
+
+ static const ExpectedRun range[] = {
+ { 0, 3, true },
+ { 3, 2, false }, // "10"
+ { 5, 1, true }, // "-"
+ { 6, 2, false }, // "12"
+ { 8, 3, true },
+ };
+ prv_assert_runs("م 10-12 م", range, 5);
+
+ // Common separators still bind (W4), and so do Arabic-Indic digits.
+ static const ExpectedRun joined[] = {
+ { 0, 3, true },
+ { 3, 7, false }, // "12:٣٠"
+ { 10, 3, true },
+ };
+ prv_assert_runs("م 12:٣٠ م", joined, 3);
+}
+
+void test_bidi__separator_after_terminator_does_not_bind(void) {
+ // W4 runs before W5, so a separator that follows a terminator is a neutral
+ // even though the terminator itself joins the number.
+ static const ExpectedRun expected[] = {
+ { 0, 3, true },
+ { 3, 3, false }, // "50%"
+ { 6, 1, true }, // "-"
+ { 7, 3, false }, // "60%"
+ { 10, 3, true },
+ };
+ prv_assert_runs("ם 50%-60% ם", expected, 5);
}
void test_bidi__trailing_emoji_joins_the_arabic_run(void) {Nit
bidi.h still says "W4-W6" and "L2 (run reordering, done by the caller)"; stale against what is implemented now.
| stretch_end = next; | ||
| } | ||
|
|
||
| // N1: neutrals between two runs of the same direction take that direction. |
There was a problem hiding this comment.
This is where the John (Work) case goes wrong: ( is between L and L and resolves L, ) is between L and R and takes the paragraph direction, so the pair splits. See finding 1 in the review body.
There was a problem hiding this comment.
Confirmed, exactly as you describe — ( J o h n _ ( W o r k …. The claim that N1/N2 covered this in practice was wrong and is gone from the PR. Deferred to the follow-up with the resolver rewrite, per your note on the perf thread.
| if (cp == 0x0609 || cp == 0x060A || cp == 0x066A) { // Per mille, per ten thousand, percent | ||
| return BidiClassET; | ||
| } | ||
| return BidiClassR; |
There was a problem hiding this comment.
Arabic letters are AL in UAX 9, not R. The only rule that tells them apart is W2 (a European number after AL becomes AN), but that is the rule that puts %, $, #, ° and +/- on the correct side of a number in Arabic text. The attached patch adds BidiClassAL here and in the 0x0700-0x08FF and presentation-form ranges.
There was a problem hiding this comment.
Applied as-is. AL now covers U+0600, Arabic Extended, and the Arabic presentation forms; Hebrew, NKo, Samaritan and Mandaic stay R. Verified الحرارة 25% → %25 and שלום 25% unchanged.
| cur = next; | ||
| continue; | ||
| } | ||
| // W4: a separator surrounded by numbers of the same class joins them. |
There was a problem hiding this comment.
W4 runs before W5, so a separator that follows a terminator does not see a number on its left: 50%-60% is EN EN ON EN EN, not one number. The patch tracks after_digit here.
There was a problem hiding this comment.
In via the patch. Verified ם 50%-60% ם splits into two numbers with a neutral - between them.
RTL rendering reverses the codepoints of each right-to-left run but never
mirrors the glyphs that call for it, so a bracket keeps facing the same
way after the reversal and "(مرحبا)" draws as ")مرحبا(". Neutral
characters are attached to whichever run happens to be open rather than
resolved against their neighbours, so the brackets around an embedded
Latin word can land in different runs and be treated inconsistently. A
combining mark is reversed away from the letter it attaches to.
Replace rtl_support.c with bidi.c, a subset of Unicode UAX 9 sized for a
single line of watch text: P2/P3 for the paragraph direction, W1 so a
mark takes the class of the character it follows, W4-W7 so a number keeps
the separators and terminators that belong to it, N1/N2 for neutrals,
I1/I2 and L2 so runs carry an embedding level and reorder by it, and L4
for mirrored glyphs. Arabic letters are AL, so a European number after
one becomes an Arabic number (W2) and terminators and signs no longer bind
to it, matching what the phone renders. Bracket pairs (N0), explicit
directional controls, isolates and embedding levels above two are out of
scope; N0 and a linear neutral resolver are tracked as a follow-up.
The weak-LTR digit and numeric separator behaviour already in
rtl_support.c is preserved, but moves from a special case inside the
reversal into the run classification: European and Arabic-Indic numbers
resolve to their own left-to-right run, which is the level UAX 9 assigns
them in either paragraph direction. Resolving neutrals also removes the
need to peel trailing spaces into their own segment, so
rtl_segment_content_end() goes away with it.
bidi_is_needed() gates the whole path on a raw byte scan covering every
block the classifier calls strong right-to-left, so the gate and the class
table agree and nothing is right-aligned while still being drawn in
logical order. Text with none of those bytes costs one comparison per byte
and keeps taking the existing left-to-right path, unchanged.
Runs carry an embedding level rather than a direction bit, because a
number following a right-to-left run sits one level deeper than the text
around it: without the level, "Total <arabic> 123" cannot be told apart
from ordinary Latin and L2 leaves the digits on the wrong side of the
Arabic. Paragraph direction is resolved from the start of the paragraph
rather than the start of the visual line, so a wrapped paragraph does not
change base direction part way down.
The run splitter in text_layout.c collapses into one bidi_next_run() call
and mirroring is applied in both the width pass and the draw pass so the
two agree. Lam-alef ligature shaping and the segment cap are untouched.
test_rtl_support.c is replaced by test_bidi.c, which keeps its digit and
separator coverage expressed as run boundaries rather than as reversal
output.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Khalid Nuaim <i@kalua.im>
63cfe7e to
61c55c3
Compare
|
Patch applied verbatim on top of the previous head, then rebased onto current N0 goes to the follow-up, paired with the single-pass resolver as you suggest — the claim is removed from the description and the commit message, and the The differential rig against |
gmarull
left a comment
There was a problem hiding this comment.
Final pass on 61c55c3. Approving.
Verified on this head:
bidi.candtest_bidi.care byte-identical to the previous head plus the W2/W4-W5 patch once both sides go through clang-format 22, and every other file in the PR is unchanged apart from what the rebase picked up frommain(the interdiff is empty). Formatting is clean and it still merges cleanly onto currentmain.- Differential run against
python-bidi/unicode-bidi, 40k random lines per alphabet: 0 mismatches on Arabic, Hebrew, extended digits, separators, terminators and neutrals. The only remaining deviations are bracket pairs, which is the N0 follow-up called out in the description;BidiCharacterTest.txtis 77081/91588 with every failure in that bucket. - The three W2 shapes render the spec way (
%25 ةرارحلا,ءاسم 12-10 نم,50$ ديصرلا) andם 50%-60% םsplits at the-. Hebrew output is unchanged. - ASan/UBSan clean on 60k corrupted-UTF-8 inputs through
bidi_next_run()/bidi_reverse_run()/bidi_mirror_codepoint(). - cortex-m4
-Os: 2223 B → 2347 B, the +124 B the description states. - Full unit test suite on this head: 333/333 pass.
The bidi.h summary now matches what is implemented, and the description and commit message no longer claim N0. The quadratic a 1 1 1 … مرحبا case is unchanged (36 us at 122 B) and goes to the follow-up with N0 as agreed.
Following the discussion in #1631 (comment) — mirroring paired brackets on its own does not cover mixed-direction text, so this does the reordering properly.
Non-RTL text never reaches the engine
bidi_is_needed()gates the whole path on a raw byte scan over the same range the previousutf8_contains_rtl()covered. Text with none of those bytes costs one comparison per byte, never decodes UTF-8, and keeps taking the existing left-to-right path. No behaviour change for Latin-only text.What it fixes
(مرحبا)draws as)مرحبا(.mainhas no mirroring at all.مرحبا abc 123 defrenders the embedded phrase asdef 123 abc.\ninto the following paragraph.What it preserves
The weak-LTR digit and numeric-separator handling already in
rtl_support.cis kept, not added. It moves from a special case inside the reversal into the run classification: European and Arabic-Indic numbers resolve to their own left-to-right run, which is the level UAX 9 assigns them in either paragraph direction.٢٠٢٦and١٢:٣٤behave as they do today. One visible change for Arabic:25%after an Arabic word now renders%25, as the phone does (W2).Resolving neutrals properly also removes the need to peel trailing spaces into a separate segment, so
rtl_segment_content_end()goes away with it. Lam-alef ligature shaping and the segment cap are untouched.Scope
Implemented: P2/P3 (paragraph direction), W1 (marks), W2 and W4–W7 (Arabic letters are AL, so a following European number becomes Arabic and its terminators and signs no longer bind; numbers keep their separators), N1/N2 (neutrals), I1/I2 and L2 (embedding levels 0–2 and run reordering), L4 (mirroring, 10 pairs), class B (paragraph separators).
Not implemented: bracket pairs (N0), explicit directional controls (LRM/RLM/RLE/PDF), isolates, embedding levels above two, the full ~380-character mirror set. N0 and a linear single-pass neutral resolver are tracked as a follow-up; until then a Latin word with a parenthetical inside an RTL paragraph (
رسالة من John (Work)) draws both brackets as opening ones.Changes
rtl_support.cbecomesbidi.c, rewritten. The run splitter intext_layout.ccollapses into a singlebidi_next_run()call, and mirroring is applied in both the width pass and the draw pass so the two agree.About 2.3 KB of flash (
.text2132 B +.rodata90 B on cortex-m4-Os, plus 124 B for the W2 change per its author's measurement), no RAM, no allocation. It replacesrtl_support.cat 550 B andtext_layout.cis net 24 lines smaller, so the firmware grows by roughly 1.8 KB.Testing
test_rtl_support.cis replaced bytests/fw/test_bidi.c— 34 cases. The digit and separator coverage from the old file is kept, re-expressed as run boundaries rather than reversal output, alongside bracket mirroring in both paragraph directions, combining marks across every block that carries them,\nas a paragraph boundary, regional-indicator pairing, and the fast-path gate. All pass, clean under ASan and UBSan.