[FFmpeg-devel] [PATCH 2/2] libavutil/libavfilter: deshake opencl filter based on comments on 20130326

[Michael Niedermayer]

On Wed, Mar 27, 2013 at 09:41:45AM +0800, Wei Gao wrote:
[...]
> >
> >
> > [...]
> > > +#endif
> > > +
> > >
> > >  static const AVFilterPad deshake_inputs[] = {
> > >      {
> > > @@ -583,3 +723,36 @@ AVFilter avfilter_vf_deshake = {
> > >      .outputs       = deshake_outputs,
> > >      .priv_class    = &deshake_class,
> > >  };
> > > +
> > > +#if CONFIG_DESHAKE_OPENCL_FILTER
> > > +
> > > +static const AVFilterPad deshake_opencl_inputs[] = {
> > > +    {
> > > +        .name         = "default",
> > > +        .type         = AVMEDIA_TYPE_VIDEO,
> > > +        .filter_frame = filter_frame,
> > > +        .config_props = config_props,
> > > +    },
> > > +    { NULL }
> > > +};
> > > +
> > > +static const AVFilterPad deshake_opencl_outputs[] = {
> > > +    {
> > > +        .name = "default",
> > > +        .type = AVMEDIA_TYPE_VIDEO,
> > > +    },
> > > +    { NULL }
> > > +};
> > > +
> > > +AVFilter avfilter_vf_deshake_opencl = {
> > > +    .name          = "deshake_opencl",
> > > +    .description   = NULL_IF_CONFIG_SMALL("Stabilize shaky video using
> > OpenCL."),
> > > +    .priv_size     = sizeof(DeshakeContext),
> > > +    .init          = init_opencl,
> > > +    .uninit        = uninit_opencl,
> > > +    .query_formats = query_formats,
> > > +    .inputs        = deshake_opencl_inputs,
> > > +    .outputs       = deshake_opencl_outputs,
> > > +    .priv_class    = &deshake_class,
> > > +};
> > > +#endif
> >
> > opencl code should not be interleaved with #ifdefs into filters
> > this would be unmaintainable especially with complexer filters
> >
> > the opencl code should be put in seperate file(s) and either used
> > like SIMD optimizations through function pointers or by including a
> > header with opencl code if thats not possible
> > Other solutions are of course possible too but such mixing with ifdefs
> > is not a good idea, it would make it also hard for you to
> > maintain the code in the future ...
> >
> I did this because the opencl code just implement the transform operations.
> and other code can be duplicated. should I have to copy the code another
> file?

no code should be duplicated

The way the code looks atm is approximatly like

lots ansi C code
...
#if CONFIG_DESHAKE_OPENCL_FILTER
lots opencl code
#else
lots ansi C code
#endif
repeat above multiple times

One way it could look instead is

deshake.c:
deshake_transform_c()
{
    lots of C code
}
init(){
    if(CONFIG_DESHAKE_OPENCL_FILTER && opencl_available && opencl_enabled) {
        context->transform = deshake_transform_opencl;
        deshake_init_opencl();
    }else{
        context->transform = deshake_transform_c;
    }
}
...
context->transform(a,b,c);
...


deshake_opencl.c:
deshake_init_opencl()
{
    lots of opencl code
}
deshake_transform_opencl()
{
    lots of opencl code
}
-----------

This way the code is more seperated, and theres a clear API between
namely whatever context->transform() would take as input and output
that makes the code easier to understand and work with.
of course it also could be kept as 2 AVFilters where doing so makes
more sense

also the way the code looks ATM either the CPU or the GPU is idle if
i dont miss anything
it would be better to have both work at the same time

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are best at talking, realize last or never when they are wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130327/8a12d57b/attachment.asc>