[FFmpeg-soc] libavfilter audio work - qualification task

[S.N. Hemanth Meenakshisundaram]

On 04/11/2010 03:51 AM, Stefano Sabatini wrote:
> On date Saturday 2010-04-10 18:59:37 -0700, S.N. Hemanth Meenakshisundaram encoded:
>    
>
>>       REGISTER_FILTER (VFLIP,       vflip,       vf);
>> +    REGISTER_FILTER (DRAWTEXT,    drawtext,    vf);
>>      
> Alphabetical order.
>    
Fixed.
> Please add a table with the meaning of all the parameters. Check the
> pad filter documentation to see how.
>    
Added table to libavfilter.texi
> [...]
>    
>> Index: Makefile
>> ===================================================================
>>   OBJS-$(CONFIG_VFLIP_FILTER)                  += vf_vflip.o
>> +OBJS-$(CONFIG_DRAWTEXT_FILTER)               += vf_drawtext.o
>>      
> Alphabetical order.
>    
Fixed.
>> + * vf_drawtext.c: print text over the screen
>>      
> No need for this, just put the copyright, description may stay in the
> @file doxy (also please mention libfreetype in this description).
>    
Done
>> +#include<stdio.h>
>> +#include<stdlib.h>
>>      
> These should be unnecessary.
>    
Removed
>    
>> +#undef time
>>      
> Why undef time? (I know they were in the vhook filter, but there is
> some reason for it?).
>    
Without undef time, ffmpeg makefile rules throw a "time is forbidden for 
security reasons" error when building.
>> +#define ONE_HALF  (1<<  (SCALEBITS - 1))
>> +#define FIX(x)    ((int) ((x) * (1<<SCALEBITS) + 0.5))
>>      
> These and scalebits are already defined in colorspace.h.
>    
Removed redundant definitions already present in colorspace.h
>> +#define SET_PIXEL(picture, yuv_color, x, y) { \
>> +    picture->data[0][ (x) + (y)*picture->linesize[0] ] = yuv_color[0]; \
>> +    picture->data[1][ ((x/2) + (y/2)*picture->linesize[1]) ] = yuv_color[1]; \
>> +    picture->data[2][ ((x/2) + (y/2)*picture->linesize[2]) ] = yuv_color[2]; \
>> +}
>> +
>> +#define GET_PIXEL(picture, yuv_color, x, y) { \
>> +    yuv_color[0] = picture->data[0][ (x) + (y)*picture->linesize[0] ]; \
>> +    yuv_color[1] = picture->data[1][ (x/2) + (y/2)*picture->linesize[1] ]; \
>> +    yuv_color[2] = picture->data[2][ (x/2) + (y/2)*picture->linesize[2] ]; \
>> +}
>>      
> These macros only work with YUV420P. Also please define them near to
> where they're used.
>    
Moved near place of use. Modified to work for other planar formats as 
well with hsub, vsub.
>> +typedef struct {
>> +    const AVClass *class;             /* To help parseutils to fill in structure */
>>      
> Nit: use ///<  style for inline doxygen comments.
>
> Nit: I prefer to lowcase the first letter if the text describing the
> field is not a complete sentence (missing the verb like here), check
> how it is done in main SVN filters.
>    
Fixed.
>
> Nit: "color"
>
> fgcolor description should explicitely mention the term "ForeGround"
> or you'll have a confused reader, also maybe these fields are not
> necessary at all.
>    
> Having two variables names bcolor and bgcolor is horribly confusuing,
> I suggest:
> bgcolor_string
> bgcolor
>    
> unexplicative name
>    
All of these are fixed. Removed the input parameters to a separate 
structure (InParams) so that shorthand parameter names f, t, T etc can 
be used there as Michael suggested. The context only has values it 
requires throughout filter operation now and all its variables are 
descriptive.

>> +    int yMax, yMin;
>>      
> y_max, y_min
>    
Changed.
>> +    char *arg_cpy = NULL;
>> +    FT_BBox bbox;
>> +    DrawTextContext *dtext = ctx->priv;
>> +
>> +    dtext->class =&drawtext_class;
>> +    dtext->font = NULL;
>> +    dtext->text = NULL;
>> +    dtext->file = NULL;
>> +    dtext->fgcolor = NULL;
>> +    dtext->bgcolor = NULL;
>> +    dtext->x = dtext->y = 0;
>> +    dtext->size=DEF_DRAWTEXT_FONT_SZ;
>>      
> av_opt_set_defaults2()
>    
Using set_defaults2 now.
>> +    dtext->fcolor[0]=255;
>> +    dtext->fcolor[1]=128;
>> +    dtext->fcolor[2]=128;
>> +    dtext->bcolor[0]=0;
>> +    dtext->fcolor[1]=128;
>> +    dtext->fcolor[2]=128;
>>      
> you can avoid to explicitely initialize these, use:
> dtext->fgcolor_string = strdup("white");
> dtext->bgcolor_string = strdup("black");
>    
Done.
>> +    dtext->bg = 0;
>> +    dtext->outline = 0;
>> +    dtext->text_height = 0;
>> +
>> +    arg_cpy = av_strdup(args);
>>      
> why this?
>    
Removed. Initially thought set_options string would modify args.
>> +        av_log(dtext, AV_LOG_ERROR, "No font file provided! (-f filename)\n");
>>      
> "-f filename" is misleading as we don't support that syntax anymore,
> same below.
>    
Fixed error messages.
>> +        if ((fp=fopen(dtext->file, "r")) == NULL) {
>>      
> please use "fp = fopen" for increased readability, also !FOO is
> preferred over FOO == NULL
>    
Fixed.
>> +        }
>> +        else {
>>      
> nit: "} else {" on the same line, here and below.
>    
Changed.
>> +        av_log(dtext, AV_LOG_ERROR, "Could not load FreeType (error# %d).\n", err);
>> +        return AVERROR(EINVAL);
>> +    }
>>      
> Try to use libfreetype error messages, they have a weird system for
> setting errors,[...]
>    
Done. Using a function for converting error messages. Is that a problem?
>> +    for (j = 0; (j<  height); j++)
>> +    for (i = 0; (i<  width); i++) {
>> +        SET_PIXEL(picture, yuv_color, (i+x), (y+j));
>> +    }
>>      
> weird indent.
>
> Also this can be done much faster with a memcpy, see how it is done in
> vf_pad.c:draw_rectangle.
>    
Using memset instead of memcpy. Works as required.
>> +    AVPicture *pic = (AVPicture *)&pic_ref->data;
>>      
> This cast is not needed, you can just work on the AVFilterPicRef, and
> avoid a dependancy on libavcodec.
>    
Fixed. Using only PicRef now.
>> +    .description = "Draw text on top of video frames.",
>>      
> please mention libfreetype in the description
>    
Done.
> Regards
> _______________________________________________
> FFmpeg-soc mailing list
> FFmpeg-soc at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
>
>    
Also fixed alignment and style wherever non standard. Please review and 
let me know if there's anything else to be fixed. I will now work on 
fixing the corruption in vf_yadif. The complete diff for drawtext, 
documentation, Makefile/allfilters changes is attached below.
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: drawtext.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100421/2ef5f0cf/attachment.txt>