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

[Wei Gao]

Hi,
Michael Niedermayer, thanks for your reviewing, some questions and
explanations as follows

Thanks
Best regards

2013/3/27 Michael Niedermayer <michaelni at gmx.at>

> On Tue, Mar 26, 2013 at 06:56:13PM +0800, Wei Gao wrote:
> >
>
>
> >  #define REGISTER_FILTER(X, x, y)
>  \
> >      {
> \
> > @@ -35,7 +38,21 @@
> >          extern AVFilter avfilter_##x;
> \
> >          avfilter_register(&avfilter_##x);
> \
> >      }
> > +#if CONFIG_OPENCL
> > +#define OPENCL_REGISTER_FILTER(X, x, y)
>          \
> > +    {
>          \
> > +        extern AVFilter avfilter_##y##_##x;
>          \
> > +        if (CONFIG_##X##_FILTER) {
>           \
> > +            avfilter_register(&avfilter_##y##_##x);
>          \
>
> > +
>  av_opencl_register_kernel((avfilter_##y##_##x).name,ff_kernel_##x);  \
>
> compiling the kernel at registration is not ideal.
> Consider there are 50 filters with opencl, its very unlikely more
> then 3 will be used during runtime. compiling or loading the others
> is a waste of time.
>
this is just register kernel code, and not compile, the compile operations
is in function "av_opencl_init", these code is because that at the first
time, we designed to compiled the kernel on time and create a binary file,
and next time we just read the binary file without compiling.

>
> [...]
>
> > +#if CONFIG_DESHAKE_OPENCL_FILTER
> > +    if (deshake->is_opencl) {
> > +        int ret = 0;
>
> > +        if ((!deshake->opencl_env.cl_inbuf.cl_buf) ||
> (deshake->opencl_env.cl_outbuf.cl_buf)) {
>
> is this supposed to be !A || B or !A || !B ?
>
sorry, my mistake. !A || !B

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

>
> Thanks
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> I do not agree with what you have to say, but I'll defend to the death your
> right to say it. -- Voltaire
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>