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

[Michael Niedermayer]

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.

[...]
> +        if(picture.linesize[2])
> +            picture.linesize[1] = picture.linesize[2] =
> +                FFMAX(picture.linesize[1],
> +                      picture.linesize[2]);

when is this needed?

and the rest is ok

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Frequently ignored awnser#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.
-------------- 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/20080214/e4bf374c/attachment.pgp>