[FFmpeg-devel] [PATCH v2] drawtext: Add basic text shaping using libfribidi - Fixes ticket #3758

[Andrey Utkin]

2014-07-10 13:47 GMT+03:00 Marc Jeffreys <maj160 at live.co.uk>:
> Changes since last time:
> I've made the changes to configure, and squashed the patches together.
> Option changed from fribidi=1 (default 0) to text_shaping=1 (default 1).
> (Ideas for better names are definitely welcome.)
> Hopefully I've made the documentation more understandable.
> No longer testing for NULL before av_free.

This shouldn't go in commit message, consider using "---8<---"
scissors marker for annotation to mailing list in the beginning of
email, before actual commit message. BTW in this case empty commit
message is fine in my opinion, also maintainers can come up with
something.

> +    static FriBidiFlags flags = FRIBIDI_FLAGS_DEFAULT       | \
> +                                FRIBIDI_FLAGS_ARABIC        ;

Backslash is not needed, you can just break the line. Spacing is not
needed too, i think. I am not sure what is the position of bitwise OR
operator more following the style - at the new line, or on first line.

> +    bidi_types = av_malloc(len * sizeof(*bidi_types));
> +    if (!bidi_types) {
> +        ret = AVERROR(ENOMEM);

In this function, you have set ret = 0 in the beginning, and then set
to AVERROR(ENOMEM) literally everywhere. You can save the lines by
setting it to AVERROR(ENOMEM) at the beginning, i think. However,
that's not that important

> +    if (!fribidi_get_par_embedding_levels(bidi_types, len, &direction,
> +                                         embedding_levels)) {

Second line has one char less of spacing than everywhere else in the patch.

> +    len = j;
> +
> +    if (!(tmp = av_realloc(s->text, (len * 4 + 1) * sizeof(*s->text)))) {
> +    /* Use len * 4, as one unicode character can be up to 4 bytes in UTF-8 */

Comment not indented.

I have tested this patch, both with "--enable-libfribidi" and without.
Compiles without warnings and works as expected.

LGTM. Especially I like the fact that it handles both cases
(left-to-right and right-to-left) by itself, without extra parameters
from user.

-- 
Andrey Utkin