[FFmpeg-devel] [PATCH] libavfilter-soc: implement pad filter

[Vitor Sessak]

Stefano Sabatini wrote:
> On date Thursday 2009-07-30 06:26:46 +0200, Vitor Sessak encoded:
>> Stefano Sabatini wrote:
>>> On date Monday 2009-07-27 06:15:47 +0200, Vitor Sessak encoded:
>>>> Stefano Sabatini wrote:
>>> [...]
>>>>> Now I'm posting other patches, what I'd like to discuss are these two
>>>>> points:
>>>>>
>>>>> * I tried both to put the information required for padding (x, y, h,
>>>>>   w, exp_h, exp_w) in the link and pass it in the get_video_buffer()
>>>>>   params, they're pretty equivalent but the second method (as in the
>>>>>   patch) seems more flexible, as the information is not stored
>>>>>   statically in the links, so it doesn't need a reconfiguration when
>>>>>   changing it.  (BTW maybe we can as well remove w/h from the link,
>>>>>   that should make simpler to implement variable size filters, I still
>>>>>   have to experiment with this).
>>>> Just a question: does it works when linesize is negative?
>>> I believe that should work (not 100% sure though, I shuld double-check
>>> it), note that the vflip filter as it is currently doesn't work for
>>> another reason.
>> There should be some samples that output after decoding negative  
>> linesize (I have no example in mind now, unfortunately).
>>
>>>>> * I tinkered about the vflip problem much time, and I could'nt find 
>>>>> some   way to keep the previous behavior with the new system.
>>>>>   The new system assumes that the position of the area to pad
>>>>>   (referred by the x/y params in the picref) needs to be invariant
>>>>>   when passing a picref to the next filter.
>>>>>   That's because the pad filter expects this:
>>>>>
>>>>> static void start_frame(AVFilterLink *link, AVFilterPicRef *picref)
>>>>> {
>>>>>     PadContext *pad = link->dst->priv;
>>>>>     AVFilterPicRef *ref2 = avfilter_ref_pic(picref, ~0);
>>>>>     link->dst->outputs[0]->outpic = ref2;
>>>>>
>>>>>     ref2->data[0] -=  pad->x             + (pad->y * ref2->linesize[0]);
>>>>>     ref2->data[1] -= (pad->x>>pad->hsub) + (pad->y * ref2->linesize[1]>>pad->vsub);
>>>>>     ref2->data[2] -= (pad->x>>pad->hsub) + (pad->y * ref2->linesize[2]>>pad->vsub);
>>>>>     if (ref2->data[3])
>>>>>         ref2->data[3] -= pad->x + pad->y * ref2->linesize[3];
>>>>>     ref2->x -= pad->x;
>>>>>     ref2->y -= pad->y;
>>>>>     ref2->w = pad->out_w;
>>>>>     ref2->h = pad->out_h;
>>>>>
>>>>>     avfilter_start_frame(link->dst->outputs[0], ref2);
>>>>> }
>>>>>
>>>>> to work.
>>>> Unless I'm misunderstanding something, this forbids, for ex., 
>>>> chainning  two pad filters. Am I right?
>>> Of course padding/cropping chaining is supported, that was a
>>> prerequisite for it to be acceptable, so let me clarify the problem.
>>>
>>> The pad filter takes in input a video buffer, and knows that it is
>>> offeset with respect to the area when it has to write.
>>> So we have:
>>>
>>>             [exp_w, exp_h]
>>> +--------------------------------------+
>>> |                                      |       |  (padx, pady)          
>>>               |
>>> |    +------------------------+        |
>>> |    |                        |        |
>>> |    |        [w, h]          |        |
>>> |    |                        |        |
>>> |    +------------------------+        |
>>> +--------------------------------------+
>> I suppose that in this drawing pad{x,y} is stored in  
>> AVFilterPicRef.{x,y}, no?
>>
>>> It assumes that the data in the passed reference of the video buffer
>>> points to the top-left corner of the internal area of the padded area,
>>> and so it "moves back" in the new video buffer reference the pointers
>>> to the buffer, so they point to the top-left corner of the padded
>>> area.
>> So it changes AVFilterPicRef.{x,y}.
> 
> Yes, actually only the pad and crop filter are meant to change those
> values.
> 
> For big-buffer I mean the allocated area, which accounts also for the
> padding area.
> 
> The meaning for x, y in the picref are:
> 
> * the x,y offsets, with respect to the big-frame buffer area, of the
>   data[plane] pointers.
> 
> * data[plane] pointers are supposed to contain the pointers to the
>   area of the big-buffer when the filter has to write.
> 
> The pad and crop filters are special as they change the area in the
> big-buffer when the next filter has to write.
> 
>>> The vflip filter on the other hand, puts in the passed buffer
>>> reference the pointers to the bottom-left corner of the input buffer,
>>> and invert the linesizes, which is not what the pad filter assumes.
>>> Negative linesizes shouldn't be a problem, the problem would be with
>>> the pady value which should be changed (also for all the following pad
>>> filters).
>> So, but I'm missing something. It is this last assertion that troubles  
>> me. Which field of which struct exactly a filter (or just vflip?) cannot  
>> change?
> 
> A filter such a vflip filter is not supposed to change the area in the
> big-buffer where the next filter is going to write, so it shouldn't be
> allowed to change data[plane] and the x,y offsets (neither the
> rendering vertical direction of the whole big-buffer, as I'll explain
> below).
> 
>> What will fail with the following:
>>
>> pic->data[0] += exp_w*frame->linesize[0] + exp_h;
>> pic->linesize[0] *= -1;
>> [... Same for chroma ...]
>> pic->x = pic->exp_w - pic->x - pic->w;
>> pic->y = pic->exp_h - pic->y - pic->h;
>>
>> ?
>>
>> I suppose I have the right to change pic->x, because the pad filter does  
>> that too.
> 
> Now there is another problem with the linesize inversion. As the
> current vflip filter works it assumes that the buffer to vflip is the
> whole buffer to display.
> 
> Let's consider this filterchain:
> vflip , pad
> 
> It would be possible to make the vflip filter to change the y offset,
> but then we would obtain something like this:
> 
> +----------------------------------+
> |                                8 |
> |                                7 |
> |                                6 |
> | +----vflip-area------+         5 |
> | |         |          |     |   4 |
> | |         v          |     v   3 |
> | +--------------------+         2 |
> | (x,y)                          1 |
> +----------------------------------+
> 
> while we would rather expect:
> 
> +----------------------------------+
> | (x,y)                          1 |
> | +----vflip-area------+         2 |
> | |         |          |     ^   3 |
> | |         v          |     |   4 |
> | +--------------------+         5 |
> |                                6 |
> |                                7 |
> |                                8 |
> +----------------------------------+
> 
> So even if we change the y offset with respect to the new coordinate
> system, the linesize inversion will cause the whole big-buffer to be
> rendered reverse-wise.

Ok, but what if it was already allocated reverse-wise to begin with? I 
mean something like the completely untested following code:

static AVFilterPicRef *vflip_get_video_buffer(AVFilterLink *link,
                                               int perms,
                                               int x, int y,
                                               int w, int h,
                                               int exp_w, int exp_h)
{
     int newx = exp_w - x - w;
     int newy = exp_h - y - h;

     return avfilter_get_video_buffer(link->dst->outputs[0], perms, 
newx, newy, w, h, exp_w, exp_h);
}

-Vitor