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

[Vitor Sessak]

Hi

Michael Niedermayer wrote:
> On Thu, Feb 14, 2008 at 09:02:23PM +0100, Vitor Sessak wrote:
>> Hi
>>
>> Michael Niedermayer wrote:
>>> On Tue, Feb 12, 2008 at 06:59:53PM +0100, Vitor Sessak wrote:
>>>> Hi
>>>>
>>>> Michael Niedermayer wrote:
>>>>> On Mon, Feb 11, 2008 at 08:07:10PM +0100, Vitor Sessak wrote:
>>>>> [...]
>>>>>>>>         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? 
>>>>> Ok, but give pix_fmt its proper type not int.
>>>>>
>>>>>
>>>>>> Do you think I should make it inline?
>>>>> no
>>>>>
>>>>>
>>>>>>>>                 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?
>>>>> see avcodec_default_get_buffer() if you figured out a way to clean it up
>>>>> it also does something similar ...
>>>> It's pretty complex. I have two other ideas:
>>>>
>>>> 1- Make avpicture_fill always create aligned linesizes
>>> Will need a fix somewhere in the raw video code i think, as that uses
>>> avpicture_fill and expects linesize=width.
>>>> or
>>>>
>>>> 2- Split avpicture_fill in two functions. One that sets the linesize and 
>>>> other that do all the rest, using linesize[i] and height to figure the 
>>>> sizes. So, to align linesizes
>>>>
>>>> avpicture_fill_linesizes(&pic, ...);
>>>> for (i) align(pic.linesize[i]);
>>>> buf = av_malloc(...);
>>>> avpicture_fill_buf(&pic, buf, ...);
>>>>
>>>>
>>>> and a third option would be trying to create a cleaner version of 
>>>> avcodec_default_get_buffer()...
>>>>
>>>> What do you think?
>>> I dunno, but theres no reason not to cleanup avcodec_default_get_buffer()
>>> if you think tha would help.
>> Ok, so fill_split.diff do what I describe as idea No. 2 and get_buffer.diff 
>> tries to make avcodec_default_get_buffer() a little less obfuscated using 
>> code added in fill_split.diff. It can be simplified further, but it was 
>> already painful to simplify it a little without breaking anything.
>>
>> Regression tests pass.
> 
> Could you please give the new functions a ff_ prefix instead of avpicture_
> and keep them out of public headers like avcodec.h.
> The simplification should definitly be seperate of any API extension.

Ok. But add it to which header file? Should I create a imgconvert.h?

> 
> [...]
>> +        if(picture.linesize[2])
>> +            picture.linesize[1] = picture.linesize[2] =
>> +                FFMAX(picture.linesize[1],
>> +                      picture.linesize[2]);
> 
> when is this needed?

Never. I misunderstood the original code?

-Vitor