[FFmpeg-devel] [PATCH] video stabilization plugins using vid.stab library
Clément Bœsch
ubitux at gmail.com
Fri Mar 29 02:48:06 CET 2013
On Thu, Mar 28, 2013 at 11:24:11PM +0100, Georg Martius wrote:
> Hi,
>
> I changed the interface of my library to have a prefixe for every function and
> type.
> Attached is the patch against master (I used git rebase, thanks) so you can
> forget the previous patches.
>
> Regards,
> Georg
>
> On Saturday 23 March 2013 13:28:55 Michael Niedermayer wrote:
> > On Tue, Mar 19, 2013 at 12:43:39AM +0100, Georg Martius wrote:
> > > I changed what you suggested and added also parts to the configure script
> > > and Changelog.
> > >
> > > I am somehow to stupid to get git squash my changes into one patch file,
> > > prob. because I didn't do them in a separate branch. I hope you can work
> > > with the
> > you can reorder and squash things using git rebase -i (see the manual)
> >
> > [...]
> From 9e8ace6a91b7bb23c83c4e2dbe1e563fcaf1ec3b Mon Sep 17 00:00:00 2001
> From: Georg Martius <georg.martius at web.de>
> Date: Thu, 28 Mar 2013 22:59:16 +0100
> Subject: [PATCH] video stabilization plugins using vid.stab library with
> (configure and renamed interface)
>
>
> Signed-off-by: Georg Martius <georg.martius at web.de>
> ---
> Changelog | 1 +
> configure | 6 +
> libavfilter/Makefile | 2 +
> libavfilter/allfilters.c | 2 +
> libavfilter/vf_stabilize.c | 336 +++++++++++++++++++++++++++++++++++++
> libavfilter/vf_transform.c | 401 ++++++++++++++++++++++++++++++++++++++++++++
> 6 files changed, 748 insertions(+)
> create mode 100644 libavfilter/vf_stabilize.c
> create mode 100644 libavfilter/vf_transform.c
>
[...]
> @@ -2111,10 +2113,12 @@ removelogo_filter_deps="avcodec avformat swscale"
> scale_filter_deps="swscale"
> smartblur_filter_deps="gpl swscale"
> showspectrum_filter_deps="avcodec rdft"
> +stabilize_filter_deps="gpl libvidstab"
> stereo3d_filter_deps="gpl"
> subtitles_filter_deps="avformat avcodec libass"
> super2xsai_filter_deps="gpl"
> tinterlace_filter_deps="gpl"
> +transform_filter_deps="gpl libvidstab"
> yadif_filter_deps="gpl"
> pixfmts_super2xsai_test_deps="super2xsai_filter"
> tinterlace_merge_test_deps="tinterlace_filter"
> @@ -3532,6 +3536,7 @@ die_license_disabled_gpl() {
>
Both your wrappers and the lib have to be mentioned in the LICENSE file if
they are under GPL. Note that I did a recent change to that file so you
should rebase before doing the change.
Also note that even though the library is GPL, your wrappers in FFmpeg
don't necessarily need to be (that's for instance the case with libx264).
Since they are unusable without the library anyway, it's not a problem;
OTOH, if the project get relicensed at some point (or if you allow a
commercial license for example), having the FFmpeg wrappers in GPL where
potentially several other developers contributed might lead to an uneasy
situation. Of course, it's perfectly rightful if you want the wrappers to
also be GPL code; I just wanted to point that out.
> die_license_disabled gpl libcdio
> die_license_disabled gpl libutvideo
> +die_license_disabled gpl libvidstab
> die_license_disabled gpl libx264
> die_license_disabled gpl libxavs
> die_license_disabled gpl libxvid
> @@ -3967,6 +3972,7 @@ enabled libtwolame && require libtwolame twolame.h twolame_init -ltwolame &&
> die "ERROR: libtwolame must be installed and version must be >= 0.3.10"; }
> enabled libutvideo && require_cpp utvideo "stdint.h stdlib.h utvideo/utvideo.h utvideo/Codec.h" 'CCodec*' -lutvideo -lstdc++
> enabled libv4l2 && require_pkg_config libv4l2 libv4l2.h v4l2_ioctl
> +enabled libvidstab && require libvidstab vid.stab/libvidstab.h vsMotionDetectInit -lvidstab
Note: it would be nice if your library could provide the necessary files
for pkg-config.
[...]
> diff --git a/libavfilter/vf_stabilize.c b/libavfilter/vf_stabilize.c
> new file mode 100644
> index 0000000..c075c4e
> --- /dev/null
> +++ b/libavfilter/vf_stabilize.c
> @@ -0,0 +1,336 @@
> +/*
> + * vf_stabilize.c
> + *
> + * Copyright (C) Georg Martius - Jan 2012
> + * georg dot martius at web dot de
> + *
> + * This file is part of vid.stab, video deshaking lib
> + *
> + * vid.stab is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2, or (at your option)
> + * any later version.
> + *
> + * vid.stab is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with GNU Make; see the file COPYING. If not, write to
> + * the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
> + *
> + */
> +
> +/* Typical call: (This will visualize some internals in the video)
> + * ffmpeg -i input -vf stabilize=shakiness=5:show=1 dummy.avi
> + * all parameters are optional
> + */
> +
This IMO belongs to the documentation.
> +#define DEFAULT_RESULT_NAME "transforms.trf"
> +
> +#include "libavutil/common.h"
> +#include "libavutil/opt.h"
> +#include "libavutil/imgutils.h"
> +// #include "libavcodec/dsputil.h"
> +#include "avfilter.h"
> +#include "internal.h"
> +
> +#include <vid.stab/libvidstab.h>
> +
System headers should be above the others.
> +/* private date structure of this filter*/
> +typedef struct _stab_data {
> + const AVClass* class;
> +
> + VSMotionDetect md;
> + AVFilterBufferRef *ref; ///< Previous frame
> +
I think you don't need this anymore. Otherwise your code needs some
changes.
> + char* args;
> + char* result;
> + FILE* f;
> +} StabData;
> +
> +
> +/* ** Commandline options *** */
> +
> +#define OFFSET(x) offsetof(StabData, x)
> +#define OFFSETMD(x) (offsetof(StabData, md)+offsetof(VSMotionDetect, x))
offsetof(StabData.md, x) doesn't work?
> +#define FLAGS AV_OPT_FLAG_FILTERING_PARAM|AV_OPT_FLAG_VIDEO_PARAM
> +
> +static const AVOption stabilize_options[]= {
> + {"result", "path to the file used to write the transforms (def:transforms.trf)", OFFSET(result), AV_OPT_TYPE_STRING, {.str = DEFAULT_RESULT_NAME}},
> + {"shakiness", "how shaky is the video and how quick is the camera?\n"
> + " 1: little (fast) 10: very strong/quick (slow) (def: 5)", OFFSETMD(shakiness), AV_OPT_TYPE_INT, {.i64 = 5}, 1, 10, FLAGS},
> + {"accuracy", "(>=shakiness) 1: low 15: high (slow) (def: 9)", OFFSETMD(accuracy), AV_OPT_TYPE_INT, {.i64 = 9 }, 1, 15, FLAGS},
> + {"stepsize", "region around minimum is scanned with 1 pixel resolution (def: 6)", OFFSETMD(stepSize), AV_OPT_TYPE_INT, {.i64 = 6}, 1, 32, FLAGS},
> + {"mincontrast", "below this contrast a field is discarded (0-1) (def: 0.3)", OFFSETMD(contrastThreshold), AV_OPT_TYPE_DOUBLE, {.dbl = 0.25}, 0.0, 1.0, FLAGS},
> + {"show", "0: draw nothing (def); 1,2: show fields and transforms", OFFSETMD(show), AV_OPT_TYPE_INT, {.i64 = 0}, 0, 2, FLAGS},
> + {"tripod", "virtual tripod mode (if >0): motion is compared to a reference\n"
> + " reference frame (frame # is the value) (def: 0)", OFFSETMD(virtualTripod), AV_OPT_TYPE_INT, {.i64 = 0}, 0, INT_MAX, FLAGS},
You should avoid cutting the description strings: it will break automatic
output printing (see ffmpeg -help full)
> + {NULL},
> +};
> +
> +AVFILTER_DEFINE_CLASS(stabilize);
> +
> +/* ** some conversions from avlib to vid.stab constants and functions *** */
> +
> +/** convert AV's pixelformat to vid.stab pixelformat */
> +static VSPixelFormat AV2VSPixelFormat(AVFilterContext *ctx, enum AVPixelFormat pf){
> + switch(pf){
> + case AV_PIX_FMT_YUV420P: return PF_YUV420P;
> + case AV_PIX_FMT_YUV422P: return PF_YUV422P;
> + case AV_PIX_FMT_YUV444P: return PF_YUV444P;
> + case AV_PIX_FMT_YUV410P: return PF_YUV410P;
> + case AV_PIX_FMT_YUV411P: return PF_YUV411P;
> + case AV_PIX_FMT_YUV440P: return PF_YUV440P;
> + case AV_PIX_FMT_YUVA420P: return PF_YUVA420P;
> + case AV_PIX_FMT_GRAY8: return PF_GRAY8;
> + case AV_PIX_FMT_RGB24: return PF_RGB24;
> + case AV_PIX_FMT_BGR24: return PF_BGR24;
> + case AV_PIX_FMT_RGBA: return PF_RGBA;
> + default:
> + av_log(ctx, AV_LOG_ERROR, "cannot deal with pixel format %i!\n", pf);
> + return PF_NONE;
> + }
> +}
You have all sort of tabs here, it can't be pushed as such.
> +
> +/// struct to hold a valid context for logging from within vid.stab lib
> +typedef struct {
> + const AVClass* class;
> +} StabLogCtx;
> +
> +/** wrapper to log vs_log into av_log */
> +static int av_log_wrapper(int type, const char* tag, const char* format, ...){
> + va_list ap;
> + StabLogCtx ctx;
> + ctx.class = &stabilize_class;
This might lead to uninitialized read. Also, your logging wrapper should
definitely support an opaque context like most of the libs do.
> + av_log(&ctx, type, "%s: ", tag);
> + va_start (ap, format);
> + av_vlog(&ctx, type, format, ap);
> + va_end (ap);
> + return VS_OK;
> +}
> +
> +/** sets the memory allocation function and logging constants to av versions */
> +static void setMemAndLogFunctions(void){
style+: we avoid playingWithTheCase for functions; also we tend to break
the line after the prototype
> + vs_malloc = av_malloc;
> + vs_zalloc = av_mallocz;
> + vs_realloc = av_realloc;
> + vs_free = av_free;
> +
Accessing globals like this will cause some problems in the future; it is
recommended to write helpers for this in your library. Not necessarily
functions; you can have a struct with all the callbacks and write only one
function.
> + VS_ERROR_TYPE = AV_LOG_ERROR;
> + VS_WARN_TYPE = AV_LOG_WARNING;
> + VS_INFO_TYPE = AV_LOG_INFO;
> + VS_MSG_TYPE = AV_LOG_VERBOSE;
> +
> + vs_log = av_log_wrapper;
> +
> + VS_ERROR = 0;
> + VS_OK = 1;
> +}
> +
> +
> +/*************************************************************************/
> +
> +/* Module interface routines and data. */
> +
> +/*************************************************************************/
> +
> +static av_cold int init(AVFilterContext *ctx, const char *args)
> +{
> + StabData* sd = ctx->priv;
> + setMemAndLogFunctions();
> +
> + sd->class = &stabilize_class;
> + av_opt_set_defaults(sd); // the default values are overwritten by initMotiondetect later
> +
> + av_log(ctx, AV_LOG_VERBOSE, "stabilize filter: init %s\n", LIBVIDSTAB_VERSION);
> +
> + // save args for later, because the initialization of the vid.stab library requires
> + // knowledge about the input.
> + if(args)
> + sd->args=av_strdup(args);
The if() is not necessary.
> +
> + return 0;
> +}
> +
> +static av_cold void uninit(AVFilterContext *ctx)
> +{
> + StabData *sd = ctx->priv;
> + VSMotionDetect* md = &(sd->md);
> +
> + av_opt_free(sd);
> + if (sd->f) {
> + fclose(sd->f);
> + sd->f = NULL;
> + }
> +
> + vsMotionDetectionCleanup(md);
> + av_free(sd->result); // <- is this already free'd by av_opt_free?
Yes. You can check with valgrind.
> + av_free(sd->args);
> +}
> +
> +static int query_formats(AVFilterContext *ctx)
> +{
> + // If you add something here also add it to the above mapping function
> + static const enum AVPixelFormat pix_fmts[] = {
> + AV_PIX_FMT_YUV444P, AV_PIX_FMT_YUV422P, AV_PIX_FMT_YUV420P,
> + AV_PIX_FMT_YUV411P, AV_PIX_FMT_YUV410P, AV_PIX_FMT_YUVA420P,
> + AV_PIX_FMT_YUV440P, AV_PIX_FMT_GRAY8,
> + AV_PIX_FMT_RGB24, AV_PIX_FMT_BGR24, AV_PIX_FMT_RGBA,
> + AV_PIX_FMT_NONE
> + };
> +
> + ff_set_common_formats(ctx, ff_make_format_list(pix_fmts));
> + return 0;
> +}
> +
> +
> +static int config_input(AVFilterLink *inlink)
> +{
> + AVFilterContext *ctx = inlink->dst;
> + StabData *sd = ctx->priv;
> + int returnval;
> +
> + VSMotionDetect* md = &(sd->md);
> + VSFrameInfo fi;
> + const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(inlink->format);
> +
> + vsFrameInfoInit(&fi,inlink->w, inlink->h, AV2VSPixelFormat(ctx, inlink->format));
> + // check
> + if(fi.bytesPerPixel != av_get_bits_per_pixel(desc)/8)
> + av_log(ctx, AV_LOG_ERROR, "pixel-format error: wrong bits/per/pixel");
> + if(fi.log2ChromaW != desc->log2_chroma_w)
> + av_log(ctx, AV_LOG_ERROR, "pixel-format error: log2_chroma_w");
> + if(fi.log2ChromaH != desc->log2_chroma_h)
> + av_log(ctx, AV_LOG_ERROR, "pixel-format error: log2_chroma_h");
> +
No failure?
> + if(vsMotionDetectInit(md, &fi, "stabilize") != VS_OK){
> + av_log(ctx, AV_LOG_ERROR, "initialization of Motion Detection failed");
> + return AVERROR(EINVAL);
> + }
> +
> + // we need to do it after vsMotionDetectInit because otherwise the values are overwritten
> + if ((returnval = (av_set_options_string(sd, sd->args, "=", ":"))) < 0)
> + return returnval;
> +
> + // display help
> + /* if(optstr_lookup(sd->options, "help")) { */
> + /* av_log(ctx, AV_LOG_INFO, vs_motiondetect_help); */
> + /* return AVERROR(EINVAL); */
> + /* } */
> +
> + if(vsMotionDetectConfigure(md)!= VS_OK){
> + av_log(ctx, AV_LOG_ERROR, "configuration of Motion Detection failed\n");
You have a tab here.
> + return AVERROR(EINVAL);
> + }
> +
> + av_log(ctx, AV_LOG_INFO, "Image Stabilization Settings:\n");
> + av_log(ctx, AV_LOG_INFO, " shakiness = %d\n", md->shakiness);
> + av_log(ctx, AV_LOG_INFO, " accuracy = %d\n", md->accuracy);
> + av_log(ctx, AV_LOG_INFO, " stepsize = %d\n", md->stepSize);
> + av_log(ctx, AV_LOG_INFO, " algo = %d\n", md->algo);
> + av_log(ctx, AV_LOG_INFO, " mincontrast = %f\n", md->contrastThreshold);
> + av_log(ctx, AV_LOG_INFO, " show = %d\n", md->show);
> + av_log(ctx, AV_LOG_INFO, " result = %s\n", sd->result);
> +
> + sd->f = fopen(sd->result, "w");
> + if (sd->f == NULL) {
> + av_log(ctx, AV_LOG_ERROR, "cannot open transform file %s!\n", sd->result);
We tend to avoid yelling at users with a '!'
> + return AVERROR(EINVAL);
> + }else{
> + if(vsPrepareFile(md, sd->f) != VS_OK){
> + av_log(ctx, AV_LOG_ERROR, "cannot write to transform file %s!\n", sd->result);
> + return AVERROR(EINVAL);
> + }
> + }
> + return 0;
> +}
> +
> +
> +static int filter_frame(AVFilterLink *inlink, AVFrame *in)
> +{
> + AVFilterContext *ctx = inlink->dst;
> + StabData *sd = ctx->priv;
> + VSMotionDetect* md = &(sd->md);
> + LocalMotions localmotions;
> +
> + AVFilterLink *outlink = inlink->dst->outputs[0];
> + int direct = 0;
> + AVFrame *out;
> + VSFrame frame;
> + int plane;
> +
> + if (av_frame_is_writable(in)) {
> + direct = 1;
> + out = in;
> + } else {
> + out = ff_get_video_buffer(outlink, outlink->w, outlink->h);
> + if (!out) {
> + av_frame_free(&in);
> + return AVERROR(ENOMEM);
> + }
> + av_frame_copy_props(out, in);
> + }
> +
> + for(plane=0; plane < md->fi.planes; plane++){
> + frame.data[plane] = in->data[plane];
> + frame.linesize[plane] = in->linesize[plane];
> + }
> + if(vsMotionDetection(md, &localmotions, &frame) != VS_OK){
> + av_log(ctx, AV_LOG_ERROR, "motion detection failed");
> + return AVERROR(AVERROR_EXTERNAL);
> + } else {
> + if(vsWriteToFile(md, sd->f, &localmotions) != VS_OK){
> + av_log(ctx, AV_LOG_ERROR, "cannot write to transform file!");
ditto '!'
> + return AVERROR(EPERM);
Why perm? it could be something else. Can't you raise the AVERROR(errno)?
> + }
> + vs_vector_del(&localmotions);
> + }
> + if(md->show>0 && !direct){
> + // copy
> + av_image_copy(out->data, out->linesize,
> + (void*)in->data, in->linesize,
> + in->format, in->width, in->height);
> + }
> +
> + if (!direct)
> + av_frame_free(&in);
> +
> + return ff_filter_frame(outlink, out);
> +}
> +
> +static const AVFilterPad avfilter_vf_stabilize_inputs[] = {
> + {
> + .name = "default",
> + .type = AVMEDIA_TYPE_VIDEO,
> + .get_video_buffer = ff_null_get_video_buffer,
I think this is not necessary.
> + .filter_frame = filter_frame,
> + .config_props = config_input,
> + .min_perms = AV_PERM_READ,
You can drop this.
> + },
> + { NULL }
> +};
> +
> +static const AVFilterPad avfilter_vf_stabilize_outputs[] = {
> + {
> + .name = "default",
> + .type = AVMEDIA_TYPE_VIDEO,
> + },
> + { NULL }
> +};
> +
> +AVFilter avfilter_vf_stabilize = {
> + .name = "stabilize",
> + .description = NULL_IF_CONFIG_SMALL("extracts relative transformations of \n\
> + subsequent frames (used for stabilization together with the\n\
> + transform filter in a second pass)."),
Same as before, please avoid \n in the description string. This should be
fine though:
.description = NULL_IF_CONFIG_SMALL("Extract relative transformations of "
"subsequent frames (used for stabilization "
"together with the transform filter in a "
"second pass)."),
OTOH, such long description belongs to the documentation
(doc/filters.texi)
> + .priv_size = sizeof(StabData),
> + .init = init,
> + .uninit = uninit,
> + .query_formats = query_formats,
> +
> + .inputs = avfilter_vf_stabilize_inputs,
> + .outputs = avfilter_vf_stabilize_outputs,
> + .priv_class = &stabilize_class,
> +};
> diff --git a/libavfilter/vf_transform.c b/libavfilter/vf_transform.c
> new file mode 100644
> index 0000000..998a80d
Most of the previous comments also applies here, so I won't repeat them,
keep in mind to update this file as well.
[...]
--
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130329/aa4ca10e/attachment.asc>
More information about the ffmpeg-devel
mailing list