Add avfilter module/command

[mkurkov]

Hi,

Here is my attempt to add FFmpeg filter graphs integration to baresip.

Idea is to allow dynamically set video filter graph for current video stream. And then remove it when needed.
It's implemented as baresip filter module and command /avfilter. For example to enable vflip simple filter graph just call /avfilter vflip at any time and see how video is flipped. To disable filter graph just send /avfilter command without any filter string.
As more complex example you can apply some png overlay on top of current video stream with
/avfilter movie=/path/to/some/image.png[pic];[in][pic]overlay=(W-w)/2:H-h-20 command.
Full syntax and list of filters are available on FFmpeg Filtegraph's page: https://ffmpeg.org/ffmpeg-filters.html#Filtergraph-description

I run into several issues during development:

1. Bottom-up FFmpeg frames.

It seems that FFmpeg frame structure could keep both top-down and bottom-up plane lines orders. In the last case
linesizes are negative and data pointers point to the last rows in the buffer. Baresip uses unsigned linesizes, so as I understand
it doesn't support bottom-up order. As workaround I had to rearrange plane lines in top-down order for such frames and
update linesizes/data pointers. So my question is how Baresip works with bottom-up frames and is my approach valid.

2. Moving data between AVFrame/vidframe structures

I wonder how properly work with FFmpeg AVFrame structure. Currently as I get new AVFrame from filter graph pipeline, I just
update incoming filter frame, setting data pointers and linesizes to AVframe data. It seems to work well and without any additional copying. Main question is when I supposed to unref AVFrame structure. Normally it should be done in the end of encoding pipeline, but filter module sits in the middle of pipeline and doesn't control it. Now I unref previous AVFrame before processing new one, assuming that previous encoding cycle is finished and it's safe to clean memory. Am I right or there is a more proper way to do it?

I don't have much experience with baresip modules and video processing, so any suggestions are greatly appreciated.

Thanks,
Mikhail

[alfredh]

no need to initialise lock, static/global vars are inited to null.

[alfredh]

you should return err here, instead of ENOMEM

[alfredh]

also setting lock to NULL makes it more robust:

lock = mem_deref(lock)

[alfredh]

2 empty lines between functions

[alfredh]

time_base = 1/25 is hardcoded.

if you want to use the timestamps from the API you can use 1/1000000 as time_base.
(or VIDEO_TIMEBASE constant)

[alfredh]

2 empty lines between functions

[alfredh]

since filter_reset always returns 0, you can change the return type to void

[alfredh]

the two input arguments can be made const

[alfredh]

I think it is dangerous to declare a variable size array on the stack.

probably better to use a heap buffer, or a fixed size stack buffer

[alfredh]

curly brace at the end { should be moved to next line

[alfredh]

this sounds strange, I have never seen a negative linesize. Which plane was it (0 to 3) ?

the linesize is the number of pixels in a horizontal line, for each plane.
example: [320, 160, 160, 0]

[mkurkov]

For bottom-up frames all planes have negative linesizes, like this: linesize = {-1920, -960, -960, 0, 0, 0, 0, 0} (this is from gdb inspecting AVFrame structure).

[alfredh]

const is not needed

[alfredh]

const is not needed

[alfredh]

use TAB for indentation

[alfredh]

hi @mkurkov many thanks for this good and useful piece of work. we will merge it to master.

regarding bottom-up frames, I have never seen negative linesize in the wild.
baresip linesize is only unsigned.

regarding copying of buffers, it is fine to update data pointers in struct vidframe to point
to the real buffers. the buffer should be valid until the vidfilt encode function has returned,
because the application will use the data. The next time the vidfilt encode function is called,
they can be overwritten with new pointers.

I tested this with ffmpeg 4.1.4 and it works just fine:

$ ./baresip -m avfilter.so -e/vidloop -e"/avfilter boxblur"

I added some review comments inline, please have a look

[alfredh]

please also add your name and the new module to this list:

https://github.com/baresip/baresip/wiki/Modules

[mkurkov]

@alfredh thanks for reviewing, will fix mentioned issues.

Regarding bottom-up frames, I run into them trying simple filters like hflip - it worked properly, but with vflip I constantly got segfault. Looking into AVFrame from vflip in gdb I saw this:

{data = {0x7fffe7675cc0 '\353' <repeats 199 times>, <incomplete sequence \353>..., 
    0x7fffe76f85a0 '\200' <repeats 200 times>..., 0x7fffe7777dc0 '\200' <repeats 200 times>..., 0x0, 0x0, 0x0, 0x0, 0x0}, 
  linesize = {-1920, -960, -960, 0, 0, 0, 0, 0}, extended_data = 0x7fffec0e7600, width = 1920, height = 1080,

I guess vflip switches frame to bottom-up as optimization. Looking at vflip implementation, it's visible that they also change data to point on last row in buffer:

static int filter_frame(AVFilterLink *link, AVFrame *frame)
{
     FlipContext *flip = link->dst->priv;
     int i;
 
     for (i = 0; i < 4; i ++) {
         int vsub = i == 1 || i == 2 ? flip->vsub : 0;
         int height = FF_CEIL_RSHIFT(link->h, vsub);
 
         if (frame->data[i]) {
             frame->data[i] += (height - 1) * frame->linesize[i];
             frame->linesize[i] = -frame->linesize[i];
         }
     }
 
     return ff_filter_frame(link->dst->outputs[0], frame);
}

Not sure if this could be a problem in other parts of Baresip, but avfilter needs to support bottom-up frames too.

[mkurkov]

@alfredh issues should be resolved in last commits.

[alfredh]

thanks

I get an error when compiling now:

  CC [M]  build-x86_64/modules/avfilter/filter.o
modules/avfilter/filter.c:119:3: error: void function 'filter_reset' should not return a value
      [-Wreturn-type]
                return 0;
                ^      ~
1 error generated.
make: *** [build-x86_64/modules/avfilter/filter.o] Error 1

[alfredh]

https://patchwork.ffmpeg.org/project/ffmpeg/patch/20170806190921.10897-1-cus@passwd.hu/

librem/baresip supports only positive linesize:

/** Video frame */
struct vidframe {
	uint8_t *data[4];      /**< Video planes        */
	uint16_t linesize[4];  /**< Array of line-sizes */
	struct vidsz size;     /**< Frame resolution    */
	enum vidfmt fmt;       /**< Video pixel format  */
};

this has worked fine for many years.

adding support for negative linesize, would mean updating and testing
all modules and code that is involved with video processing.

I think for now it is best that you work around it in avfilter.so

[alfredh]

thanks a lot for your contribution :)