[FFmpeg-devel] [PATCH] avfilter: add dewobble_opencl filter

[Daniel Playfair Cal]

Thanks for the feedback.

On Sun Aug 15 14:11:27 EEST 2021 Lynne <dev at lynne.ee> wrote:
>
> 15 Aug 2021, 11:13 by daniel.playfair.cal at gmail.com:
>
> > On Sun, Aug 15, 2021 at 6:18 PM Paul B Mahol <onemda at gmail.com> wrote:
> >
> >> Non native filters can not be named like this.
>
> All library wrappers must be prefixed with 'lib', so 'libdewobble_opencl'.

Ok, I can do that, but I'm a little confused though what the
convention is. Other comparable filters which wrap an external library
such as lensfun and vidstabdetect/vidstabtransform are not named with
a lib prefix. Is this a recent change in policy, or are these filters
not comparable for some reason?

> Why are there mandatory GPU memcpys on both the input and output?
> Can't the library be made to work with images rather than buffers?

The images do eventually need to be converted to buffers because that
is all that OpenCV supports. Having said that, there are probably some
optimizations that could be done if Dewobble had access to the
original images. For example, the NV12 -> BGR conversion could
probably be combined into the same kernel with the image -> buffer
conversion (and vice versa).

However there are a few complicating/limiting factors:
 - OpenCL frames that were mapped from VA-API frames still occupy the
very limited VA-API frame pool, so it's necessary to avoid pulling too
many of them without first freeing those that are already held
 - libdewobble requires keeping a queue of frames in memory in some
form (how many depends on the options)
 - libdewobble currently directly uses the luma portion of the input
frames, including the luma portion of previously pushed frames
 - dewobble_filter_push_frame (for the threaded version of
DewobbleFilter as is used here) doesn't currently block on anything
more than pushing the frame onto a queue. Doing so (i.e. to avoid
keeping a reference to the input image) would require more message
passing between the main thread and the worker thread (where it's safe
to use OpenCV without disturbing any global context).

So TL;DR, I agree that it's a good idea to pass the images directly to
Dewobble, and I think it probably possible to avoid some/all of the
copying depending on the options. However it would be a significant
change, so I'd prefer to add this ability to libdewobble at a later
time, by adding a new input/output format on top of the existing NV12
OpenCL buffer. At that point I'll be able to submit another patch to
make use of it and avoid these copies in the FFmpeg filter.

> > /// Motion stabilization algorithm, mirroring those available in libdewobble.
> Can't it just use the defines from libdewobble instead?

There are no defines in libdewobble for motion stabilization
algorithms. The stabilizers are each separate classes with separate
constructors and varying parameters.

> Finally, coding style, follow the coding style. No brackets around
> 1-line statements,
> function arguments do not go on one line each, for (int i) is
> supported, and don't mix
> C++ and C-style comments, just use C, since that's what we use most
> often, we don't
> strictly enforce the 80-char line guideline if the code looks like a
> mess after newlines.

Ok. I ran `indent -i4 -kr -nut` as is suggested on
https://ffmpeg.org/developer.html - can you suggest alternative
options that would fit with your requirements?

Otherwise I'll change to C style comments and fumble with `indent` or
`clang-format` options until they do what you suggest.

> function arguments do not go on one line each

This one in particular is not clear to me. If not on one line each,
then where? They are not currently all on separate lines, but putting
them all on one line would result in some unreasonably long lines.

On Sun, Aug 15, 2021 at 7:13 PM Daniel Playfair Cal
<daniel.playfair.cal at gmail.com> wrote:
>
> On Sun, Aug 15, 2021 at 6:18 PM Paul B Mahol <onemda at gmail.com> wrote:
>
> > Non native filters can not be named like this.
>
> OK, how would you suggest to name it - just "dewobble"?
>
> My thinking was that since the user must ensure that the input/output
> is OpenCL hardware frames (e.g. using hwupload/hwmap), the "_opencl"
> postfix in the name would serve a hint that this is necessary (as it
> is for all the native OpenCL filters).