[FFmpeg-devel] [PATCH] video stabilization plugins using vid.stab library
Georg Martius
georg.martius at web.de
Mon Apr 8 09:39:28 CEST 2013
Hi,
may I remind you of my patch for review. I guess it was just overseen...
Thanks and best regards,
Georg
On Tuesday 02 April 2013 00:36:00 Georg Martius wrote:
> Hi,
>
> attached is my new patch with new names for the filters:
> vidstabdetect and vidstabtransform
>
> with configure script and makefile adaptations
> with documentation in filters.texi
> without tabs ;-)
>
> Regards,
> Georg
>
> 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.
> >
> > > > [...]
> > > >
> > > > > +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
--
---- Georg Martius, Tel: +49 177 6413311 -----
------- http://www.flexman.homeip.net ----------
-------------- 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/20130408/bdb78329/attachment.asc>
More information about the ffmpeg-devel
mailing list