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

[Clément Bœsch]

On Sat, Mar 30, 2013 at 01:30:33AM +0100, Georg Martius wrote:
> Hi Clément,
> 
> thanks for reviewing my patch. Add addressed all but few things you raised. I 
> also write a documentation in the filters.texi. However, let's discuss a few 
> points before I submit another patch:
> 
> On Friday 29 March 2013 02:48:06 Clément Bœsch wrote:
> > [...]
> > 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.
> Okay No problem. I changed my wrapper to LGPL this should solve the problem, 
> right?
> 

There is not really a problem :)

If you changed your wrappers to LGPL, yes you don't need to update the
LICENSE file, except to add the library in the GPL library list of that
file. And don't forget to keep the die statement in the configure so
FFmpeg can't be linked to the library if an explicit GPL build isn't
activated.

> > [...]
> > > +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.
> Done.
> 
> > [...]
> > > +
> > > +/// 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.
> 
> Well the 'tag' is my context, but it is a string. How can there be an 
> uninitialized read? The stabibilize_class is a global constant and should
> be always initialized before any logging can take place. Please enlight me.

I was thinking of StabLogCtx, but that's likely irrelevant.

> 
> > [...]
> > > +    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.
> 
> Sorry, but I don't get it. What is the difference between one global or many 
> globals? Or do you suggest to pass the these function in struct to each 
> function to avoid global variables. I agree that global variables are bad but 
> here I really don't see the point. 
> Which globals are the problem?  the vs_ or the av_
> 

The problem is that several instances of your filter with different
settings could be run in parallel (imagine an application using your
library but linking against a FFmpeg linked against your lib as well).
Ideally, an API should provide something like this:

    static int av_log_wrapper(void *opaque, int type, const char *tag, const char *fmt, ...)
    {
        StabData *sd = opaque;
        av_log(sd, type, ...)
        ...
    }

    void foo()
    {
        StabData *sd = ...;
        VSContext *vsctx;
        VSAllocFunc allocfn = {
          .malloc  = av_malloc,
          .zalloc  = av_mallocz,
          .realloc = av_realloc,
          .free    = av_free,
        };

        vsctx = vs_alloc_context(sd);
        vs_set_alloc_funcs(vsctx, &allocfn);
        vs_set_log_callback(vsctx, av_log_wrapper);

        ...
    }

With such model, you have the allocation functions, and logging system
within your context. Also, your library is supposed to call the logging
wrapper with your custom context (the opaque parameter is the StabData
pointer), which allows you to have access to have more control on the
logging (to disable it when necessary for instance).

> > [...]
> > > +#define OFFSET(x) offsetof(StabData, x)
> > > +#define OFFSETMD(x) (offsetof(StabData, md)+offsetof(VSMotionDetect, x))
> 
> > offsetof(StabData.md, x) doesn't work?
> 
> No it does not. You need a typename in the first argument of offsetof.
> 

I was thinking of offsetof(StabData, md.x) sorry

-- 
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/20130331/7aac9553/attachment.asc>