Thanks for the feedback. On Sun Aug 15 14:11:27 EEST 2021 Lynne <d...@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....@gmail.com> wrote: > > On Sun, Aug 15, 2021 at 6:18 PM Paul B Mahol <one...@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). _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
