[FFmpeg-soc] libavfilter audio work - qualification task

S.N. Hemanth Meenakshisundaram smeenaks at ucsd.edu
Mon May 3 10:11:07 CEST 2010 

On 04/23/2010 05:03 PM, Stefano Sabatini wrote:
> On date Thursday 2010-04-22 17:19:16 -0700, S.N. Hemanth Meenakshisundaram encoded:
> [...]
>    
>> [...]
>>
>> 4. User now needs to provide only one of text or file, if both are
>> given then file is used.
>>      
> Uhmmm... this may be a source of errors so not sure it is a good idea,
> I would prefer to just bail out if both options are used.
>
>    

Done. If both options are specified, we bail out now.

>> 10. Updated documentation with new parameter names, strftime
>> behavior and the file/text requirement.
>>      
> Note: the strfmt syntax may be extended, for example it would be
> rather useful to be able to print some of the information about the
> frame/link, e.g. PTS, number of frame, format, size, etc.,
> this may be done eventually in a further patch...
>    

I'll add this in a later patch.

>
>    
>> As for the alpha channel support in drawbox function, do we need to
>> update the alpha values? Should the caller be allowed to specify
>> bgcolor or fgcolor in #RRGGBBAA format?
>>      
> Why not? That would be cool don't you agree? ;-) If looks not trivial
> skip it, that may be eventually added in a further patch.
>    

Added alpha support for drawing text and outline but not for the 
background box yet.
This is simple to do but I would have to start setting each pixel 
instead of one stride at
a time as is done currently. Is this ok?

Checking for localtime_r support now and fixed other nits below. All the 
diffs for drawtext are attached at the bottom.

>> [...]
>> +static const AVOption drawtext_options[]= {
>> +{"font", "set font",       OFFSET(font),           FF_OPT_TYPE_STRING, 0,  CHAR_MIN, CHAR_MAX },
>> +{"text", "set text",       OFFSET(text),           FF_OPT_TYPE_STRING, 0,  CHAR_MIN, CHAR_MAX },
>> +{"file", "set file",       OFFSET(file),           FF_OPT_TYPE_STRING, 0,  CHAR_MIN, CHAR_MAX },
>>      
> Just an idea, file/font/text may be misleading, I suggest to use:
> fontfile
> textfile
> text
>    

>> +{"fgcolor", "set fgcolor", OFFSET(fgcolor_string), FF_OPT_TYPE_STRING, 0,  CHAR_MIN, CHAR_MAX },
>>      
> set foreground color
>    
>> +{"bgcolor", "set bgcolor", OFFSET(bgcolor_string), FF_OPT_TYPE_STRING, 0,  CHAR_MIN, CHAR_MAX },
>>      
> same
>    
>> +{"size", "set size",       OFFSET(size),           FF_OPT_TYPE_INT,   16,         1,       72 },
>>      
> "set font size"
>    

>> +    if (color_str&&  (av_parse_color(rgba, color_str, ctx))) {
>> +        return -1;
>>      
> Is the color_str check necessary?
>
> if (...&&  ((err = av_parse_color(...)))
>     return err;
>
>    
>> +    if (av_set_options_string(dtext, args, "=", ":")<  0) {
>>      
> I'd prefer:
> if ((ret = ...)<  0) {
>     av_log(...)
>     return ret;
> }
>
> here and when we check an internal error code.
>
> We don't know the cause of failure, it could well be a memory problem
> for example, even if the provided parameter is allright.
>
>    
>> +    if (!dtext->font || *(dtext->font) == 0) {
>>      
> Is the second check necessary?
>
>    
>> +        av_log(ctx, AV_LOG_ERROR, "No font file provided! (=f:filename)\n");
>>      
> "(=f:filename)", that's quite confusing, make it more clear or remove that.
>
>    
>> +        return AVERROR(EINVAL);
>> +    }
>> +
>> +    if (dtext->file&&  *(dtext->file)) {
>> +        FILE *fp;
>>      
> Maybe a check could be added here, if the text is already defined then
> issue an error.
>
>    
>> +        if (!(fp = fopen(dtext->file, "r"))) {
>> +            av_log(ctx, AV_LOG_WARNING, "WARNING: The file could not be opened.\n");
>>      
> I'd skip the "WARNING" here and below, this may conflict with how the
> user overrides the default log callback.
>
>
>    
>> +        } else {
>> +            uint16_t read_bytes;
>> +            char *tbuff = av_malloc(MAX_TEXT_SIZE);
>>      
> missing check. See the function read_file() in cmdutils.c.
>
>    
>> +            read_bytes = fread(tbuff, sizeof(char), MAX_TEXT_SIZE-1, fp);
>>      
> Nit: dont' break the "..." string, put it on the second line.
>
>    
>> +            file_valid = 1;
>>      
> What's the point of file_valid?
>
>    
>> +    if (extract_color(ctx, dtext->fgcolor_string, dtext->fgcolor)) {
>> +        av_log(ctx, AV_LOG_ERROR, "Invalid foreground color: '%s'.\n", dtext->fgcolor_string);
>> +        return AVERROR(EINVAL);
>> +    }
>>      
> if ((err = extract_color)<  0) {
>     av_log(...);
>     return err;
>
> Same below.
>
>    
>> +
>> +    if (extract_color(ctx, dtext->bgcolor_string, dtext->bgcolor)) {
>> +        av_log(ctx, AV_LOG_ERROR, "Invalid foreground color: '%s'.\n", dtext->fgcolor_string);
>>      
>                                                ^^^^                               ^^
> ehm..
>
>    
>
>> +    strftime(buff, sizeof(buff), text, localtime_r(&now,&ltime));
>>      
> Missing check on the presence of localtime_r().
>    
>
>> +It accepts the following parameters:
>> +font=<font file>:text=<text>:file=<text file>:x=<x offset>:y=<y offset>:
>> +fgcolor=<foreground color>:bgcolor=<background color>:size=<font size>:box=<draw box>:
>> +outline=<draw outline>
>>      
> I find this a little confusing, as this seems to imply that the
> parameters have to be provided in a fixed order. Maybe something of
> the kind:
> It accepts a list of $var{key}=var{value} pairs as parameters,
> separated by ":".
>
>
>    
>> +If a date/time format string is passed in place of text, the current
>> +date and time are drawn in the specified format.
>>      
> I don't like that "in place of". Maybe something of the type:
>
> The filter recognizes strftime() sequences in the provided text and
> expand them accordingly. Check the documentation of strftime().
>
> [...]
>
> Regards.
>    

The new diffs follow:


-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: drawtext.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100503/46bd5b39/attachment.txt>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: config.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100503/46bd5b39/attachment.asc>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: make.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100503/46bd5b39/attachment-0001.txt>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: doc.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100503/46bd5b39/attachment-0001.asc>

More information about the FFmpeg-soc mailing list