[FFmpeg-devel] [PATCH] [1/??] [2/3] Basic infrastructure
Michael Niedermayer
michaelni
Mon Feb 11 00:36:38 CET 2008
On Sun, Feb 10, 2008 at 09:56:35PM +0100, Vitor Sessak wrote:
> Hi
>
> Michael Niedermayer wrote:
>> On Sun, Feb 10, 2008 at 05:59:43PM +0100, Vitor Sessak wrote:
>>> Hi
>>>
>>> Michael Niedermayer wrote:
>>>> On Sun, Feb 10, 2008 at 11:03:48AM +0100, Vitor Sessak wrote:
>>>>> Hi and thanks for the review
>>>> [...]
>>>>>>> switch(link->init_state) {
>>>>>>> case AVLINK_INIT:
>>>>>>> continue;
>>>>>>> case AVLINK_STARTINIT:
>>>>>>> av_log(filter, AV_LOG_ERROR, "circular filter chain
>>>>>>> detected\n");
>>>>>>> return -1;
>>>>>>> case AVLINK_UNINIT:
>>>>>>> link->init_state = AVLINK_STARTINIT;
>>>>>>>
>>>>>>> if(avfilter_config_links(link->src))
>>>>>>> return -1;
>>>>>>>
>>>>>>> if(!(config_link = link_spad(link).config_props))
>>>>>>> config_link = avfilter_default_config_output_link;
>>>>>>> if(config_link(link))
>>>>>>> return -1;
>>>>>>>
>>>>>>> if((config_link = link_dpad(link).config_props))
>>>>>>> if(config_link(link))
>>>>>>> return -1;
>>>>>>>
>>>>>>> link->init_state = AVLINK_INIT;
>>>>>>> }
>>>>>>> }
>>>>>> what does the above mean for filter graphs like:
>>>>>>
>>>>>> ->mix--->duplicate--->
>>>>>> ^ |
>>>>>> | v
>>>>>> \------delay
>>>>>>
>>>>>> aka an infinite impulse response video filter :)
>>>>>> it looks like it would return -1 for that ...
>>>>> What should be done in these cases? Something like av_log("Circular
>>>>> video chain, expect trouble\n")?
>>>> Well, id say avfilter should work with circular chains as well. Of
>>>> course
>>>> not with all, one can easily build ones which would deadlock ...
>>>> Actually most random circular chains would deadlock, but above would not
>>>> as long as the duplicate filter is carefully implemented. That is if
>>>> a request from the delay filter would be satifies with whatever last
>>>> frame the duplicate filter has and never be passed on to the mix filter.
>>> Ok. So I think a warning is more appropriated.
>>>
>>>> [...]
>>>>> All the other points I don't mention, agreed and changed. New code in
>>>>> http://svn.mplayerhq.hu/soc/libavfilter/avfilter.c?content-type=text%2Fplain&view=co
>>>>> (Diego, nits changed in the soc tree too).
>>>> This is just one file the patch contained more, thus i cant review it
>>>> and
>>>> you should send patches anyway instead of links to non constant files.
>>> Ok. Attached.
>> [...]
>>> int avfilter_poll_frame(AVFilterLink *link);
>> this should be where avfilter_request_frame, avfilter_start_frame, ... are
>> and also have a doxy
>>> /**
>>> * A filter pad used for either input or output.
>>> */
>>> struct AVFilterPad
>>> {
>>> /**
>>> * Pad name. The name is unique among inputs and among outputs, but
>>> an
>>> * input may have the same name as an output. This may be NULL if
>>> this
>>> * pad has no need to ever be referenced by name.
>>> */
>>> const char *name;
>>>
>>> /**
>>> * AVFilterPad type. Only video supported now, hopefully someone
>>> will
>>> * add audio in the future.
>>> */
>>> int type;
>>> #define AV_PAD_VIDEO 0 ///< video pad
>> CodecType could be used here
>> [...]
>>> /* TODO: set the buffer's priv member to a context structure for the
>>> whole
>>> * filter chain. This will allow for a buffer pool instead of the
>>> constant
>>> * alloc & free cycle currently implemented. */
>>> AVFilterPicRef *avfilter_default_get_video_buffer(AVFilterLink *link, int
>>> perms)
>>> {
>>> AVFilterPic *pic = av_mallocz(sizeof(AVFilterPic));
>>> AVFilterPicRef *ref = av_mallocz(sizeof(AVFilterPicRef));
>>>
>>> ref->pic = pic;
>>> ref->w = link->w;
>>> ref->h = link->h;
>>>
>>> /* make sure the buffer gets read permission or it's useless for
>>> output */
>>> ref->perms = perms | AV_PERM_READ;
>>>
>>> pic->refcount = 1;
>>> pic->format = link->format;
>>> pic->free = avfilter_default_free_video_buffer;
>>> avpicture_alloc((AVPicture *)pic, pic->format, ref->w, ref->h);
>> This will set linesize == w*pixel_size while many filters will need
>> linesize
>> to be a multiple of 16 (due to SSE not likring misaligned accesses).
>
> Agreed with everything. New patch attached.
[...]
> void avfilter_insert_pad(unsigned idx, unsigned *count, size_t padidx_off,
> AVFilterPad **pads, AVFilterLink ***links,
> AVFilterPad *newpad)
> {
> unsigned i;
>
> idx = FFMIN(idx, *count);
>
> *pads = av_realloc(*pads, sizeof(AVFilterPad) * (*count + 1));
> *links = av_realloc(*links, sizeof(AVFilterLink*) * (*count + 1));
> memmove(*pads +idx+1, *pads +idx, sizeof(AVFilterPad) * (*count-idx));
> memmove(*links+idx+1, *links+idx, sizeof(AVFilterLink*) * (*count-idx));
> memcpy(*pads+idx, newpad, sizeof(AVFilterPad));
> (*links)[idx] = NULL;
>
> (*count) ++;
> for(i = idx+1; i < *count; i ++)
> if(*links[i])
> (*(unsigned *)((uint8_t *)(*links[i]) + padidx_off)) ++;
the () around links seem superflous
> }
>
> int avfilter_link(AVFilterContext *src, unsigned srcpad,
> AVFilterContext *dst, unsigned dstpad)
> {
> AVFilterLink *link;
>
> if(src->output_count <= srcpad || dst->input_count <= dstpad ||
> src->outputs[srcpad] || dst->inputs[dstpad])
> return -1;
>
> src->outputs[srcpad] =
> dst->inputs[dstpad] = link = av_mallocz(sizeof(AVFilterLink));
this can be aligned nicer:
src->outputs[srcpad] =
dst-> inputs[dstpad] = link = av_mallocz(sizeof(AVFilterLink));
[...]
> if((config_link = link_dpad(link).config_props))
> if(config_link(link))
> return -1;
indention ...
[...]
> int avfilter_request_frame(AVFilterLink *link)
> {
> if(link_spad(link).request_frame)
> return link_spad(link).request_frame(link);
> else if(link->src->inputs[0])
> return avfilter_request_frame(link->src->inputs[0]);
> else return -1;
> }
>
> int avfilter_poll_frame(AVFilterLink *link)
> {
> int i, min=INT_MAX;
>
> if(link_spad(link).poll_frame)
> return link_spad(link).poll_frame(link);
> else
> for (i=0; i<link->src->input_count; i++) {
> if(!link->src->inputs[i])
> return -1;
> min = FFMIN(min, avfilter_poll_frame(link->src->inputs[i]));
> }
the elses arent needed due to the returns
[...]
> src[0] = link->srcpic-> data[0] + y * link->srcpic-> linesize[0];
> dst[0] = link->cur_pic->data[0] + y * link->cur_pic->linesize[0];
> for(i = 1; i < 4; i ++) {
> if(link->srcpic->data[i]) {
> src[i] = link->srcpic-> data[i] + (y >> vsub) * link->srcpic-> linesize[i];
> dst[i] = link->cur_pic->data[i] + (y >> vsub) * link->cur_pic->linesize[i];
> } else
> src[i] = dst[i] = NULL;
> }
the src/dst[0] can be put in the loop and then this and te next loop can be
merged
>
> for(i = 0; i < 4; i ++) {
> if(!src[i]) continue;
>
> for(j = 0; j < h >> (i==0 ? 0 : vsub); j ++) {
> memcpy(dst[i], src[i], link->cur_pic->linesize[i]);
this should be width*pixel size or something like that ideally
> src[i] += link->srcpic ->linesize[i];
> dst[i] += link->cur_pic->linesize[i];
> }
> }
[...]
> if(!link_dpad(link).draw_slice)
> return;
>
> link_dpad(link).draw_slice(link, y, h);
> }
if(link_dpad(link).draw_slice)
link_dpad(link).draw_slice(link, y, h);
[...]
> avpicture_alloc((AVPicture *)pic, pic->format,
> (ref->w + 15) & (~15), // make linesize a multiple of 16
wont work for chroma
except these the patch looks ok and can be commited
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080211/1e84d7e0/attachment.pgp>
More information about the ffmpeg-devel
mailing list