[FFmpeg-devel] a64 encoder

Michael Niedermayer michaelni
Fri Jan 16 14:00:26 CET 2009


On Thu, Jan 15, 2009 at 03:02:31PM +0100, 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.

If you are in control of the format why dont you use an existing
muxer/demuxer?
And no iam not suggesting you implement a full avi demuxer on teh c64 but
just one that can handle the subset of avi used in the muxer


The patch is also very big, spliting it per encoder would definitly be a
good idea

also id like to hear some use case for each encoder, that is some situation
in which it is better than all others.


[...]
> Index: libavcodec/a64colormixes.h
> ===================================================================
> --- libavcodec/a64colormixes.h	(Revision 0)
> +++ libavcodec/a64colormixes.h	(Revision 0)
[...]
> +/**
> + * @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},
> +    {0xf, 0x3},
> +    {0x7, 0xd},

you dont need int, uint8_t is enough


[...]

> +#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 *);
> +
> +/* local vars */
> +uint8_t *multicol_charset;
> +uint8_t *meta_charset;
> +uint8_t *multicol_bitmap;
> +int *multicol_charmaps;
> +int *multicol_resolve_map;
> +int *multicol_colram_map;
> +int *multicol_charset_stats;

as alraedy mentioned by others these things are not thread safe.



> +
> +int a64multi_close_encoder(AVCodecContext *avctx) {

non static functions must have a
ff_ or av_ prefix to avoid possible namespace clashes with other libs


> +    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;
> +    }
> +		

trailing whitespace is forbiden in ffmpeg svn


> +    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);

av_free(NULL) is safe thus no need to check


> +    return 0;
> +}
> +
> +int a64multi_init_encoder(AVCodecContext *avctx) {
> +    A64Context *c = avctx->priv_data;
> +
> +    c->multicol_cs_lifetime=4;
> +    c->multicol_frame_counter=0;
> +    c->multicol_dithersteps=MULTICOL_DITHERSTEPS;

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

could be verticall aligned like
if(avctx->codec->id==CODEC_ID_A64_MULTI5) c->multicol_5col=1;
else                                      c->multicol_5col=0;

which looks prettier
though it can even be written as

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));

the casts are useless and it would be more readable with a little bit of
vertical aligment


[...]
> +    /* fill up charset with framedata until lifetime exeeded */
> +    if(c->multicol_frame_counter<c->multicol_cs_lifetime) {
> +    	if(c->multicol_5col==1) a64_bgr_to_meta(avctx, p, meta_charset+32000*c->multicol_frame_counter,c->multicol_dithersteps*4);
> +    	else a64_bgr_to_meta(avctx, p, meta_charset+32000*c->multicol_frame_counter,c->multicol_dithersteps*3);
> +        c->multicol_frame_counter++;
> +    }

tabs are also forbidden in ffmpeg svn

[...]
> +static const uint32_t dpattern[8][4][4]= {
> +{
> +        {0,0,0,0},
> +        {0,0,0,0},
> +        {0,0,0,0},
> +        {0,0,0,0}
> +},
> +{
> +        {1,0,0,0},
> +        {0,0,0,0},
> +        {0,0,1,0},
> +        {0,0,0,0}
> +},

its realls just 1 bit each so doesnt need uint32_t


[...]

> +#ifdef DEBUG_A64
> +
> +#include <time.h>
> +#include <sys/timeb.h>
> +#include <sys/types.h>
> +#include <sys/time.h>
> +
> +struct timeb start;
> +struct timeb end;
> +
> +void begin_measure() {
> +    ftime(&start);
> +    return;
> +}
> +
> +void end_measure() {
> +    double time;
> +    double timediff;
> +    ftime(&end);
> +    timediff=(end.time-start.time)*1000.0+(double)end.millitm-(double)start.millitm;
> +
> +    time=timediff/1000.0;
> +    av_log(NULL,AV_LOG_ERROR,"calc.time: %.4fs\n", time);
> +    return;
> +}
> +#endif

see START/STOP_TIMER


> +
> +static av_cold int a64_close_encoder(AVCodecContext *avctx)
> +{
> +    switch(avctx->codec->id) {
> +        case CODEC_ID_A64_MULTI:
> +        case CODEC_ID_A64_MULTI5:
> +            a64multi_close_encoder(avctx);
> +        break;
> +
> +        case CODEC_ID_A64_ECM_CHAR:
> +//            a64ecmc_close_encoder(avctx);
> +        break;
> +
> +        case CODEC_ID_A64_ECM_HYBRID:
> +            a64ecmh_close_encoder(avctx);
> +        break;
> +
> +        case CODEC_ID_A64_ECM_PETSCII:
> +        case CODEC_ID_A64_PETSCII:
> +            a64petscii_close_encoder(avctx);
> +        break;
> +    }
> +    return 0;
> +}

these functions could be used directly i think

[...]

> +    c->num_mixes=(sizeof(a64_color_mixes)/sizeof(a64_color_mixes[0]));

FF_ARRAY_ELEMS


[...]
> +/**
> + * @file a64palette.h
> + * A64 Video Encoder - C64 Palette in RGB
> + */
> +
> +static int a64_palette[16][3] = {
> +    {0x00,0x00,0x00},
> +    {0xff,0xff,0xff},
> +    {0x85,0x1f,0x02},
> +    {0x65,0xcd,0xa8},
> +    {0xa7,0x3b,0x9f},
> +    {0x4d,0xab,0x19},
> +    {0x1a,0x0c,0x92},
> +    {0xeb,0xe3,0x53},
> +    {0xa9,0x4b,0x02},
> +    {0x44,0x1e,0x00},
> +    {0xd2,0x80,0x74},
> +    {0x46,0x46,0x46},
> +    {0x8b,0x8b,0x8b},
> +    {0x8e,0xf6,0x8e},
> +    {0x4d,0x91,0xd1},
> +    {0xba,0xba,0xba}
> +};

should be const


[...]
> +static unsigned int dither_patterns[9][4][4]={
> +{
> +    {0,0,0,0},
> +    {0,0,0,0},
> +    {0,0,0,0},
> +    {0,0,0,0}
> +},
> +{
> +    {1,0,0,0},
> +    {0,0,0,0},
> +    {0,0,1,0},
> +    {0,0,0,0}
> +},
> +{
> +    {1,0,0,0},
> +    {0,0,1,0},
> +    {0,1,0,0},
> +    {0,0,0,1}
> +},
> +{
> +    {1,0,0,0},
> +    {0,1,0,1},
> +    {0,0,1,0},
> +    {0,1,0,1}
> +},
> +{
> +    {1,0,1,0},
> +    {0,1,0,1},
> +    {1,0,1,0},
> +    {0,1,0,1}
> +},
> +{
> +    {1,1,1,0},
> +    {0,1,0,1},
> +    {1,0,1,1},
> +    {0,1,0,1}
> +},
> +{
> +    {0,1,1,1},
> +    {1,1,0,1},
> +    {1,0,1,1},
> +    {1,1,1,0}
> +},
> +{
> +    {0,1,1,1},
> +    {1,1,1,1},
> +    {1,1,0,1},
> +    {1,1,1,1}
> +},
> +{
> +    {1,1,1,1},
> +    {1,1,1,1},
> +    {1,1,1,1},
> +    {1,1,1,1}
> +},
> +};

you dont need int for 1 bit


[...]
> +uint8_t hires_charset_std[]={
> +/* original charset */
> +/* char 00 - 0 */
> +0, 0, 2, 2, 2, 2, 0, 0,  //  ####  
> +0, 2, 2, 0, 0, 2, 2, 0,  // ##  ## 
> +0, 2, 2, 0, 2, 2, 2, 0,  // ## ### 
> +0, 2, 2, 0, 2, 2, 2, 0,  // ## ### 
> +0, 2, 2, 0, 0, 0, 0, 0,  // ##     
> +0, 2, 2, 0, 0, 0, 2, 0,  // ##   # 
> +0, 0, 2, 2, 2, 2, 0, 0,  //  ####  
> +0, 0, 0, 0, 0, 0, 0, 0,  //        

I assume the charset of the c64 cannot be changed? Because some of these
are likely not good choices for encoding videos
and its missing a const


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090116/65db1779/attachment.pgp>



More information about the ffmpeg-devel mailing list