[FFmpeg-soc] expand filter (alternative to pad syntax)

[Vitor Sessak]

Hi

vmrsss wrote:
> Hi everybody
> 
>     I attach an improved version of vf_pad.c, thoroughly tested, with a 
> few questions for you.
> 
> (1)
> On 15 May 2008, at 10:30, Víctor Paesa wrote:
>> Try sscanf(arg,"%d:%d:%d:%d:%255[^:]:%255[^:]:%d",&a,&b,&c,&d,e,f,&g);
> 
> I am having a problem with inputs like
> 
>     pad=::::16/9:16
> 
> (which should mean "expand to a 16/9 frame mod 16"), because sscanf 
> can't find the %d in between the ::, and so the conversion stops. 
> Ideally the form above should be allowed, lest one needs to ask users to 
> type in clumsy forms with default values as in:
> 
>     pad=-1:-1:-1:-1:16/9:16
> 
> (which is what the code currently requires). Is there to get sscanf do 
> it? I don't think so. One alternative is to use sscanf to read args one 
> ":" at a time, as I currently do for two of the parameters which can 
> take different forms (aspect and color).

I don't know very well sscanf, so I can't help you much with it...

> (2) Following up on the above, would a form like
> 
>     pad=w=...:h=...:x=...:y=...:a=...:r=...:c=....
> 
> be desirable? 

It depends if things like

pad=-10:-10:c=4

will be allowed. If we could maintain some compatibility with the 
mplayer vf_expand filter syntax, it'll make it easier to make the 
mplayer filter deprecated one day...

> It'd allow to omit parameters and list them in any order, 
> but for filters with few parameters might be a cumbersome overkill. Eg
> 
>     scale=624:368 against scale=w=624:h=368
> 
> (3) I have added the possibility of specifying a color for the padding. 
> The structure which lists the colors is taken from vf_drawbox.c. If 
> people like the idea, that can easily be factored out in some 
> avfilter_common file.

Looks nice to me, but I think that before it is ok to be factored in a 
common file (libavfilter/filter_parse_utils.c?), it should accept colors 
in RGB format and have boxcolor.{r,g,b} values.

> (4) The aspect parameter doesn't work as it should for files with SAR 
> different from 1. It's easy to fix, only that the sar information seems 
> unavailable at the filter configuration stage (see the code); I expect 
> Vitor wll be able to help on this one.

I agree that you could just add (move?) the pixel_format variable to 
AVFilterLink struct. It should be pretty simple to make a patch to do it.

> (5) I have not resolved the issue of removing memcpy. There is only one 
> call to memcpy in the code; to get rid of it is likely to require some 
> significant change to the avfilter API, that will have to involve Vitor. 
> In these circumstances, I wonder whether it would make sense to commit 
> the code to SOC anyway, and then remove the one call to memcpy when 
> we'll know how. (Observe that this situation is common to several filters.)

I've discussed it a little with vmrsss in pvt and I think the solution 
to it would be to negotiate the link dimensions during graph creation as 
we do for pix_fmt. But after this is done, the changes to make vf_pad.c 
memcpy-less would be rather minor, so I think this filter is better 
commited to the soc tree (after review) instead of bitrotting as a patch 
in the mailing list.

> /*
>  * Video pad filter
>  * copyright (c) 2008 vmrsss
>  *
>  * This file is part of FFmpeg.
>  *
>  * FFmpeg is free software; you can redistribute it and/or
>  * modify it under the terms of the GNU Lesser General Public
>  * License as published by the Free Software Foundation; either
>  * version 2.1 of the License, or (at your option) any later version.
>  *
>  * FFmpeg is distributed in the hope that it will be useful,
>  * but WITHOUT ANY WARRANTY; without even the implied warranty of
>  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>  * Lesser General Public License for more details.
>  *
>  * You should have received a copy of the GNU Lesser General Public
>  * License along with FFmpeg; if not, write to the Free Software
>  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>  */
> 
> #include <string.h>
> #include <stdio.h>
> 
> #include "avfilter.h"
> 
> typedef struct
> {
>     const char *name;
>     unsigned char y;
>     unsigned char cb;
>     unsigned char cr;
> } box_color;
> 
> #define NUM_COLORS 8
> static box_color colors[] =
> {
>     {.name = "blue",   .y =  40, .cr = 109, .cb = 240},
>     {.name = "red",    .y =  81, .cr = 240, .cb =  90},
>     {.name = "black",  .y =  16, .cr = 128, .cb = 128},
>     {.name = "white",  .y = 255, .cr = 128, .cb = 128},
>     {.name = "green",  .y = 144, .cr =  34, .cb =  53},
>     {.name = "yellow", .y = 210, .cr = 146, .cb =  16},
>     {.name = "gray",   .y = 128, .cr = 128, .cb = 128},
>     {.name = "grey",   .y = 128, .cr = 128, .cb = 128},
>     {.name = "yuv" ,   .y = 0, .cr = 0, .cb = 0}
> };

as I said, this is RGB unfriendly.

> typedef struct
> {
>     /* required size */
>     int  dim[2], off[2];
>     /* round to mod r */
>     int r;     
>     /* expand to aspect a */
>     double a;     

I'd prefer if you use AVRation for the aspect, at least for consistency 
with the rest of FFmpeg.

>     /* padding color */
>     int c;     
>     /* chroma subsampling  */
>     int sub[2];         
> } PadContext;

This comments could be doxygen compatible...

> static int init(AVFilterContext *ctx, const char *args, void *opaque)
> {
>     PadContext *pad = ctx->priv;
>     char s[255];
>     int n, m, r, b;
> 
>     /* default parameters */
>     pad->dim[0] = pad->dim[1] = -1;
>     pad->off[0] = pad->off[1] = -1;
>     pad->r =  1;
>     pad->a =  0.;
>     pad->c =  2;
> 
>     if( !args ){
>         av_log(NULL, AV_LOG_ERROR, "usage:  pad=w:h:x:y:r:a:c , where\n\t\
> w,h (width, height): -1 use current size [default]; -N enlarge by N pixels\n\t\
> x,y (offsets): -N center [default]\n\t\
> r: round to mod_r [1]\n\t\
> a: expand to fit aspect [don't]\n\t\
> c (color): blue|red|black|white|green|yellow|gray|grey|yuv/Y/CR/CB [black]\n");

I think that

av_log(NULL, AV_LOG_ERROR, "\
     usage:  pad=w:h:x:y:r:a:c , where\n
     w,h (width, height): -1 use current size [default]; -N enlarge by N 
pixels\n\
     x,y (offsets): -N center [default]\n\
     r: round to mod_r [1]\n\
     a: expand to fit aspect [don't]\n\
     c (color): blue|red|black|white|green|yellow|gray|grey|yuv/Y/CR/CB 
[black]\n");

Is more readable. Actually I'm thinking if the right solution wouldn't 
be to add a *AVFilter.show_help field, but it is unrelated with this 
review...

>     sscanf(args, "%d:%d:%d:%d:%d%n", 
>        &pad->dim[0], &pad->dim[1], &pad->off[0], &pad->off[1], &pad->r, &n);
> 
>     while(*(args += n) != '\0'){
>        sscanf(args, ":%[^:]%n", s, &n);
> 
>        if( sscanf(s,"yuv/%u/%u/%u", &m, &r, &b)  == 3 ){
>            colors[NUM_COLORS].y  = (unsigned char)m;
> 	   colors[NUM_COLORS].cr = (unsigned char)r;
> 	   colors[NUM_COLORS].cb = (unsigned char)b;

This is not thread safe. It is better to have a PadContext.color[3] 
field and make the colors[] array const.

[...]

> 
> static void draw_padding(AVFilterLink *link)
> {
>     PadContext *pad = link->dst->priv;
>     AVFilterPicRef *in  = link->cur_pic;
>     AVFilterPicRef *out = link->dst->outputs[0]->outpic;
>     int i, j, plane, vsub, hsub;

Well, it would be nice to support more pixel formats (including RGB and 
YUVA). You could maybe use some of Ryo's code (having two people 
contribute code for the same thing _have_ to have some advantage), but 
before please try to simplify the cases with pix_fmt_info.

-Vitor