[HarfBuzz] fixing graphite rtl integration

[Behdad Esfahbod]

On 15-06-15 11:45 PM, Martin Hosken wrote:
> Dear Behdad,
> 
> The enclosed patch fixes rtl integration of graphite in hb-graphite2. It handles the cluster reversals, etc. and has received a modicum of testing.

Can you give us examples of cases this fixes?

> The only differences I see between running text through graphite and through
graphite with hb is reordering of ltr text in an rtl run. But I think that
difference is correct in that hb doesn't expect to do bidi reordering and
assumes things are reordered first or called as sub runs with the correct
direction.

That's not good.  HarfBuzz shapes as if run direction was forced by higher
level.  Can you do that in Graphite?


> In code terms the patch only affects the code following the cluster analysis, which is unchanged. The first hunk fixes a memory management problem which valgrind spotted. The second hunk is the core of the change.

Re valgrind, again, I like to reproduce it before fixing it.



> TIA,
> Yours,
> Martin
> 
> diff --git a/src/hb-graphite2.cc b/src/hb-graphite2.cc
> index 6214403..6c2ea24 100644
> --- a/src/hb-graphite2.cc
> +++ b/src/hb-graphite2.cc
> @@ -359,6 +359,7 @@ _hb_graphite2_shape (hb_shape_plan_t    *shape_plan,
>      return false;
>    }
>  
> +  buffer->ensure (glyph_count);     // since sizeof(pos) > sizeof(cluster)+sizeof(codepoint)
>    scratch = buffer->get_scratch_buffer (&scratch_size);
>    while ((DIV_CEIL (sizeof (hb_graphite2_cluster_t) * buffer->len, sizeof (*scratch)) +
>  	  DIV_CEIL (sizeof (hb_codepoint_t) * glyph_count, sizeof (*scratch))) > scratch_size)
> @@ -433,31 +434,58 @@ _hb_graphite2_shape (hb_shape_plan_t    *shape_plan,
>    buffer->len = glyph_count;
>    //buffer->swap_buffers ();
>  
> -  if (HB_DIRECTION_IS_BACKWARD(buffer->props.direction))
> -    curradvx = gr_seg_advance_X(seg);
> -
> -  hb_glyph_position_t *pPos;
> -  for (pPos = hb_buffer_get_glyph_positions (buffer, NULL), is = gr_seg_first_slot (seg);
> -       is; pPos++, is = gr_slot_next_in_segment (is))
> +  if (!HB_DIRECTION_IS_BACKWARD(buffer->props.direction))
>    {
> -    pPos->x_offset = gr_slot_origin_X (is) - curradvx;
> -    pPos->y_offset = gr_slot_origin_Y (is) - curradvy;
> -    pPos->x_advance = gr_slot_advance_X (is, grface, grfont);
> -    pPos->y_advance = gr_slot_advance_Y (is, grface, grfont);
> -    if (HB_DIRECTION_IS_BACKWARD (buffer->props.direction))
> -      curradvx -= pPos->x_advance;
> -    pPos->x_offset = gr_slot_origin_X (is) - curradvx;
> -    if (!HB_DIRECTION_IS_BACKWARD (buffer->props.direction))
> +    hb_glyph_position_t *pPos;
> +    for (pPos = hb_buffer_get_glyph_positions (buffer, NULL), is = gr_seg_first_slot (seg);
> +         is; pPos++, is = gr_slot_next_in_segment (is))
> +    {
> +      pPos->x_offset = gr_slot_origin_X (is) - curradvx;
> +      pPos->y_offset = gr_slot_origin_Y (is) - curradvy;
> +      pPos->x_advance = gr_slot_advance_X (is, grface, grfont);
> +      pPos->y_advance = gr_slot_advance_Y (is, grface, grfont);
>        curradvx += pPos->x_advance;
> -    pPos->y_offset = gr_slot_origin_Y (is) - curradvy;
> -    curradvy += pPos->y_advance;
> -  }
> -  if (!HB_DIRECTION_IS_BACKWARD (buffer->props.direction))
> +      curradvy += pPos->y_advance;
> +    }
>      pPos[-1].x_advance += gr_seg_advance_X(seg) - curradvx;
> -
> -  if (HB_DIRECTION_IS_BACKWARD (buffer->props.direction))
> +  }
> +  else
> +  {
> +    //curradvx = gr_seg_advance_X (seg);
> +    //curradvy = gr_seg_advance_Y (seg);
> +    hb_glyph_position_t *pPos = hb_buffer_get_glyph_positions (buffer, NULL) + buffer->len - 1;
> +    const hb_glyph_info_t *info = buffer->info + buffer->len - 1;
> +    const hb_glyph_info_t *tinfo;
> +    const gr_slot *tis;
> +    int currclus = -1;
> +    float clusx = 0., clusy = 0.;
> +    for (is = gr_seg_last_slot (seg); is; pPos--, info--, is = gr_slot_prev_in_segment (is))
> +    {
> +      if (info->cluster != currclus)
> +      {
> +        curradvx += clusx;
> +        curradvy += clusy;
> +        currclus = info->cluster;
> +        clusx = 0.;
> +        clusy = 0.;
> +        for (tis = is, tinfo = info; tis && tinfo->cluster == currclus; tis = gr_slot_prev_in_segment (tis), tinfo--)
> +        {
> +          clusx += gr_slot_advance_X (tis, grface, grfont);
> +          clusy += gr_slot_advance_Y (tis, grface, grfont);
> +        }
> +        curradvx += clusx;
> +        curradvy += clusy;
> +      }
> +      pPos->x_advance = gr_slot_advance_X (is, grface, grfont);
> +      pPos->y_advance = gr_slot_advance_Y (is, grface, grfont);
> +      curradvx -= pPos->x_advance;
> +      curradvy -= pPos->y_advance;
> +      pPos->x_offset = gr_slot_origin_X (is) - curradvx;
> +      pPos->y_offset = gr_slot_origin_Y (is) - curradvy;
> +    }
>      hb_buffer_reverse_clusters (buffer);
> -
> +  }
> + 
>    if (feats) gr_featureval_destroy (feats);
>    gr_seg_destroy (seg);
>  
> _______________________________________________
> HarfBuzz mailing list
> HarfBuzz at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/harfbuzz
>