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

[Stefano Sabatini]

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.

My proposal is to simply make illegal the linesize inversion, as it is
now it is only useful for the vflip filter, and makes very hard to
implement a sane behavior with big-buffer areas, and frankly I can't
imagine any other filter when the linesize inversion may be useful.

The price to pay in this case is that the vflip filter would require a
memcpy (check the already posted patch which implements that).

Regards.
-- 
FFmpeg = Fundamentalist and Faboulous Mystic Portable Elected God