[FFmpeg-soc] [PATCH] new avfilter vf_logo.c to overlay a (alpha-pathed) logo onto a video stream

[Vitor Sessak]

Jürgen Meissner wrote:
> vf_logo.c implements avfilters "logo" and "logo_without_alpha"

Great! This is one of the feature most requested by users!

> logo=10:20:logo.jpg overlays the video at x=10 y=20 with the logo (is 
> respecting the alpha-path of the logo)
> logo=-1:1:logo.png   overlays the video at 1 pixel from the right and 1 
> pixel from the top border
> logo_without_alpha=10:20:logo.gif overlays the video at 10,20 for all 
> pixels of the logo, doesn't look for an alpha-path in the logo
> logo=0:0:0:10:20:logo.gif overlays video at 10,20 and assumes black 
> RGB=(0,0,0) is the transparent color in the logo
> 
> vf_logo.c is relying on the new pixdesc.c rather than the old pix-fmt.c 
> and therefor needs pixdesc.o to be included into the libavcodec.a archive.

I'll add a few comments to those Setefano gave.

> 03_libavcodec_Makefile.diff adds the pixdesc.o to the OBS list in the 
> libavcodec/Makefile

I leave this to Stefano/Michael...

> +/**
> + * RGBA pixel.
> + */
> +typedef struct {
> +    uint8_t R;                  ///< Red.
> +    uint8_t G;                  ///< Green.
> +    uint8_t B;                  ///< Blue.
> +    uint8_t A;                  ///< Alpha.
> +} RGBA;
> +

We usually do not use those type of structs, but just uint8_t[4].

> +static const char *pixdesc_name(int pix_fmt)
> +{
> +    return av_pix_fmt_descriptors[pix_fmt].name;
> +}
> +

I think you can just inline this function where it is needed.

> +    if (logo->pCodecCtx->pix_fmt != PIX_FMT_RGBA) {
> +        // transform it with swscaler to PIX_FMT_RGBA 

It seems that you always convert the logo to RGBA. This is a bad idea 
because if both the logo and the movie is in YUV format, the logo will 
be converted YUV->RGB->YUV, that would degrade the logo quality.

> +        // Allocate an AVFrame structure
> +        logo->plogo_frame_rgba32 = avcodec_alloc_frame();

What this comment says is already obvious from the line it describes, 
making it useless and distracting (that probably happens elsewhere in 
the code too).

> +static void uninit(AVFilterContext * ctx)
> +{
> +
> +    LogoContext *logo;
> +
> +    logo = ctx->priv;
> +
> +    if (logo->sws != NULL)
> +        sws_freeContext(logo->sws);

I think that if you do colorspace conversion just once, there is no need 
to having logo->sws in the context (and that would simplify 
allocation/freeing).

> +static AVFilterFormats *make_format_list(LogoContext * logo)
> +{
> +    AVFilterFormats *ret;
> +    int i;
> +
> +    ret = av_mallocz(sizeof(AVFilterFormats));
> +    ret->formats = av_malloc(sizeof(int) * PIX_FMT_NB);
> +
> +    for (i = 0; i < PIX_FMT_NB; i++) {
> +        switch (i) {
> +            /* don't support these */
> +        case PIX_FMT_YUYV422:
> +        case PIX_FMT_MONOBLACK:
> +        case PIX_FMT_UYVY422:
> +            break;
> +            /* support everything else (if named) */
> +        default:
> +            if (av_pix_fmt_descriptors[i].name)
> +                ret->formats[ret->format_count++] = i;
> +        }
> +    }
> +    return ret;

While I agree with Stefano that it would be hard to do less ugly, what 
bothers me about this code is that it is fragile. It is very likely it 
will break when new formats are added....

Thanks for your contribution!
-Vitor