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

[Georg Martius]

On Sunday 31 March 2013 11:15:48 Clément Bœsch wrote:
> 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.

Okay, I will do that.

> > > [...]
> > > 
> > > > +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).

When my lib is as dynamically loaded lib then the globals are not shared 
between the different instances of loading. The problem with the design below 
is that not all functions in my lib have access to the context. Take for 
instance a simple vector object: why should it know something about a highly 
specialized context? Then one can hardly reuse any code. 
I understand that the design is not ideal, but I would like to keep it the way 
it is.

> 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

I short test suggest that it works, but I don't know how robust that is. I 
think my implementation is more transparent and certainly correct.

Regards,
  Georg

-- 
---- Georg Martius,  Tel: +49 177 6413311  -----
--------- http://georg.hronopik.de -------------
-------------- 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/20130401/6557ad37/attachment.asc>