[FFmpeg-devel] [PATCH] Add drawtext filter from the libavfilter soc repo.

[Stefano Sabatini]

On date Saturday 2011-02-19 15:21:13 +0000, M?ns Rullg?rd encoded:
> Stefano Sabatini <stefano.sabatini-lala at poste.it> writes:
[...]
> > +    /* load ASCII chars */
> > +    for (code = 0; code < 256; code++)
> > +        load_glyph(ctx, code, &y_min, &y_max);
> 
> ASCII ends at 127.  The remainder up to 255 is latin1 aka 8859-1.
> 
> > +    /* load all the other non-ASCII chars presented in the UTF-8 sequence */
> > +    for (p = dtext->text; *p; ) {
> > +        GET_UTF8(code, *p++, continue;);
> > +        load_glyph(ctx, code, &y_min, &y_max);
> > +    }
> 
> Why do you preload the entire latin1 character set?  I think it would be
> better to load glyphs on demand as they are encountered while
> rendering.  That would save memory and speed up lookup (since the tree
> will be shallower).  Your current version will also fail if the strftime
> expansion introduces non-latin1 characters not used elsewhere in the
> format string.  On-demand loading would fix that as well.

Yes I was aware of the problem, now it should be fixed, all the glyphs
are loaded and cached lazily when found for the first time in the
expanded string.

> > +    dtext->text_height = y_max - y_min;
> > +    dtext->baseline    = y_max;
> > +
> > +#if !HAVE_LOCALTIME_R
> > +    av_log(ctx, AV_LOG_WARNING, "strftime() expansion unavailable!\n");
> > +#endif
> > +
> > +    return 0;
> > +}
> > +
> > +static int query_formats(AVFilterContext *ctx)
> > +{
> > +    /* FIXME: Add support for other formats */
> > +    enum PixelFormat pix_fmts[] = {
> 
> static const

Fixed.

> > +        PIX_FMT_YUV420P, PIX_FMT_YUV444P, PIX_FMT_YUV422P,
> > +        PIX_FMT_YUV411P, PIX_FMT_YUV410P,
> > +        PIX_FMT_YUV440P, PIX_FMT_NONE
> > +    };
> > +
> > +    avfilter_set_common_formats(ctx, avfilter_make_format_list(pix_fmts));
> > +    return 0;
> > +}
> > +
> > +static int glyph_enu_free(void *opaque, void *elem)
> > +{
> > +    av_free(elem);
> > +    return 0;
> > +}
> > +
> > +static av_cold void uninit(AVFilterContext *ctx)
> > +{
> > +    DrawTextContext *dtext = ctx->priv;
> > +    av_freep(&dtext->fontfile);
> > +    av_freep(&dtext->text);
> > +    av_freep(&dtext->fgcolor_string);
> > +    av_freep(&dtext->bgcolor_string);
> > +    av_tree_enumerate(dtext->glyphs, NULL, NULL, glyph_enu_free);
> > +    av_tree_destroy(dtext->glyphs);
> > +    dtext->glyphs = 0;
> > +    FT_Done_Face(dtext->face);
> > +    FT_Done_FreeType(dtext->library);
> > +}
> 
> Do those FT functions handle null pointers?

Yes, checked, they do at least with my version of libfreetype (but not
documented in the FT API).

> [...]
> 
> > +static int draw_text(AVFilterContext *ctx, AVFilterBufferRef *picref,
> > +                     int width, int height)
> > +{
> > +    DrawTextContext *dtext = ctx->priv;
> > +    FT_Face face = dtext->face;
> > +    char *text = dtext->text;
> > +    uint32_t code = 0;
> > +    int x = 0, y = 0, i = 0;
> > +    uint8_t *p;
> > +    int str_w, str_w_max;
> > +    FT_Vector delta;
> > +    Glyph *glyph = NULL, *prev_glyph = NULL;
> > +    Glyph dummy = { 0 };
> > +    Glyph *glyph0 = av_tree_find(dtext->glyphs, &dummy, (void *)glyph_cmp, NULL);
> > +
> > +#if HAVE_LOCALTIME_R
> > +    time_t now = time(0);
> > +    struct tm ltime;
> > +    size_t expanded_text_len;
> > +
> > +    dtext->expanded_text[0] = '\1';
> > +    expanded_text_len = strftime(dtext->expanded_text, MAX_EXPANDED_TEXT_SIZE,
> > +                                 text, localtime_r(&now, &ltime));
> > +
> > +    if (expanded_text_len == 0 && dtext->expanded_text[0] != '\0') {
> 
> This isn't bullet-proof.  The buffer contents are unspecified if the
> return value is zero.  It is safe insofar an unterminated string will
> never escape, but it doesn't necessarily catch all errors.  Consider
> this remark merely as such.

Uhm OK, blame the fools who designed the strftime interface...

> > +        av_log(ctx, AV_LOG_WARNING,
> > +               "String expanded by strftime() is too big");
> > +        return AVERROR(EINVAL);
> > +    }
> > +    text = dtext->expanded_text;
> > +#endif
> > +
> > +    /* measure text size and save glyphs position */
> > +    str_w = str_w_max = 0;
> > +    x = dtext->x;
> > +    y = dtext->y;
> > +
> > +    for (i = 0, p = text; *p; i++) {
> > +        GET_UTF8(code, *p++, continue;);
> > +
> > +        /* get glyph */
> > +        prev_glyph = glyph;
> > +        dummy.code = code;
> > +        glyph = av_tree_find(dtext->glyphs, &dummy, (void *)glyph_cmp, NULL);
> > +        if (!glyph)
> > +            glyph = glyph0;
> > +
> > +        /* kerning */
> > +        if (dtext->use_kerning && prev_glyph && glyph->code) {
> > +            FT_Get_Kerning(dtext->face, prev_glyph->code, glyph->code,
> > +                           ft_kerning_default, &delta);
> > +            x += delta.x >> 6;
> > +        }
> > +
> > +        if (x + glyph->advance >= width || code == '\n' || code == '\r') {
> > +            if (code != '\n' || code != '\r')
> > +                str_w_max = width - dtext->x - 1;
> > +            y += dtext->text_height;
> > +            x = dtext->x;
> > +        }
> 
> This will double-space windows-style lines.  You could probably get away
> with ignoring \r and advancing the line on \n only.  A strict solution
> that also works with old-style Mac (\r only) lines is more complex, but
> we don't run on such systems anyway.

Should be fixed, I tested with \n and \r\n, should also work with \r.

[...]
> > +    /* draw glyphs */
> > +    for (i = 0, p = text; *p; i++) {
> > +        Glyph dummy = { 0 };
> > +        GET_UTF8(code, *p++, continue;);
> > +
> > +        /* skip new line char, just go to new line */
> > +        if (code == '\n' || code == '\r')
> > +            continue;
> > +
> > +        dummy.code = code;
> > +        glyph = av_tree_find(dtext->glyphs, &dummy, (void *)glyph_cmp, NULL);
> > +        if (!glyph)
> > +            glyph = glyph0;
> > +
> > +        draw_glyph(picref, &glyph->bitmap,
> > +                   dtext->positions[i].x, dtext->positions[i].y, width, height,
> > +                   dtext->fgcolor, dtext->bgcolor, dtext->outline,
> > +                   dtext->hsub, dtext->vsub);
> > +
> > +        /* increment pen position */
> > +        x += face->glyph->advance.x >> 6;
> > +    }
> 
> You might want to do something clever with the tab character.  I don't
> really have a good suggestion though.

Skipped for now, we could add an option for setting the tab size.

Other changes: I *removed* the code for rendering the font outline,
the result was honestly rather poor, this also allowed to make more
clear the interface (fgcolor -> fontcolor, bgcolor -> boxcolor).

Again on the interface side, I wonder if it is a good idea to keep the
ftload flags, the only useful flag which I can discern amongst the
many flags is +monochrome, but it may be enabled with monochrome=1 in
the options string.
Comments?

Still missing: docs update and RGB support, it should not be too
complicate to add it (but I can eventually do it as a separate patch).
-- 
FFmpeg = Faithful Free Multipurpose Picky Ermetic Glue