[FFmpeg-cvslog] r26315 - trunk/libavfilter/vf_pad.c

[Baptiste Coudurier]

On 01/11/2011 05:19 PM, Michael Niedermayer wrote:
> On Tue, Jan 11, 2011 at 05:01:00PM -0800, Baptiste Coudurier wrote:
>> On 01/11/2011 04:45 PM, Michael Niedermayer wrote:
>>> On Tue, Jan 11, 2011 at 04:35:14PM -0800, Baptiste Coudurier wrote:
>>>> On 01/11/2011 03:53 PM, michael wrote:
>>>>> Author: michael
>>>>> Date: Wed Jan 12 00:53:24 2011
>>>>> New Revision: 26315
>>>>>
>>>>> Log:
>>>>> Fix design of the pad filter.
>>>>> Previously the pad filter just drawed borders in the surrounding of the input
>>>>> without checking if this area was allocated or writeable. Now we check and
>>>>> allocate a new buffer if the input is unsuitable.
>>>>
>>>> How so ? pad filter get_buffer should ensure that the buffer is
>>>> correctly allocated.
>>>
>>> yes, but nothing gurantees that what is input into start_frame/draw_slice
>>> is what get_buffer returned
>>
>> Humm I have a bad feeling about this. I think it should be better
>> guaranteed given how many filters handle the buffers currently.
>>
>>> An example to show this problem
>>>
>>>                   /->pad=1000:100
>>> decoder ->   split
>>>                   \->pad=100:1000
>>>
>>> the decoder can here only get frames allocated from one of the 2 pad filters
>>> the second pad filter will get frames that have been allocated by the other
>>
>> Yes, but in this case direct rendering is not possible for the one of
>> the filters, so I don't see why this is a problem.
>
> in this case true, but for example:
>
>
>                    /->crop=1000:100
>   decoder ->   split
>                    \->crop=100:1000
>
> direct rendering is possible but only if buffers are passed along that have
> not been provided by the next filters get_buffer().

Well, yes, but that's not the pad filter :)

>> [...]
 >>
>>> There are also simpler cases where its convenient not to return the data in
>>> the next filters provided buffer.
>>> Examples are for example when the data alraedy is somewhere else like a
>>> AVPacket from a raw demuxer or from a decoder like our mpeg2 decoder that can
>>> provide slices by reusing a 16pixel high buffer to improve cache useage
>>
>> Assuming direct rendering, the decoder will write slices in the user
>> supplied buffer using get_buffer, so I don't see why this is a problem.
>
> It wont, see yourself:
>      s->dest[0] = s->current_picture.data[0] + ((s->mb_x - 1)<<  mb_size);
>      s->dest[1] = s->current_picture.data[1] + ((s->mb_x - 1)<<  (mb_size - s->chroma_x_shift));
>      s->dest[2] = s->current_picture.data[2] + ((s->mb_x - 1)<<  (mb_size - s->chroma_x_shift));
>
>      if(!(s->pict_type==FF_B_TYPE&&  s->avctx->draw_horiz_band&&  s->picture_structure==PICT_FRAME))
>      {
>          if(s->picture_structure==PICT_FRAME){
>          s->dest[0] += s->mb_y *   linesize<<  mb_size;
>          s->dest[1] += s->mb_y * uvlinesize<<  (mb_size - s->chroma_y_shift);
>          s->dest[2] += s->mb_y * uvlinesize<<  (mb_size - s->chroma_y_shift);
>          }else{
>              s->dest[0] += (s->mb_y>>1) *   linesize<<  mb_size;
>              s->dest[1] += (s->mb_y>>1) * uvlinesize<<  (mb_size - s->chroma_y_shift);
>              s->dest[2] += (s->mb_y>>1) * uvlinesize<<  (mb_size - s->chroma_y_shift);
>              assert((s->mb_y&1) == (s->picture_structure == PICT_BOTTOM_FIELD));
>          }
>      }
>
> B frames of codecs except h264 (where its impossible due to intra pred)
> will be returned in 16pixel high slices when draw_horiz_band is used never
> filling the whole buffer from get_buffer

Well, this is a mess IMHO. I have the feeling this was tested ~10 years 
ago and is no longer true.

In general, I'm against making APIs more complicated because of some 
obscure case.

Anyway, I don't think I agree with not guaranteeing that the buffer 
passed to start_frame is the buffer returned by get_buffer, this will 
make filters a mess and will only cause problems.

And this would break yadif as well.

Implementation of pad filter was good as it was, now it's overly 
complicated.

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org