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

[Vitor Sessak]

Hi, and thanks for the patch!

Ryo Hirafuji wrote:
> Oh, I'm sorry.
> Here is a patch.
> It includes changes of allfilters.c and Makefile.

I'll give a first look:

> Index: vf_expand.c
> ===================================================================
> --- vf_expand.c	(revision 0)
> +++ vf_expand.c	(revision 0)
> @@ -0,0 +1,317 @@
> +/*
> +* video expand filter (replacement of pad syntax)

Bad aligned "*"

> +static char** split(char* str,int str_len,int* argc,char delim){
> +	if(!str || delim=='\0' || str_len < 0){
> +		return 0;
> +	}
> +	char** argv = av_malloc(sizeof(char*));

Declaration after code is forbidden in C99

> +	if(!argv){
> +		return 0;
> +	}
> +	int last = 0;
> +	int i;
> +	int arg_cnt = 0;
> +	for(i=0;i<str_len;i++){
> +		if(str[i] == delim){
> +			str[i] = '\0';
> +			argv[arg_cnt] = &str[last];
> +			arg_cnt++;
> +			last = i+1;
> +			argv = av_realloc(argv,sizeof(char*) * (arg_cnt+1));
> +		}
> +	}
> +	argv[arg_cnt] = &str[last];
> +	*argc = arg_cnt + 1;
> +	return argv;
> +}
> +
> +static int init(AVFilterContext *ctx, const char *args, void *opaque){
> +    Context *expand = ctx->priv;
> +
> +    /* default parameters */
> +    expand->x =  -1;
> +    expand->y =  -1;
> +    expand->ew = -1;
> +    expand->eh = -1;
> +	expand->osd = 0;
> +	expand->aspect = 0.0f;
> +	expand->round = 0;
> +
> +	expand->bpp = 0;
> +	expand->hsub = 0;
> +	expand->vsub = 0;
> +
> +	if(args){
> +		int argc;
> +		int length = strlen(args);
> +		char* copy_arg = av_malloc(length+1);
> +
> +		memcpy(copy_arg,args,length);
> +		copy_arg[length] = '\0';
> +
> +		char** argv = split(copy_arg,length,&argc,':');
> +		if(argc >= 1 && strlen(argv[0]) > 0){
> +			expand->ew = atoi(argv[0]);
> +		}
> +		if(argc >= 2 && strlen(argv[1]) > 0){
> +			expand->eh = atoi(argv[1]);
> +		}
> +		if(argc >= 3 && strlen(argv[2]) > 0){
> +			expand->x = atoi(argv[2]);
> +		}
> +		if(argc >= 4 && strlen(argv[3]) > 0){
> +			expand->y = atoi(argv[3]);
> +		}
> +
> +		if(argc >= 5 &&  strlen(argv[4]) > 0){ //checked, but not used in this version.
> +			if(!strncmp(argv[4],"true",4)){
> +				expand->osd = 1;
> +			}else{
> +				expand->osd = atoi(argv[4]);
> +			}
> +		}
> +
> +		if(argc >= 6 &&  strlen(argv[5]) > 0){
> +			int a,b;
> +			double eval;
> +
> +			sscanf(argv[5],"%d/%d",&a,&b);
> +			if(a != 0 && b != 0 && (a*b) > 0){
> +				expand->aspect = ((double)a)/b;
> +			}else if((eval = atof(argv[5])) >= 0.0f){
> +				expand->aspect = eval;

See av_parse_video_frame_rate() and AVRational.

> +			}else{
> +			}
> +		}
> +
> +		if(argc >= 7 &&  strlen(argv[6]) > 0){
> +			expand->round = atoi(argv[6]);
> +		}

I agree with the others that this could be much simplified using
sscanf() or similars.

> +		av_log(0, AV_LOG_INFO, "Expand: %dx%d , (%d,%d) , osd: %d, aspect: %lf, round: %d\n",
> +	    expand->ew, expand->eh, expand->x, expand->y, expand->osd, expand->aspect, expand->round);

When using av_log(), always give a context (in this case it could be
ctx), instead of NULL.

> +static int config_input(AVFilterLink *link){
> +    Context *expand = link->dst->priv;
> +	int width = link->w;
> +	int height = link->h;
> +
> +	if (expand->ew == -1){
> +		expand->ew=width;
> +	} else if (expand->ew < -1){
> +		expand->ew=width - expand->ew;
> +	} else if (expand->ew < width){
> +		expand->ew=width;
> +	}
> +
> +	if (expand->eh == -1){
> +		expand->eh=height;
> +	} else if (expand->eh < -1){
> +		expand->eh=height - expand->eh;
> +	} else if (expand->eh < height){
> +		expand->eh=height;
> +	}

It might be a matter of taste, but I think it would like much better
without the braces.

> +	av_log(0, AV_LOG_INFO, "x:%d y:%d\n",expand->x,expand->y);

context missing

> +static void start_frame(AVFilterLink *link, AVFilterPicRef *picref){
> +    Context *expand = link->dst->priv;
> +	AVFilterLink *out = link->dst->outputs[0];
> +	int i;
> +	int is_yuv;
> +	out->outpic      = avfilter_get_video_buffer(out, AV_PERM_WRITE);
> +    out->outpic->pts = picref->pts;
> +	is_yuv = out->outpic->data[1] != 0;
> +	if(is_yuv){

I suppose you mean is_planar? Also you should see pix_fmt_info

> +       	char* mem = out->outpic->data[0];
> +		for(i=0;i<out->outpic->h;i++){
> +			memset(mem,16,out->outpic->w);
> +			mem += out->outpic->linesize[0];
> +		}

Aren't you drawing some pixels to overwrite them latter? It's faster to
paint just the borders...

> +		for(i=1;i<3;i++) {

What about YUVA (YUV+alpha)?

> +	        if(out->outpic->data[i]) {
> +	        	int j;
> +	        	int h = out->outpic->h;
> +	        	mem = out->outpic->data[i];
> +	        	for(j=0;j<h;j+=(1<<expand->vsub)){
> +	        		memset(mem,128,out->outpic->w >> expand->hsub);
> +	        		mem += out->outpic->linesize[i];
> +				}
> +
> +	        }
> +		}

Maybe you can merge the loops for luma and chroma...

> +static void draw_slice(AVFilterLink *link, int y, int h){
> +    Context *expand = link->dst->priv;
> +	AVFilterPicRef *pic = link->dst->outputs[0]->outpic;
> +	AVFilterPicRef *cur_pic = link->cur_pic;

Maybe inpic and outpic are better names

> +	int i;
> +	unsigned int out_linesize = pic->linesize[0];
> +	unsigned char* out_buff = pic->data[0];
> +	unsigned int in_linesize = cur_pic->linesize[0];
> +	const unsigned char* in_buff = cur_pic->data[0];
> +	int copy_length = cur_pic->w * expand->bpp;

Maybe the code would be more readable to use pic->linesize[0] directly?
Maybe some of the others vars could be removed too (if it make the code
more clear, of course).

> +	for(i=0;i<h;i++){
> +		memcpy(out_buff,in_buff,cur_pic->w * expand->bpp);
> +		out_buff += out_linesize;
> +		in_buff += in_linesize;
> +	}
> +	for(i=1;i<4;i++) {

I think it'll be more simple if those two loops are merged

> +AVFilter avfilter_vf_expand = {
> +    .name      = "expand",
> +    .priv_size = sizeof(Context),
> +
> +    .init      = init,
> +
> +    .inputs    = (AVFilterPad[]) {{ .name            = "default",
> +                                    .type            = CODEC_TYPE_VIDEO,
> +                                    .start_frame     = start_frame,
> +                                    .draw_slice      = draw_slice,
> +                                    .end_frame       = end_frame,
> +                                    .config_props    = config_input, },
> +                                  { .name = NULL}},
> +    .outputs   = (AVFilterPad[]) {{ .name            = "default",
> +                                    .type            = CODEC_TYPE_VIDEO,
> +                                    .config_props    = config_output, },
> +                                  { .name = NULL}},
> +};

You don't set *AVFilter.formats. That means that your filter will accept
any format as input, but it don't handle PAL8 or 1 bit per pixel
blackwhite or other esoteric formats.