[FFmpeg-devel] [PATCH 2/2] drawtext: Use libfribidi to correctly render Arabic text - Fixes ticket #3758
Nicolas George
george at nsup.org
Wed Jul 9 18:31:22 CEST 2014
Thanks for the patch. A first quick set of remarks.
Le primidi 21 messidor, an CCXXII, Marc Jeffreys a écrit :
> I've added this as a "fribidi=1" option to drawtext rather than enabling
> it by default, so as not to break anything.
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.
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.
> Difference can be seen by compiling with --enable-libfribidi and comparing:
I think you should merge the change in configure with this patch.
>
> ffplay -loglevel debug -f lavfi -i "color=color=white,drawtext=fontfile=/usr/share/fonts/dejavu/DejaVuSans.ttf:text=أحمد"
>
> ffplay -loglevel debug -f lavfi -i "color=color=white,drawtext=fribidi=1:fontfile=/usr/share/fonts/dejavu/DejaVuSans.ttf:text=أحمد"
>
> A slight note: This does create some -Wundef warnings on compilation, but they're from libfribidi itself rather than this code.
> I opted to leave them alone rather than start defining macros just to keep things quiet, but if necessary that can be changed.
>
> ---
> doc/filters.texi | 8 +++
> libavfilter/vf_drawtext.c | 137 ++++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 139 insertions(+), 6 deletions(-)
>
> diff --git a/doc/filters.texi b/doc/filters.texi
> index ada33a7..a9e7360 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -3653,6 +3653,8 @@ To enable compilation of this filter, you need to configure FFmpeg with
> @code{--enable-libfreetype}.
> To enable default font fallback and the @var{font} option you need to
> configure FFmpeg with @code{--enable-libfontconfig}.
> +To enable the @var{fribidi} option, you need to configure FFmpeg with
> + at code{--enable-libfribidi}.
>
> @subsection Syntax
>
> @@ -3707,6 +3709,9 @@ This parameter is mandatory if the fontconfig support is disabled.
> The font size to be used for drawing text.
> The default value of @var{fontsize} is 16.
>
> + 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.
> +
> @item ft_load_flags
> The flags to be used for loading the fonts.
>
> @@ -4010,6 +4015,9 @@ For more information about libfreetype, check:
> For more information about fontconfig, check:
> @url{http://freedesktop.org/software/fontconfig/fontconfig-user.html}.
>
> +For more information about libfribidi, check:
> + at url{http://fribidi.org/}.
> +
> @section edgedetect
>
> Detect and draw edges. The filter uses the Canny Edge Detection algorithm.
> diff --git a/libavfilter/vf_drawtext.c b/libavfilter/vf_drawtext.c
> index 0d829a6..ecdc569 100644
> --- a/libavfilter/vf_drawtext.c
> +++ b/libavfilter/vf_drawtext.c
> @@ -59,6 +59,10 @@
> #include "internal.h"
> #include "video.h"
>
> +#if CONFIG_LIBFRIBIDI
> +#include <fribidi.h>
> +#endif
> +
> #include <ft2build.h>
> #include FT_FREETYPE_H
> #include FT_GLYPH_H
> @@ -182,6 +186,9 @@ typedef struct DrawTextContext {
> int tc24hmax; ///< 1 if timecode is wrapped to 24 hours, 0 otherwise
> int reload; ///< reload text file for each frame
> int start_number; ///< starting frame number for n/frame_num var
> +#if CONFIG_LIBFRIBIDI
> + int fribidi; ///< 1 if using fribidi
> +#endif
> AVDictionary *metadata;
> } DrawTextContext;
>
> @@ -226,6 +233,10 @@ static const AVOption drawtext_options[]= {
> {"fix_bounds", "if true, check and fix text coords to avoid clipping", OFFSET(fix_bounds), AV_OPT_TYPE_INT, {.i64=1}, 0, 1, FLAGS},
> {"start_number", "start frame number for n/frame_num variable", OFFSET(start_number), AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX, FLAGS},
>
> +#if CONFIG_LIBFRIBIDI
> + {"fribidi", "pass text through libfribidi before rendering", OFFSET(fribidi), AV_OPT_TYPE_INT, {.i64=0}, 0, 1, FLAGS},
> +#endif
> +
> /* FT_LOAD_* flags */
> { "ft_load_flags", "set font loading flags for libfreetype", OFFSET(ft_load_flags), AV_OPT_TYPE_FLAGS, { .i64 = FT_LOAD_DEFAULT }, 0, INT_MAX, FLAGS, "ft_load_flags" },
> { "default", NULL, 0, AV_OPT_TYPE_CONST, { .i64 = FT_LOAD_DEFAULT }, .flags = FLAGS, .unit = "ft_load_flags" },
> @@ -482,6 +493,113 @@ static int load_textfile(AVFilterContext *ctx)
> return 0;
> }
>
> +static inline int is_newline(uint32_t c)
> +{
> + return c == '\n' || c == '\r' || c == '\f' || c == '\v';
> +}
> +
> +#if CONFIG_LIBFRIBIDI
> +static int fribidi_text(AVFilterContext *ctx)
> +{
> + DrawTextContext *s = ctx->priv;
> + uint8_t *tmp;
> + int ret = 0;
> + static FriBidiFlags flags = FRIBIDI_FLAGS_DEFAULT | \
> + FRIBIDI_FLAGS_ARABIC ;
> + FriBidiChar *unicodestr = NULL;
> + FriBidiStrIndex len;
> + FriBidiParType direction = FRIBIDI_PAR_ON;
> + FriBidiStrIndex line_start = 0;
> + FriBidiStrIndex line_end = 0;
> + FriBidiLevel *embedding_levels = NULL;
> + FriBidiArabicProp *ar_props = NULL;
> + FriBidiCharType *bidi_types = NULL;
> + FriBidiStrIndex i,j;
> +
> + len = strlen(s->text);
> + if (!(unicodestr = av_malloc(len * sizeof(*unicodestr)))) {
> + ret = AVERROR(ENOMEM);
> + goto out;
> + }
> + len = fribidi_charset_to_unicode(FRIBIDI_CHAR_SET_UTF8,
> + s->text, len, unicodestr);
> +
> + bidi_types = av_malloc(len * sizeof(*bidi_types));
> + if (!bidi_types) {
> + ret = AVERROR(ENOMEM);
> + goto out;
> + }
> +
> + fribidi_get_bidi_types(unicodestr, len, bidi_types);
> +
> + embedding_levels = av_malloc(len * sizeof(*embedding_levels));
> + if (!embedding_levels) {
> + ret = AVERROR(ENOMEM);
> + goto out;
> + }
> +
> + if (fribidi_get_par_embedding_levels(bidi_types, len, &direction,
> + embedding_levels) == 0) {
> + ret = AVERROR(ENOMEM);
> + goto out;
> + }
> +
> + ar_props = av_malloc(len * sizeof(*ar_props));
> + if (!ar_props) {
> + ret = AVERROR(ENOMEM);
> + goto out;
> + }
> +
> + fribidi_get_joining_types(unicodestr, len, ar_props);
> + fribidi_join_arabic(bidi_types, len, embedding_levels, ar_props);
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.
> + 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?
> + goto out;
> + }
> + line_start = line_end + 1;
> + }
> + }
> +
> + /* Remove zero-width fill chars put in by libfribidi */
> + for (i = 0, j = 0; i < len; i++)
> + if (unicodestr[i] != FRIBIDI_CHAR_FILL)
> + unicodestr[j++] = unicodestr[i];
> + 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 */
> + ret = AVERROR(ENOMEM);
> + goto out;
> + }
> +
> + s->text = tmp;
> + len = fribidi_unicode_to_charset(FRIBIDI_CHAR_SET_UTF8,
> + unicodestr, len, s->text);
> + ret = 0;
> +
> +out:
> + if(unicodestr)
> + av_free(unicodestr);
> +
> + if (embedding_levels)
> + av_free(embedding_levels);
> +
> + if (ar_props)
> + av_free(ar_props);
> +
> + if (bidi_types)
> + av_free(bidi_types);
Freeing NULL is legal, precisely in order to make these tests unnecessary.
> +
> + return ret;
> +}
> +#endif
> +
> static av_cold int init(AVFilterContext *ctx)
> {
> int err;
> @@ -509,6 +627,12 @@ static av_cold int init(AVFilterContext *ctx)
> return err;
> }
>
> +#if CONFIG_LIBFRIBIDI
> + if (s->fribidi)
> + if ((err = fribidi_text(ctx)) < 0)
> + return err;
> +#endif
> +
> if (s->reload && !s->textfile)
> av_log(ctx, AV_LOG_WARNING, "No file to reload\n");
>
> @@ -617,11 +741,6 @@ static av_cold void uninit(AVFilterContext *ctx)
> av_bprint_finalize(&s->expanded_text, NULL);
> }
>
> -static inline int is_newline(uint32_t c)
> -{
> - return c == '\n' || c == '\r' || c == '\f' || c == '\v';
> -}
> -
> static int config_input(AVFilterLink *inlink)
> {
> AVFilterContext *ctx = inlink->dst;
> @@ -1132,9 +1251,15 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
> DrawTextContext *s = ctx->priv;
> int ret;
>
> - if (s->reload)
> + if (s->reload) {
> if ((ret = load_textfile(ctx)) < 0)
> return ret;
> +#if CONFIG_LIBFRIBIDI
> + if (s->fribidi)
> + if ((ret = fribidi_text(ctx)) < 0)
> + return ret;
> +#endif
> + }
>
> s->var_values[VAR_N] = inlink->frame_count+s->start_number;
> s->var_values[VAR_T] = frame->pts == AV_NOPTS_VALUE ?
> --
> 1.8.3.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140709/58b19fcf/attachment.asc>
More information about the ffmpeg-devel
mailing list