[FFmpeg-devel] [PATCH 2/2] drawtext: Use libfribidi to correctly render Arabic text - Fixes ticket #3758

[Marc Jeffreys]

Date: Wed, 9 Jul 2014 18:31:22 +0200
From: george at nsup.org
To: ffmpeg-devel at ffmpeg.org
Subject: Re: [FFmpeg-devel] [PATCH 2/2] drawtext: Use libfribidi to correctly render Arabic text - Fixes ticket #3758

> Do you have any idea about what it could break? IMHO, if ffmpeg is capable
> of correctly rendering Arabic text, it should do it by default.

The only case that comes to mind is if someone is reversing the arabic text
before sending it to ffmpeg, to counteract the existing issue. But you're
right; default-on would probably be better in the long-term.
 
> Also, I do not like the name: it is about the implementation and not the
> function. If we were to change the implementation (using another library),
> we would either have to change the option name or have an inconsistency.

Good point. I've been at a loss for names to to use, however. Perhaps, if
we default to using this, we could have something like "raw_text=1" to switch
the behaviour off.
 
> I think you should merge the change in configure with this patch.

I was following http://ffmpeg.org/developer.html#Development-Policy ,
point 5:

"Also do not forget that if part B depends on part A, but A does not
depend on B, then A can and should be committed first and separate from B."

Parts A and B being the configure patch and this patch, respectively.

But I'm happy to do a merge if necessary.
 
>> + at item fribidi
>> +If set to 1, pass the text through libfribidi before rendering. By default 0.
 
> I believe the doc should explain what it does to a normal, multimedia-savvy
> user, but not require knowledge of subtle free-software implementations of
> things that are only vaguely related to multimedia.

I'll see what I can do.
 
> Can you explain what scripts will be correctly rendered with this
> implementation?
>  
> If code has to be added for each script, then maybe using a more high-level
> library than fribidi would be better.


I'm not an expert in this field, but my understanding is:
 - All right-to-left/left-to-right reading issues should be resolved.
 - This won't fix any non-arabic scripts which require glyph substitution
      (But I don't think these are that common).
 - It may break some custom fonts (An ideal implementation would look at the
      Opentype layout tables in the font itself).

And yes, a better implementation might use something like harfbuzz,
but a small fix is perhaps better than none.  

>> +    fribidi_shape(flags, embedding_levels, len, ar_props, unicodestr);
>> +
>> +    for (line_end = 0, line_start = 0; line_end < len; line_end++) {
>> +        if (is_newline(unicodestr[line_end]) || line_end == len - 1) {
>> +            if (fribidi_reorder_line(flags, bidi_types, line_end - line_start,
>> +                                     line_start, direction, embedding_levels,
>> +                                     unicodestr, NULL) == 0) {
 
>> +                ret = AVERROR(ENOMEM);
 
> According to the doc, ENOMEM is the most likely but not the only
> possibility. Can you elaborate?

Looking through the source of fribidi-0.19.6, I can't actually see a case in which
this function ever fails. So I suppose we should go by the documentation, just to be safe.
 
> Freeing NULL is legal, precisely in order to make these tests unnecessary.

Noted. I'll fix that.

I won't be able to make any more changes to my code until tomorrow morning, but I
can try to respond to any technical points. I'll resubmit as soon as I can.