[FFmpeg-soc] libavfilter audio work - qualification task

[S.N. Hemanth Meenakshisundaram]

On 04/19/2010 12:17 PM, Stefano Sabatini wrote:
> On date Monday 2010-04-19 00:42:37 -0700, S.N. Hemanth Meenakshisundaram encoded:
>    
>> On 04/18/2010 07:44 PM, S.N. Hemanth Meenakshisundaram wrote:
>>      
>> +#include "avfilter.h"
>> +#include<stdio.h>
>> +#include<stdlib.h>
>> +#include<string.h>
>> +#include<inttypes.h>
>> +#include<math.h>
>> +#include "config.h"
>>      
> many of these are not required if you include "avfilter.h", also
> "config.h" should be removed.
>
>    
>> +typedef struct {
>> +    int mode;
>> +    int parity;
>> +    int buffered_tff;
>> +    double buffered_pts;
>> +    AVFilterPicRef *buffered_pic;
>> +    int stride[3];
>> +    uint8_t *ref[4][3];
>> +} YadifContext;
>>      
> Please document all these fields.
>
>    
>> +static inline void * memcpy_pic(void * dst, const void * src,
>> +                                int bytesPerLine, int height,
>> +                                int dstStride, int srcStride)
>>      
> nits: use K&R style, plain_style variables.
>
>    
>
>> +static void store_ref(YadifContext *p, uint8_t *src[3], int src_stride[3], int width, int height) {
>>      
> p is not a good name, yadif or something like that is better.
>
>    
>
>> +static void filter_line_c(YadifContext *p, uint8_t *dst, uint8_t *prev, uint8_t *cur, uint8_t *next, int w, int refs, int parity){
>>      
> Split this overly long line, also p is not a good name.
>
>    
>> +        av_log(p, AV_LOG_ERROR, "Params are : '%d'\n", refs);
>>      
> AV_LOG_DEBUG, also this will crash as YadifContext is not a proper log
> context.
>
>    
>> +#if HAVE_MMX&&  defined(NAMED_ASM_ARGS)
>> +    if(gCpuCaps.hasMMX2) __asm__ volatile("emms \n\t" : : : "memory");
>> +#endif
>>      
> skip this, FFmpeg has no gCpuCaps.
>
>    
>> +        av_log(NULL, AV_LOG_ERROR, "Alloc dims are : '%d, %d, %d, %ld'\n", w, h, i, yadif);
>>      
> use link->dst as the log context, replace AV_LOG_ERROR ->  AV_LOG_INFO
> and use a notation such as: w:%d h:%d
>
>    
>> +        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;
>> +    }
>> +    return 0;
>> +}
>> +
>> +static void continue_buffered_image(AVFilterContext *ctx);
>>      
> Maybe you can avoid this forward declaration re-ordering a little the
> code.
>
>    
>> +extern int correct_pts;
>>      
> Well this shouldn't be an extern int, maybe this should be a parameter
> of the yadif context.
>
>    
>> +
>> +static void start_frame(AVFilterLink *link, AVFilterPicRef *picref)
>> +{
>> +    AVFilterLink *out = link->dst->outputs[0];
>> +
>> +    out->outpic      = avfilter_get_video_buffer(out, AV_PERM_WRITE, picref->w, picref->h);
>> +    out->outpic->pts = picref->pts;
>> +    // out->outpic->fields = picref->fields;
>>      
> Try to provide also a patch for this, it basically requires to add a
> pixel format field flags to AVFilterPicRef, check
> libmpcodecs/mp_image.h for ideas, that may be something like:
> #define AV_PIX_FMT_FIELD_ORDERED      0x01
> #define AV_PIX_FMT_FIELD_TOP_FIRST    0x02
> #define AV_PIX_FMT_FIELD_REPEAT_FIRST 0x04
> ...
>
>    
>
> Regards.
> _______________________________________________
> FFmpeg-soc mailing list
> FFmpeg-soc at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
>
>    

Hi,

I made changes as per the comments above. Deinterlacing now works, 
however there are still some issues I need help on. One of them is also 
seen in the original mplayer yadif :

1. We initially wait for first two frames before calling the 'filter' 
function for the first time. This is so that we have a 'cur' frame and a 
'next' frame. However, we do not have a 'prev' frame at this point. 
'prev' frames are available only for subsequent frames. The problem is 
that the filter_line_c function does a branch depending on the 'prev' 
values even when 'prev' data is unavailable and thus the memory of 
'prev' is uninitialized. This has no visible effect on output video but 
shows up on running valgrind. The issue is not seen in mplayer initially 
because the yadif there uses the assembly version of filter_line by 
default. But on forcing mplayer yadif filter to use filter_line_c, the 
same error is seen in valgrind for mplayer too.

I fixed it here by initializing the memory to zero (av_mallocz instead 
of av_malloc for the ref array in yadif context). Is this a correct fix?


2. The second issue is a leak. I call unref_pic on both input and output 
PicRefs after passing data to the next filter. Yet, memory leaks in 
multiples of PicRef/Pic structure size for modes 1 and 3. For modes 0 
and 2, there is a single PicRef size block leaked at the beginning. 
valgrind shows the memory allocated on calling get_video_buffer is the 
one that is leaked. Why is the leak still seen after calling unref pic? 
I do ensure that for modes 1 and 3, I call unref pic twice for the two 
output frames.

3. We need to wait for two frames initially before being able to do 
deinterlacing but the filter chain does not supply a second input frame 
until the first is processed and passed on. So am sending an initial 
blank (all green) frame at the start of the video alone. Is this ok?

The diff is attached.

Regards,


-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: yadif.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100425/9034a8a9/attachment.asc>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: avfilter.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100425/9034a8a9/attachment.txt>