[HarfBuzz] Arabic presentation forms and the uniscribe backend

[Jonathan Kew]

On 20/8/13 09:44, Behdad Esfahbod wrote:
> On 13-08-18 10:01 AM, Jonathan Kew wrote:
>> I'm seeing some problems with the uniscribe backend when using Arabic
>> presentation-form codepoints; in many cases, the glyph positioning seems to be
>> badly off.
>>
>> For example, see the Uniscribe rendering at
>>
>> http://ec2-54-226-13-158.compute-1.amazonaws.com/testcase-view.html?file=out/fonts/win8/arabtype.ttf/texts/arab/wikipedia/ur.txt.diffs#L_1_41824
>>
>> where there's a large gap between the last two glyphs. There are many similar
>> examples in the list there, too.
>>
>> This problem does NOT appear when using the same text and font in Firefox with
>> uniscribe shaping, which makes me suspect it's a glitch of some kind in the
>> harfbuzz-uniscribe backend.
>
>
> I tracked it down to this:
>
>    - If I switch back to using non-OpenType versions of
> ScriptItemize/Shape/Place, things work correctly,
>
>    - With the OpenType version of them, I'm getting back from ScriptItemize
> script 'latn' for the presentation forms but 'arab' for the normal characters.
>   So, that's one Uniscribe bug,
>
>    - I pass down that script code to Shape/Place, and I believe results in the
> generic shaper being chosen, which I guess ignores the fRTL flag of the
> analysis and always returns results as if base direction was LTR, while we
> expect RTL,
>
>    - Things go south from there.
>
> This may also explain some other issues I've been trying to understand, but I
> have to verify.
>
> Any idea how to address this best?  A hack would be to use the non-OpenType
> versions if no user features are provided.
>
> CC'ing Andrew.
>

Interesting analysis. I thought I'd tried using the non-OpenType 
functions, but still saw problems. But maybe I didn't hack that correctly.

AFAICT, explicitly setting the fLogicalOrder flag in the SCRIPT_ANALYSIS 
structure before calling Shape and Place will avoid the problem; it also 
actually serves to slightly simplify the code overall, as fewer places 
care about the 'backward' state, although it may be less efficient as we 
end up having to add an hb_buffer_reverse call.

Anyhow, the attached patch seems to be working well in my testing so far.

JK

-------------- next part --------------
diff --git a/src/hb-uniscribe.cc b/src/hb-uniscribe.cc
index a3a480a..d79dab1 100644
--- a/src/hb-uniscribe.cc
+++ b/src/hb-uniscribe.cc
@@ -832,9 +832,8 @@ retry:
   unsigned int glyphs_offset = 0;
   unsigned int glyphs_len;
   bool backward = HB_DIRECTION_IS_BACKWARD (buffer->props.direction);
-  for (unsigned int j = 0; j < item_count; j++)
+  for (unsigned int i = 0; i < item_count; i++)
   {
-    unsigned int i = backward ? item_count - 1 - j : j;
     unsigned int chars_offset = items[i].iCharPos;
     unsigned int item_chars_len = items[i + 1].iCharPos - chars_offset;
 
@@ -875,10 +874,13 @@ retry:
       }
     }
 
+    SCRIPT_ANALYSIS sa = items[i].a;
+    sa.fLogicalOrder = true;
+
   retry_shape:
     hr = funcs->ScriptShapeOpenType (font_data->hdc,
 				     &font_data->script_cache,
-				     &items[i].a,
+				     &sa,
 				     script_tags[i],
 				     language_tag,
 				     range_char_counts.array,
@@ -954,15 +956,9 @@ retry:
     uint32_t *p = &vis_clusters[log_clusters[buffer->info[i].utf16_index()]];
     *p = MIN (*p, buffer->info[i].cluster);
   }
-  if (!backward) {
-    for (unsigned int i = 1; i < glyphs_len; i++)
-      if (vis_clusters[i] == -1)
-	vis_clusters[i] = vis_clusters[i - 1];
-  } else {
-    for (int i = glyphs_len - 2; i >= 0; i--)
-      if (vis_clusters[i] == -1)
-	vis_clusters[i] = vis_clusters[i + 1];
-  }
+  for (unsigned int i = 1; i < glyphs_len; i++)
+    if (vis_clusters[i] == -1)
+      vis_clusters[i] = vis_clusters[i - 1];
 
 #undef utf16_index
 
@@ -996,10 +992,14 @@ retry:
 
     /* TODO vertical */
     pos->x_advance = info->mask;
-    pos->x_offset = info->var1.u32;
+    pos->x_offset = backward ? -info->var1.u32 : info->var1.u32;
     pos->y_offset = info->var2.u32;
   }
 
+  if (backward) {
+    hb_buffer_reverse (buffer);
+  }
+
   /* Wow, done! */
   return true;
 }