[FFmpeg-soc] libavfilter audio work - qualification task

[S.N. Hemanth Meenakshisundaram]

On 04/09/2010 04:18 PM, Stefano Sabatini wrote:
> User-documentation for filters should be put in doc/libavfilter.texi,
> also I don't like this syntax, possibly we should try hard at giving a
> consistent interface to the user, so we should either use
> PARAM-1:PARAM-2:...:PARAM-N either use libavutil/parseutils.c (which
> still depends on opt.c so I don't believe Michail won't accept this to
> be included in the main repo, but that's not a problem for now).
>    
Done. Moved documentation to libavfilter.texi, syntax is now of the form :
./ffplay -vfilters 
drawtext=font=FreeSerif.ttf:text=TestText:file=tfil.txt:x=100:y=50:size=24:fgcolor=Yellow:bgcolor=Red:outline=1:bg=1 
in.avi

Is this too descriptive?
>    
> please avoid unnecessary headers inclusion
Removed 3 unnecessary headers.
>> +
>> +#define RGB_TO_YUV(rgb_color, yuv_color) do { \
>> +  yuv_color[0] = (FIX(0.29900)    * rgb_color[0] + FIX(0.58700) * rgb_color[1] + FIX(0.11400) * rgb_color[2] + ONE_HALF)>>  SCALEBITS; \
>> +  yuv_color[2] = ((FIX(0.50000)   * rgb_color[0] - FIX(0.41869) * rgb_color[1] - FIX(0.08131) * rgb_color[2] + ONE_HALF - 1)>>  SCALEBITS) + 128; \
>> +  yuv_color[1] = ((- FIX(0.16874) * rgb_color[0] - FIX(0.33126) * rgb_color[1] + FIX(0.50000) * rgb_color[2] + ONE_HALF - 1)>>  SCALEBITS) + 128; \
>> +} while (0)
>>      
> use the macro defined in "libavcodec/colorspace.h"
>    
Done.
>> +#define COPY_3(dst,src) { \
>> +    dst[0]=src[0]; \
>> +    dst[1]=src[1]; \
>> +    dst[2]=src[2]; \
>> +}
>>      
> I don't think this is necessary, a simple memcpy should be enough.
>    
Done.
> please use DrawTextContext, no need to use obfuscated names, chars are
> cheapers these days.
>
> Also all the variables should be documented when their meaning is not
> obvious.
>    
Changed to DrawText, added comments for variables.
> Are you sure all these formats are supported?
>    
Only YUV pixel formats supported for now. Updated query_formats.
> av_parse_color()
>    
Done. Removed the local parse_color function.
> General stylistic note: use K&R style,
> if (cond) {
>    ...
> }
> style is preferred, plain_variable is preferred over camelVariable
> style, try also the patcheck tool for checking the most common trivial
> errors.
>
>    
Fixed style.
>> +        av_log(NULL, AV_LOG_ERROR, "Could not load FreeType (error# %d).\n", err);
>>      
> Use the filter context for logging.
>    
Fixed.
>> +        return -1;
>>      
> I'd like to start to use meaningful error codes in libavfilter, use
> AVERROR(EINVAL) in this case.
>    
Done. It looks like invalid value and unknown error both are EINVAL. Is 
this OK?
> All this can be greatly simplified and made more robust using
> parseutils.c.
>    
Using parseutils function now.
> [...]
>    
>> +AVFilter avfilter_vf_drawtext = {
>> +    .name      = "drawtext",
>>      
> missing long name.
>    
Added description.

Also added FIXME for Unicode support later on as Victor suggested.

Tested font/background colors/sizes, wrap-around text, displaying from 
file, drawing box and outline around text, ran valgrind.

Attached is the diff of vf_drawtext.c, allfilters.c, libavfilter.texi 
and libavfilter Makefile. Diff is with soc/libavfilter repo and not 
trunk. However configure.diff has configure script changes with respect 
to trunk.

Currently freetype requires complete font path to be specified. Should 
we add code for retrieving FONTPATH and appending to font file name or 
should the user be expected to provide it?

Please let me know if anything else needs to be fixed.

Ronald,

I will use just memcpy instead of fast_memcpy. Is that OK?

Thanks,
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: drawtext.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100410/527b44b3/attachment.txt>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: configure.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100410/527b44b3/attachment.asc>