[FFmpeg-devel] aalib video filter patch review request
Michael Niedermayer
michaelni at gmx.at
Thu Jun 26 06:04:48 CEST 2014
On Mon, Jun 23, 2014 at 08:57:42PM +0400, iamtakingiteasy at eientei.org wrote:
> Hello, i have written (basing on drawtext) a simple video filter,
> which utilizes aalib to render input frames as ascii-art and
> rastarizes this ascii-art to output frames using freetype2 library.
> Also fontconfig can be used for font discovery.
>
> Attaching a patch implementing this filter.
>
> Here is example video:
>
> before:
> http://www.youtube.com/watch?v=44oBi1ikNZk
>
> and after applying this filter:
>
> ffmpeg -i disharmony_plain.mp4 -vcodec libx264 -preset slow -crf 18
> -pix_fmt yuv420p -vf 'scale=iw/3:ih/4,
> aa=fontname=terminus:fontsize=12:linespacing=1.0:contrast=50,
> pad=width=1920:x=(ow-iw)/2' -aspect 16:9 -acodec aac -strict -2 -f
> mp4 disharmony_aa_filter.mp4
>
> http://www.youtube.com/watch?v=LQytXqovLOM
>
> I hope for suggestions how to adopt this code in order to upstream it.
>
> This is one of my first steps on FOSS world and the first one
> towards ffmpeg suite, so it most likely will be needing a strong
> scepsis during review, because approaches i took most likely wasn't
> perfect ones to say the least.
>
> Thanks.
[...]
> +static void vf_driver_uninit(struct aa_context *context)
> +{
> +}
> +
> +static void vf_driver_setattr(aa_context *context, int attr)
> +{
> +
> +}
using null pointers does not work?, these empty functions are needed ?
[...]
> +static const AVOption aa_options[] = {
> + {"fontfile", "set font file", OFFSET(fontfile), AV_OPT_TYPE_STRING, {.str=NULL}, CHAR_MIN, CHAR_MAX, FLAGS},
> +#if CONFIG_LIBFONTCONFIG
> + {"fontname", "set font name", OFFSET(fontname), AV_OPT_TYPE_STRING, {.str="Monospace"}, CHAR_MIN, CHAR_MAX, FLAGS},
> + {"fontstyle", "set font style", OFFSET(fontstyle), AV_OPT_TYPE_STRING, {.str="Regular"}, CHAR_MIN, CHAR_MAX, FLAGS},
> +#endif
> + {"fgcolor", "set foreground color", OFFSET(fgcolor.rgba), AV_OPT_TYPE_COLOR, {.str="white"}, CHAR_MIN, CHAR_MAX, FLAGS},
> + {"bgcolor", "set background color", OFFSET(bgcolor.rgba), AV_OPT_TYPE_COLOR, {.str="black"}, CHAR_MIN, CHAR_MAX, FLAGS},
> + {"fontsize", "set font size", OFFSET(fontsize), AV_OPT_TYPE_DOUBLE, {.dbl=0.0}, 0, DBL_MAX, FLAGS},
> + {"linespacing", "set line spacing", OFFSET(linespacing), AV_OPT_TYPE_DOUBLE, {.dbl=1.2}, 0, DBL_MAX, FLAGS},
> + {"brightness", "set brightness", OFFSET(brightness), AV_OPT_TYPE_INT, {.i64=0}, 0, 255, FLAGS},
> + {"contrast", "set contrast", OFFSET(contrast), AV_OPT_TYPE_INT, {.i64=0}, 0, 127, FLAGS},
> + {"gamma", "set gamma", OFFSET(gamma), AV_OPT_TYPE_DOUBLE, {.dbl=1.0}, DBL_MIN, DBL_MAX, FLAGS},
> + {"inversion", "set inversion", OFFSET(inversion), AV_OPT_TYPE_INT, {.i64=0}, 0, 1, FLAGS},
> + {"aaflags", "aalib flags", OFFSET(aaflags), AV_OPT_TYPE_FLAGS, {.i64=0}, 0, INT_MAX, FLAGS, "aaflags"},
> + {"reverse", NULL, 0, AV_OPT_TYPE_CONST, {.i64=AA_REVERSE_MASK}, .flags = FLAGS, .unit = "aaflags"},
> + {"all", NULL, 0, AV_OPT_TYPE_CONST, {.i64=AA_ALL}, .flags = FLAGS, .unit = "aaflags"},
> + {"eight", NULL, 0, AV_OPT_TYPE_CONST, {.i64=AA_EIGHT}, .flags = FLAGS, .unit = "aaflags"},
> + {"extended", NULL, 0, AV_OPT_TYPE_CONST, {.i64=AA_EXTENDED}, .flags = FLAGS, .unit = "aaflags"},
> + {NULL}
> +};
these probably should also be documented in doc/filters.texi
[...]
> +static av_cold int init(AVFilterContext *ctx)
> +{
> + int err;
> + int i;
> + AAContext *s = ctx->priv;
> + Glyph *glyph;
> +
> + if (!s->fontfile && !CONFIG_LIBFONTCONFIG) {
> + av_log(ctx, AV_LOG_ERROR, "No font filename provided\n");
> + return AVERROR(EINVAL);
> + }
> +
> + err = FT_Init_FreeType(&s->library);
> +
> + if (err) {
> + av_log(ctx, AV_LOG_ERROR, "Could not load FreeType: %s\n", FT_ERRMSG(err));
> + return AVERROR(EINVAL);
> + }
> +
> + err = load_font(ctx);
> +
> + if (err) {
> + return err;
> + }
> +
> + if (s->fontsize == 0.0) {
> + s->fontsize = 10.0;
> + }
> +
> + err = FT_Set_Pixel_Sizes(s->face, 0, s->fontsize);
> +
> + if (err) {
> + av_log(ctx, AV_LOG_ERROR, "Could not set font size to %f pixels: %s\n", s->fontsize, FT_ERRMSG(err));
> + return AVERROR(EINVAL);
> + }
> +
> + for (i = 0; i < 256; i++) {
> + err = load_glyph(ctx, i, &glyph);
> + if (!err) {
> + s->xadvance = FFMAX(s->xadvance, glyph->bbox.xMax);
> + }
> + }
if you use/instanciate exactly 256 glyphs then av_tree makes no
sense and a array of 256 would be simpler and faster.
[...]
> +static int config_props_in(AVFilterLink *inlink)
> +{
> + AVFilterContext *ctx = inlink->dst;
> + AAContext *s = ctx->priv;
> +
> + ff_draw_init(&s->dc, inlink->format, 0);
> + ff_draw_color(&s->dc, &s->fgcolor, s->fgcolor.rgba);
> + ff_draw_color(&s->dc, &s->bgcolor, s->bgcolor.rgba);
> +
> + if (!s->aa) {
> + s->w = (inlink->w/2);
> + s->h = (inlink->h/2);
> +
> + s->aa_params.supported = s->aaflags | AA_NORMAL_MASK | AA_REVERSE_MASK;
> + s->aa_params.width = s->w;
> + s->aa_params.height = s->h;
> +
> + s->renderparams.bright = s->brightness;
> + s->renderparams.contrast = s->contrast;
> + s->renderparams.gamma = s->gamma;
> + s->renderparams.dither = AA_FLOYD_S;
why is this hardcoded, while other parameters can be choosen by
the user ?
also you might want to add yourself to the MAINTAINERs file
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140626/d5a29ac6/attachment.asc>
More information about the ffmpeg-devel
mailing list