[FFmpeg-soc] libavfilter audio work - qualification task

[S.N. Hemanth Meenakshisundaram]

On 04/18/2010 07:44 PM, S.N. Hemanth Meenakshisundaram wrote:
> On 04/18/2010 05:10 PM, Stefano Sabatini wrote:
>> On date Sunday 2010-04-18 16:10:35 -0700, S.N. Hemanth 
>> Meenakshisundaram encoded:
>>> On 04/13/2010 12:05 AM, Stefano Sabatini wrote:
>>>> On date Monday 2010-04-12 22:52:44 -0700, S.N. Hemanth 
>>>> Meenakshisundaram encoded:
>>>> [...]
>>>>> I had one (dumb?) question about yadif :
>>>>>
>>>>> Since the PicRef structure in libavfilter has no 'fields' member,
>>>>> how do I find out if the image data in the buffer is
>>>>> MP_IMGFIELD_ORDERED, MP_IMGFIELD_TOP_FIRST etc. Is it indicated in
>>>>> the PixelFormat?
>>>> Ehm no, I believe we need to add it to the AVFilterPicRef struct.
>>>>
>>> Hi,
>>>
>>> I have a couple more questions :
>>>
>>> 1. It looks like we output two frames for every input frame/field if
>>> mode is 1 or 3, else we output a single frame. Is this right?
>> > From the manual page of mplayer:
>>
>>         yadif=[mode[:field_dominance]]
>>                Yet another deinterlacing filter
>> <mode>
>>                        0: Output 1 frame for each frame.
>>                        1: Output 1 frame for each field.
>>                        2: Like 0 but skips spatial interlacing check.
>>                        3: Like 1 but skips spatial interlacing check.
>>
>> So yes you look right.
>>
>>> 2. The mplayer code has a correct_pts variable that seems to allow
>>> doubling of frame rate. Here is it safe to assume this is always
>>> allowed/true?
>> I suppose so, that will also depend on the mode, from what I can
>> understand you'll need to duplicate the frame rate only with mode 1/3,
>> this will be done manipulating the output pts.
>>
>>> 3. If I add a 'fields' member to AVFilterPicRef struct, how do I
>>> make sure it will be set to the right value (i.e. TOP_FIRST etc)?
>> It is a problem of the calling application to set this, for example an
>> application requests a picref with avfilter_get_video_buffer() and
>> sets the field accordingly. In the case of ffmpeg.c/ffplay.c this
>> depends on the options set in the codec context or by the user
>> (e.g. -top_field 1).
>>
>>> Thanks
>> Regards.
> Thank you. Attached is an initial vf_yadif.c - I have only compiled it 
> yet and will start testing it shortly. A couple of questions :
>
> 1. I wasn't sure how to modify the header since we wanted to keep 
> yadif under the GPL while the rest of FFmpeg is under the LGPL.
>
> 2. Is the PixelFormat YUV420P equivalent to YV12 or IYUV/I420. The 
> description for YUV420P doesn't seem to say if the 2nd and 3rd planes 
> are VU or UV. Where can I find the other PixelFormat equivalent.
>
> Regards,

Hi,

Videos do play with vf_yadif in the filter chain, but there are a couple 
of problems right now. Am working on fixing them.

The first is a memory corruption reported by valgrind :

==30358== Conditional jump or move depends on uninitialised value(s)
==30358==    at 0x418218: filter_line_c (vf_yadif.c:331)
==30358==    by 0x418762: end_frame (vf_yadif.c:381)

==30358==  Uninitialised value was created by a heap allocation
[...]
==30358==    by 0x90D187: av_malloc (mem.c:83)
==30358==    by 0x4183D1: config_props_input (vf_yadif.c:405)

The offending piece of code seems to be :

filter_line_c :

         int spatial_score= FFABS(cur[-refs-1] - cur[+refs-1]) + FFABS(c-e)
                          + FFABS(cur[-refs+1] - cur[+refs+1]) - 1;

#define CHECK(j)\
     {   int score= FFABS(cur[-refs-1+j] - cur[+refs-1-j])\
                  + FFABS(cur[-refs  +j] - cur[+refs  -j])\
                  + FFABS(cur[-refs+1+j] - cur[+refs+1-j]);\
         if(score < spatial_score){\
             spatial_score= score;\
             spatial_pred= (cur[-refs  +j] + cur[+refs  -j])>>1;\

Line 331:     CHECK(-1) CHECK(-2) }} }}

The allocation is here :

     for(i=0; i<3; i++) {
         int is_chroma= !!i;
         int w= ((link->w     + 31) & (~31))>>is_chroma;
         int h= ((link->h + 6 + 31) & (~31))>>is_chroma;

         av_log(NULL, AV_LOG_ERROR, "Alloc dims are : '%d, %d, %d, 
%ld'\n", w, h, i, yadif);
         yadif->stride[i]= w;
         for(j=0; j<3; j++)
             yadif->ref[j][i]= (uint8_t 
*)(av_malloc(w*h*sizeof(uint8_t)))+3*w;


Am wondering if this is because the store_refs function does a :

     memcpy (p->ref[3], p->ref[0], sizeof(uint8_t *)*3);
     memmove(p->ref[0], p->ref[1], sizeof(uint8_t *)*3*3);

Isn't ref[3] unallocated at this time? This code is from the mplayer 
yadif, so I guess am doing something wrong here.


The 2nd issue is strange and might be related :

The av_log statement above

         av_log(NULL, AV_LOG_ERROR, "Alloc dims are : '%d, %d, %d, 
%ld'\n", w, h, i, yadif);

throws a segmentation fault in av_log code if I use yadif (which is a 
YadifContext * pointer) instead of NULL as the logging context. I tried 
logging the context pointer and it is not NULL and appears valid.

Am working on this, but please let me know if there's something I am 
missing. Full diff is again attached.

Thanks,
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: yadif.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100419/54056ea5/attachment.asc>