[FFmpeg-cvslog] r26315 - trunk/libavfilter/vf_pad.c
Baptiste Coudurier
baptiste.coudurier
Wed Jan 12 05:34:01 CET 2011
On 1/11/11 8:12 PM, Michael Niedermayer wrote:
> On Tue, Jan 11, 2011 at 05:35:11PM -0800, Baptiste Coudurier wrote:
>> 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 :)
>
> no but if we enforce that buffers passed down must have come from get_buffer()
> of the matching filter then copying will happen here because (at least currently)
> teh code cant know if it can skip the copy into a get_buffer() buffer
> aka split/ the core cant know if simple pad (needs copy) or crop (needs no copy)
> is later
>
>
>>
>>>> [...]
>>>>
>>>>> 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.
>
> mplayer uses this codepath
Yes, but that does not mean that this special case does still improve cache
usage and that this improvement is significant/still matters.
>>
>> In general, I'm against making APIs more complicated because of some
>> obscure case.
>
> i agree but iam unsure if this is obscure and not rather common
> Filters can duplicate frames which means more will be passed down than there
> where get_buffer() calls.
Filters must use get_buffer if the next filter has it. I'm not sure I
understand here. Yadif duplicates frames and call next filter get_buffer.
> crop can modify the origin and size of a picture, vflip can flip the linesize,
> frame2field can double the linesize.
>
> simple pad, before my commit very definitly did not support this
I think I don't understand what exactly was the problem.
If you do treatment that breaks direct rendering don't expect it to work.
If you are messing with linesize, here comes problems because of
chaining and direct rendering.
> And a decoder even if it supports get_buffer() (most do but not all)
> can return the frames in arbitrarily different order from the get_buffer()
> calls.
>
> is it after all this still all that helpfull to know that the frame was
> allocated through ones own get_buffer() ?
If the filter is doing special treatment in the get_buffer function to
work, then not doing it will break the filter.
Besides what's the use of get_buffer if you can override it ? It doesn't
make sense to me.
>>
>> 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.
>
> libmpcodecs has no such limitation ...
> I dont think its filters are that messy as a result.
> Besides its a double sided sword, if we require that buffers passed down
> must have come from get_buffer() then we have another thing that must be
> taken care of, namely no filter ever must break that rule. For some decoders
> that will lead to mandatory memcpy() of the frame
I think no filter must ever break that rule. If the decoder does not
support DR1 then DR1 must be added to this decoder.
>>
>> And this would break yadif as well.
>
> why do you think so?
Because yadif allocates more space to be able to do its processing. If
you don't pass a buffer compatible it will crash.
>>
>> Implementation of pad filter was good as it was, now it's overly
>> complicated.
>
> It did not work before. It segfaulted, i also changed vflip to make sure
> it passed only buffers from pads get_buffer() locally as test, it still
> segfaults without my changes.
vflip is special case IMHO, messing with linesize is not error prone,
but my gut feeling is that many filters do not support messing with
linesize.
I think this discussion is leading to a more complicated issue, namely
how can a filter (like yadif) ensure some mandatory requirements on the
buffer allocated. Currently get_buffer is used, if you bypass that
requirement, we need another mechanism.
I'd like the simple case to be handled in a simple manner.
Complicated treatments like messing with linesize should be supported if
wanted, but must not make the simpler treatment a mess.
I expect people to be able to create filters in an easy way without
worrying about hundreds of corner cases.
--
Baptiste COUDURIER
Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer http://www.ffmpeg.org
More information about the ffmpeg-cvslog
mailing list