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

[Georg Martius]

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?

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

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

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

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/20130330/9b9e0e76/attachment.asc>