[FFmpeg-devel] [PATCH] [1/??] [2/3] Basic infrastructure

Vitor Sessak vitor1001
Mon Feb 11 20:07:10 CET 2008


Hi

Michael Niedermayer wrote:
> 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

[...]

>> 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

indeed. done.

> 
> 
>> }
>>
>> 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 ...

ok

> 
> 
> [...]
>> 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

ok

> 
> 
> [...]
>>         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

Ok.

> 
>>         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

Well, that would mean duplicating a lot of code already in 
av_picture_copy(), unless I create a
int av_get_plane_bytewidth(int pix_fmt, int width, int plane)
to factorate this code (see attached patch). Are you ok with that? Do 
you think I should make it inline?

> 
> 
>>                 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

It's a bit complicated... I need either to duplicate code in 
avpicture_alloc, either rounding up to the next multiple of 16<<hsub. 
Any ideas?

Also, do you think it is worth also rounding up the height to the next 
multiple of 16 as I've done? Thinking again, it don't look very useful...

-Vitor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: get_plane_bwidth.diff
Type: text/x-patch
Size: 2031 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080211/318e83bd/attachment.bin>



More information about the ffmpeg-devel mailing list