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

[Michael Niedermayer]

On Fri, Feb 15, 2008 at 07:05:30PM +0100, Vitor Sessak wrote:
> Hi
>
> Michael Niedermayer wrote:
>> On Fri, Feb 15, 2008 at 05:39:31PM +0100, Vitor Sessak wrote:
>>> 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?
>> thats a possibility ...
>
> Since you didn't say explicitly yes, here is the patch (it also makes the 
> function I added in another patch in this thread not to be in the public 
> API).
>

ok

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

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus
-------------- 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/20080215/e4b3b097/attachment.pgp>