[FFmpeg-devel] [PATCH] video stabilization plugins using vid.stab library

[Georg Martius]

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 
incremental ones? (worries because of the tabs). In case of problems I create 
a single patch.

More details below (anwsering to email of Michael and Clément):

On Monday 18 March 2013 00:55:19 Michael Niedermayer wrote:
> > [...]
> > I didn't adapt the configure scripts because I am not familiar with them:
> > "-lvidstab" is required for linking.
> 
> its quite simple, just see how other libs are detected
Indeed, done!

> > [...]
> > > > +OBJS-$(CONFIG_STABILIZE_FILTER)              += vf_stabilize.o
> > +OBJS-$(CONFIG_TRANSFORM_FILTER)              += vf_transform.o
> 
> someone will soon tell you something about maintaining alphabetic
> ordered lists
Sorry! corrected

> > [...]
> > > > +/* private date structure of this filter*/
> > +typedef struct _stab_data {
> > +    AVClass* class;
> > +
> > 
> > +    MotionDetect md;
> 
> the struct name should probably have some prefix to avoid namespace
> conflicts, if iam guessing correctly that its from libvidstab.h
I see. I didn't prefix all my declarations now because it require a bit too 
much work for the moment, but I do it definitelly. 

> > [...]
> > +/// pointer to context for logging
> > +void *_stab_ctx = 0;
> 
> non constant globals are generally a bad idea, there can be multiple
> filter instances, multiple filter graphs and they can get accessed
> at the same time from different threads
I removed it now. I didn't like it either. I want that the logging from my lib 
uses the av_log facilities. Now it creates a save context:

/// struct to hold a valid context for logging from within vid.stab lib
typedef struct {
    const AVClass* class;
} TransformLogCtx;

/** wrapper to log vs_log into av_log */
static int av_log_wrapper(int type, const char* tag, const char* format, ...){
    va_list ap;
    TransformLogCtx ctx;
    ctx.class = &transform_class;
    av_log(&ctx,  type, "%s: ", tag);
    va_start (ap, format);
    av_vlog(&ctx, type, format, ap);
    va_end (ap);
    return VS_OK;
}

> [...]
> also see tools/patcheck
Mh. it suggests to merge the av_log calls, but I think it would decrease 
readability so I ignored it.

On Monday 18 March 2013 02:03:51 Clément Bœsch wrote:
> On Sun, Mar 17, 2013 at 11:59:17PM +0100, Georg Martius wrote:
> > [...]
> > +    sd->result = av_malloc(VS_INPUT_MAXLEN);
> > +    snprintf(sd->result, VS_INPUT_MAXLEN, DEFAULT_TRANS_FILE_NAME);
> > +
> 
> av_strdup() is your friend
> 
> Also make sure you free such strings.
> 
I avoided it completely because it can be done from the opt tools.

> >  [...]
> > +    // save args for later
> > +    if(args)
> > +        fd->args=av_strdup(args);
> > +    else
> > +        fd->args=0;
> > +
> 
> This looks a bit suspicious; is it necessary?
> 
> Also, setting to 0 has no point at this point; everything in the context is
> zeroed.

Well, the args are needed in the config function to initialize the library. It 
cannot be done before because otherwise the values set by the user would be 
overwritten by the default values. I added a comment and removed the setting 
to 0.

> > [...]
> > +
> > +    if(fd->args) av_free(fd->args);
> 
> No need to check for pointer before free.
> 
Okay, didn't know. Am I correct that the strings that are allocated in by the 
options are free'd on av_opt_free()?

> 
> I'll have some more comments later.
Welcome!

Cheers,
  Georg
-- 
---- Georg Martius,  Tel: +49 177 6413311  -----
--------- http://georg.hronopik.de -------------
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-video-stabilization-plugins-using-vid.stab-library.patch
Type: text/x-patch
Size: 29694 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130319/1b716442/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-modified-stabilize-and-transform-filters-to-comply-f.patch
Type: text/x-patch
Size: 36852 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130319/1b716442/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130319/1b716442/attachment.asc>