[FFmpeg-devel] a64 encoder

Vitor Sessak vitor1001
Thu Jan 15 18:57:03 CET 2009


Hi!

Bitbreaker/METALVOTZE wrote:
> So after getting the hint on the mplayer-dev list to better implement my
> encoder things with ffmpeg to make it available to a broader range of
> applications, i did so. Encoding works fine so far for a bunch of modes.
> The muxer is yet kind of very simple by just adding two bytes header
> information (what is so far the loading address for files on a commodore
> 64) I shall add information about the used mode, fps and so on as well 
> to the header, but so far, the players i wrote for the c64 are still 
> kind of experimental, and forgo on that header information anyway.
> 
> Attached you'll find the patch for review, if you find it worth being 
> added to the repository, i'd be happy.

I'll give a first pre-review, but probably Michael will come with a 
full-blown review soon... Note that it when most people submit code to 
ffmpeg for the first time it take usually quite a few review cycles to 
get it accepted...

> --- libavcodec/a64colormixes.h	(Revision 0)
> +++ libavcodec/a64colormixes.h	(Revision 0)
> @@ -0,0 +1,113 @@
> +/*
> + * A64 Video Encoder - List of colormixes that are practicable/eye friendly
> + * Copyright (c) 2009 Tobias Bindhammer.
> + *
> + * 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
> + */
> +
> +/**
> + * @file a64colormixes.h
> + * A64 Video Encoder - List of colormixes that are practicable/eye friendly
> + */
> +
> +static int a64_color_mixes[][2] = {
> +    /* not noticable flicker */
> +    {0x9, 0x6},		
> +    {0x2, 0xb},
> +    {0x8, 0x4},
> +    {0xc, 0xe},
> +    {0xa, 0x5},

This table should be declared as const. It also fits in 8 bits, so 
declaring it with

static const uint8_t a64_color_mixes[][2] = {...}

save some binary size and makes better use of the cache. It also shows 
up in several other places...

> Index: libavcodec/a64colorlines.h
> ===================================================================
> --- libavcodec/a64colorlines.h	(Revision 0)
> +++ libavcodec/a64colorlines.h	(Revision 0)
> @@ -0,0 +1,206 @@
> +/*
> + * A64 Video Encoder - Colorlines for C64 Colorspace Transformation
> + * Copyright (c) 2009 Tobias Bindhammer.
> + *
> + * 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
> + */
> +
> +/**
> + * @file a64colorlines.h
> + * A64 Video Encoder - Colorlines for C64 Colorspace Transformation
> + */
> +
> +static unsigned char cl_solid_cyan[]={

static const uint8_t cl_solid_cyanp[] = {

> +/**
> + * @file a64ecmcharset.h
> + * A64 Video Encoder - Symbol Tables for ECM Hybrid Mode
> + */
> +
> +static uint32_t ecm_symbols[][8]={
> +{   /* 0x00 */

ditto. Also maybe it would be cleaner to merge all these .h files in a 
single a64_tables.h.

> Index: libavcodec/a64multi.c
> ===================================================================
> --- libavcodec/a64multi.c	(Revision 0)
> +++ libavcodec/a64multi.c	(Revision 0)
> @@ -0,0 +1,419 @@
> +/*
> + * A64 Video Encoder Multicolor Modes
> + * Copyright (c) 2009 Tobias Bindhammer.
> + *
> + * 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
> + */
> +
> +/**
> + * @file a64multi.c
> + * A64 Video Multicolor Modes
> + */
> +
> +#include "a64enc.h"
> +#include "a64multi.h"
> +#include "a64prepare.h"
> +
> +/* local methods */
> +
> +static void find_favourite(int, int, uint32_t *, uint32_t *);
> +static int calc_difference(int, int);
> +static int extract_multicol_charset(AVCodecContext *);
> +static int find_multicol_char(int, int);
> +static void render_multicol_charset(AVCodecContext *);

If you reorder your functions, there is no need for these forward 
declarations.

> +/* local vars */
> +uint8_t *multicol_charset;
 > +uint8_t *meta_charset;

Those are not local (no "static"), and also using global vars in general 
is not thread safe (unless it is written just once, in initialization). 
It is better to move them to A64Context.

> +uint8_t *multicol_bitmap;

This one is unused. Please get rid of all unused vars.

> +int *multicol_charmaps;
> +int *multicol_resolve_map;
> +int *multicol_colram_map;
> +int *multicol_charset_stats;

> +int a64multi_close_encoder(AVCodecContext *avctx) {
> +    A64Context *c = avctx->priv_data;
> +    /* clean up and convert remaing frames from buffer */
> +    /* XXX need flush function for codec to convert remaining frames on closing */
> +    /* yet those remaining %4 frames are dropped */
> +    if(c->multicol_frame_counter>0) {
> +        //c->multicol_cs_lifetime=c->multicol_frame_counter;
> +    }

Please follow our conventions on indentation: four spaces, no tabs, no 
trailing whitespaces.

> +		
> +    if(multicol_charmaps!=NULL) av_free(multicol_charmaps);
> +    if(multicol_bitmap!=NULL) av_free(multicol_bitmap);
> +    if(multicol_charset!=NULL) av_free(multicol_charset);
> +    if(multicol_charset_stats!=NULL) av_free(multicol_charset_stats);
> +    if(multicol_resolve_map!=NULL) av_free(multicol_resolve_map);
> +    if(multicol_colram_map!=NULL) av_free(multicol_colram_map);
> +    if(meta_charset!=NULL) av_free(meta_charset);
> +    return 0;
> +}
> +
> +int a64multi_init_encoder(AVCodecContext *avctx) {
> +    A64Context *c = avctx->priv_data;
> +
> +    c->multicol_cs_lifetime=4;

This is constant, so using a #define would simplify the code.

> +    c->multicol_frame_counter=0;

> +    c->multicol_dithersteps=MULTICOL_DITHERSTEPS;

Same thing here, could just use MULTICOL_DITHERSTEPS everywhere.

> +    if(avctx->codec->id==CODEC_ID_A64_MULTI5) c->multicol_5col=1;
> +    else c->multicol_5col=0;

c->multicol_5col = avctx->codec->id==CODEC_ID_A64_MULTI5;

> +
> +    if(multicol_charmaps==NULL) multicol_charmaps=(int *)av_malloc(0x400*c->multicol_cs_lifetime*sizeof(int));
> +    if(multicol_bitmap==NULL) multicol_bitmap=(uint8_t *)av_malloc(64000*c->multicol_cs_lifetime*sizeof(uint8_t));
> +    if(multicol_charset==NULL) multicol_charset=(uint8_t *)av_malloc(0x800*sizeof(uint8_t));
> +    if(multicol_charset_stats==NULL) multicol_charset_stats=(int *)av_malloc(1000*c->multicol_cs_lifetime*sizeof(int));
> +    if(multicol_resolve_map==NULL) multicol_resolve_map=(int *)av_malloc(1000*c->multicol_cs_lifetime*sizeof(int));
> +    if(multicol_colram_map==NULL) multicol_colram_map=(int *)av_malloc(1000*c->multicol_cs_lifetime*sizeof(int));
> +    if(meta_charset==NULL) meta_charset=(uint8_t *)av_malloc(32000*c->multicol_cs_lifetime*sizeof(uint8_t));

If c->multicol_cs_lifetime is just a constant, you could put all this in 
A64Context by

typedef struct A64Context {
(...)
     int multicol_charmaps[0x400*MULTICOL_CS_LIFETIME];
     uint8_t multicol_bitmap[64000*MULTICOL_CS_LIFETIME];
etc
(...)
}

this avoid all the need for alloc'ing/freeing.

> +
> +int a64multi_encode_frame(AVCodecContext *avctx, unsigned char *buf, int buf_size, AVFrame *p) {

static...

> +    A64Context *c = avctx->priv_data;
> +    int frame;
> +    int a;
> +    int framesize=0x800+(0x400+0x400*c->multicol_5col)*c->multicol_cs_lifetime;
> +	
> +    if(framesize>buf_size) {
> +        av_log(avctx, AV_LOG_ERROR, "buf size too small (need %d, got %d)\n", framesize, buf_size);
> +        return -1;
> +    }
> +
> +    /* start with clean arrays */
> +    if(c->multicol_frame_counter==0) {
> +        memset(multicol_charmaps,0,0x400*c->multicol_cs_lifetime*sizeof(int));
> +        memset(multicol_bitmap,0,64000*c->multicol_cs_lifetime*sizeof(uint8_t));
> +        memset(multicol_charset,0,0x800*sizeof(uint8_t));
> +        memset(multicol_resolve_map,0,1000*c->multicol_cs_lifetime*sizeof(int));
> +        memset(multicol_charset_stats,0,1000*c->multicol_cs_lifetime*sizeof(int));
> +        memset(meta_charset,0,32000*c->multicol_cs_lifetime*sizeof(uint8_t));
> +        memset(multicol_colram_map,0,1000*c->multicol_cs_lifetime*sizeof(int));
> +    }

Are you sure all those memsets are really needed? Also using something like

memset(multicol_charmaps,0,0x400*c->multicol_cs_lifetime*sizeof(*multicol_charmaps));

avoid introducing bugs if you ever change the type of multicol_charmaps...

> +/* own methods */
> +static void render_multicol_charset(AVCodecContext *avctx) {
> +    int charpos;
> +    int x,y;
> +    uint8_t row;
> +    int dither;
> +    int index;
> +    int minpix;
> +    int maxpix;
> +    int vals[]={3,2,1,0,3};

const uint8_t vals[]={3,2,1,0,3};

> +
> +static void find_favourite(int charpos, int lastchar, uint32_t* diff, uint32_t* best) {
> +    uint32_t a;
> +    uint32_t delta;
> +    uint32_t x;

Here you could use just int (it will use just as much mem, since it will 
  be mapped to registers anyway). Also, C99 warrants that int has at 
least 32 bits.

> +
> +static int extract_multicol_charset(AVCodecContext *avctx) {
> +    A64Context *c = avctx->priv_data;
> +    uint32_t lastchar;
> +    uint32_t bitmappos;
> +    uint32_t temp;
> +
> +    uint32_t a;
> +
> +    uint32_t killedchars;
> +    uint32_t diff[1000*c->multicol_cs_lifetime];
> +    uint32_t best[1000*c->multicol_cs_lifetime];
> +    uint32_t new_char;
> +    uint32_t old_char;
> +    uint32_t old_diff;
> +    uint32_t replacements[1000*c->multicol_cs_lifetime];

ditto for those.

> +static av_cold int a64_init_encoder(AVCodecContext *avctx)
> +{
> +    A64Context *c=avctx->priv_data;
> +    int a,b;
> +    int col1,col2;
> +

> +    if (avctx->pix_fmt != PIX_FMT_BGR24) {
> +        av_log(avctx, AV_LOG_ERROR, "video parameters incompatible with a64 codecs (used format: %d)\n",avctx->pix_fmt);
> +        return -1;
> +    }

This is not needed.

> +    for(b=0;b<16;b++) {
> +        for(a=0;a<16;a++) {
> +            c->flicker_map[b][a]=abs((a64_palette[b][0]*0.3+a64_palette[b][1]*0.59+a64_palette[b][2]*0.11)-(a64_palette[a][0]*0.3+a64_palette[a][1]*0.59+a64_palette[a][2]*0.11));
> +        }
> +    }

It would be better to not use floating point, to get the same results in 
different platforms (no rounding differences).

-Vitor




More information about the ffmpeg-devel mailing list