[FFmpeg-cvslog] r26315 - trunk/libavfilter/vf_pad.c
Michael Niedermayer
michaelni
Wed Jan 12 05:12:17 CET 2011
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
>
> 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.
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
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() ?
>
> 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
>
> And this would break yadif as well.
why do you think so?
>
> 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.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Avoid a single point of failure, be that a person or equipment.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-cvslog/attachments/20110112/f8f6d2e5/attachment.pgp>
More information about the ffmpeg-cvslog
mailing list