[FFmpeg-devel] [PATCH] libavcodec/imgconvert.c new function: avcodec_get_pix_fmt_info()

Vitor Sessak vitor1001
Mon Mar 23 22:38:45 CET 2009


J?rgen Meissner wrote:
> patch comment (trunk)
> 
> imgconvert.c gets a new function (with prototype in libavcodec/avcodec.h)
> 
> const PixFmtInfo *avcodec_get_pix_fmt_info(enum PixelFormat pix_fmt)
> {
>    if (pix_fmt < 0 || pix_fmt >= PIX_FMT_NB)
>        return NULL;
>    else
>        return &pix_fmt_info[pix_fmt];
> }
> 
> 
> to make it sincerly public

There is an effort to do exactly that, and Stefano is working on this. 
Follow the thread starting at 
http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2009-February/062567.html .

To resume: PixFmtInfo is not clean enough to be part of the public API 
(and thus set on stone), so Michael would not accept it unless it is 
refactored. Stefano is working on it, see libavcodec/pixdesc.{c,h}.

> removed some defines and typedef for PixFmtInfo, they moved from 
> libavcodec/imgconvert.c
> to libavutil/pixfmt.h
> 
> 
> first usage in libavfilter/vf_logo.c filter
> (included here to make it clear, why it's usefull,
> will be checked in to soc/libavfilter after beeing
> based an a ffmpeg revision containing the new function)

I'll give a first round of review...

> Index: libavfilter/vf_logo.c
> ===================================================================
> --- libavfilter/vf_logo.c	(revision 0)
> +++ libavfilter/vf_logo.c	(revision 0)
> @@ -0,0 +1,780 @@
> +#define VF_LOGO_VERSION "0.9.3 22.3.2009"
> +/**
> + * libavfilter/vf_logo.c
> + * filter to overlay (with or without alpha) logo on top of video
> + * Copyright (c) 2009 Juergen Meissner (using parts of previous code)
> + * Copyright (c) 2008 Victor Paesa     (libavfilter/vsrc_movie.c)
> + * Copyright (c) 2007 Bobby Bingham    (libavfilter/vf_overlay.c)
> + * Copyright (c) 2007 Juergen Meissner (vhook/overlay.c)
> + *
> + * 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
> + */
> +
> +
> +/**
> + *
> + * example of using libavfilter/vf_logo:
> + *
> + * ffmpeg -i inputvideofile -vfilters logo=10:20:logofile.png -y outputvideofile
> + *
> + * image of logofile.png is overlayed onto every frame of inputvideofile 
> + * at offset x=10 y=20 giving outputvideofile
> + *
> + * x <INT>
> + *
> + *   Defines a logo (left border) offset from the left side of the video.
> + *   A negative value offsets (logo right border) from
> + *   the right side of the video.
> + *
> + * y <INT>
> + *
> + *   Defines a logo (top border) offset from the top of the video.
> + *   A negative value offsets (logo bottom border) from
> + *   the bottom of the video.
> + *
> + * if logofile has no alpha-path You can prefix another 3 fields R,G,B
> + * to select a RGB-color to be the transparent one, or You can code
> + * 999:999:999 to force overlaying all pixels (even if no alpha-path)
> + *
> + * ffmpeg -i inputvideofile -vfilters logo=0:0:0:10:20:logofile.png -y outputvideofile
> + *   black is the color to be understood as transparent
> + *
> + * ffmpeg -i inputvideofile -vfilters logo=999:999:999:10:20:logofile.png -y outputvideofile
> + *   overlay with all the pixels, alpha will be adjusted to 255
> + *
> + *
> + */

This would be even better at libavcodec/doc/vfilters.texi

> +#include <stdio.h>
> +#include "avfilter.h"
> +#include "libavcodec/avcodec.h"
> +#include "libavcodec/imgconvert.h"
> +#include "libavformat/avformat.h"
> +#include "libavutil/avutil.h"
> +#include "libswscale/swscale.h"
> +#include "libswscale/swscale_internal.h"

That's a lot of includes, are they all needed?

> +/*
> + * function declarations
> + */
> +static int  load_logo_create_frames(AVFilterContext *ctx);

It would be cleaner to reorder the functions not to need the forward 
declaration.


> +static av_cold int init(AVFilterContext *ctx, const char *args, void *opaque)
> +{
> +    LogoContext     *logo = ctx->priv;
> +    int             num_fields;
> +
> +    av_log(NULL, AV_LOG_DEBUG, " vf_logo: version %s\n", VF_LOGO_VERSION);

While it is not wrong per se, the filter is supposed to be part of the 
FFmpeg source, so there is no need to print the version...

> +    logo->ctx =     ctx;  // remember pointer to AVFilterContext in LogoContext
> +
> +    av_log(NULL, AV_LOG_DEBUG, " vf_logo: vf_logo context is at %p\n",logo);

We forbit using av_log() with NULL as first argument. You can just use 
av_log(ctx, AV_LOG_DEBUG, ...);

> +    logo->sws                            = NULL;
> +    logo->plogo_frame                    = NULL;
> +    logo->plogo_frame_rgba32             = NULL;
> +    logo->plogo_frame_video_format       = NULL;
> +    logo->buffer_logo_frame              = NULL;
> +    logo->buffer_logo_frame_rgba32       = NULL;
> +    logo->buffer_logo_frame_video_format = NULL;
> +    logo->pRuler_0                       = NULL;
> +    logo->pRuler_1_2                     = NULL;

This is not needed, the context is alloc'ed with av_mallocz().

> +    logo->alpha_R = -1;   // you can pick a color to be the transparent one
> +    logo->alpha_G = -1;   // or 999,999,999 if you don't want any 
> +    logo->alpha_B = -1;   // transparency

I find it more readable

logo->alpha_R = 0xFF;

> +    if(!args || strlen(args) > 1024) {
> +        av_log(NULL, AV_LOG_ERROR, " vf_logo: Invalid arguments!\n");
> +        return -1;
> +    }

If you put ctx instead of NULL, there is no need to put "vf_logo:"...

> +    num_fields = sscanf(args, "%d:%d:%d:%d:%d:%512[^:]",
> +                        &logo->alpha_R, &logo->alpha_G, &logo->alpha_B,
> +                        &logo->x,       &logo->y,       logo->file_name);
> +    if (num_fields == 6) {
> +      av_log(NULL, AV_LOG_INFO,

Indentation differs from our standard...

> +        " vf_logo: RGB=(%d,%d,%d) x=%d y=%d file=%s\n",
> +        logo->alpha_R, logo->alpha_G, logo->alpha_B,
> +        logo->x,       logo->y,       logo->file_name);
> +    }
> +    else {
> +    num_fields = sscanf(args, "%d:%d:%512[^:]",

same here

> +      &logo->x, &logo->y, logo->file_name);
> +    if (num_fields == 3) {
> +      av_log(NULL, AV_LOG_INFO,
> +        " vf_logo: x=%d y=%d file=%s\n",
> +        logo->x, logo->y, logo->file_name);
> +      av_log(NULL, AV_LOG_DEBUG,
> +        " vf_logo: RGB=(%d,%d,%d) x=%d y=%d file=%s\n",
> +        logo->alpha_R, logo->alpha_G, logo->alpha_B,
> +        logo->x,       logo->y,       logo->file_name);
> +    }
> +    else {
> +      av_log(NULL, AV_LOG_ERROR, " vf_logo: expected 3 or 6 arguments\n\t\t\tlogo={R:G:B:}x:y:filename\n\t\t\toptional R,G,B selects a color to be the transparent one\n\t\t\t999,999,999 forces overlay of all pixels\n\t\t\tbut wrong args are given: '%s'\n", args);
> +      return -1;
> +    }
> +    }
> +
> +    if (!(logo->sws = sws_getContext(16,16,0,16,16,0,SWS_BICUBIC,NULL,NULL,NULL))) {
> +      av_log(NULL, AV_LOG_ERROR, " vf_logo: cannot get SwsContext for swscale\n");
> +      return -1;
> +    }
> +    // load logo image and create rgba32 and video_format frames of logo
> +    return load_logo_create_frames(ctx);
> +}

> +static av_cold void uninit(AVFilterContext *ctx)
> +{
> +    LogoContext     *logo = ctx->priv;
> +
> +    av_log(NULL, AV_LOG_DEBUG, " vf_logo: uninit\n");
> +
> +    if (logo->sws != NULL) sws_freeContext(logo->sws);
> +    av_freep(&logo->plogo_frame);
> +    av_freep(&logo->plogo_frame_rgba32);
> +    av_freep(&logo->plogo_frame_video_format);
> +    av_freep(&logo->buffer_logo_frame);
> +    av_freep(&logo->buffer_logo_frame_rgba32);
> +    av_freep(&logo->buffer_logo_frame_video_format);
> +    av_freep(&logo->pRuler_0);
> +    av_freep(&logo->pRuler_1_2);

There is no point in setting the pointers to NULL, use just av_free().
> +
> +static int config_input_main(AVFilterLink *link)
> +{
> + LogoContext     *logo = link->dst->priv;
> + AVFilterContext *ctx;
> + int             i, j, inc_i, inc_j, numBytes, r_0_numBytes,r_1_2_numBytes;
> + RGBA            *pRGBA;
> + uint8_t         *pRGBA_sol;
> + uint8_t         *pRuler;
> +
> + ctx = logo->ctx; // get AVFilterContext pointer from LogoContext
> +
> + av_log(NULL, AV_LOG_DEBUG, " vf_logo: config_input_main\n");
> +
> + switch(link->format) {
> + case PIX_FMT_RGB32:
> + case PIX_FMT_BGR32:
> +      logo->bpp = 4;
> +      break;
> + case PIX_FMT_RGB24:
> + case PIX_FMT_BGR24:
> +      logo->bpp = 3;
> +      break;
> + case PIX_FMT_RGB565:
> + case PIX_FMT_RGB555:
> + case PIX_FMT_BGR565:
> + case PIX_FMT_BGR555:
> + case PIX_FMT_GRAY16BE:
> + case PIX_FMT_GRAY16LE:
> +      logo->bpp = 2;
> +      break;
> + default:
> +      logo->bpp = 1;
> + }

Isn't the whole point of making PixFmtInfo public not to need making 
those type of switches?

> + avcodec_get_chroma_sub_sample(link->format, &logo->hsub, &logo->vsub);
> +    
> + logo->video_w      = link->w;
> + logo->video_h      = link->h;
> + logo->video_format = link->format;
> + av_log(NULL, AV_LOG_DEBUG, " vf_logo: video size is %dx%d\tpix-fmt:%s\n", logo->video_w, logo->video_h, sws_format_name(logo->video_format));
> +
> +  if (link->format == logo->format)
> +  {
> +    logo->plogo_frame_video_format       = logo->plogo_frame;
> +    logo->buffer_logo_frame_video_format = logo->buffer_logo_frame;
> +  }
> +  else if (link->format != PIX_FMT_RGBA)
> +  {
> +    // transform it with swscaler from PIX_FMT_RGBA to link->format
> +    av_log(NULL, AV_LOG_DEBUG, " vf_logo: transform logo image from RGBA to pix_fmt=%s\n", sws_format_name(link->format));

Why? In most cases, the picture is in YUV format (for ex. a jpg) and the 
video too. It is slow and lossy to convert everything to RGB and back...

I'll give a second look after you fix those things, but be sure to test 
you filter adding

"format=rgb32", "format=gray", format=yuv444", "format=bgr4" before your 
filter like

"format=rgb32,logo=..."

-Vitor



More information about the ffmpeg-devel mailing list