[FFmpeg-devel] [PATCH] video stabilization plugins using vid.stab library
Georg Martius
georg.martius at web.de
Tue Apr 2 00:36:00 CEST 2013
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://georg.hronopik.de -------------
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-video-stabilization-plugins-using-vid.stab-library-v.patch
Type: text/x-patch
Size: 37404 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130402/3fce263a/attachment.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/20130402/3fce263a/attachment.asc>
More information about the ffmpeg-devel
mailing list