[Ffmpeg-devel] [RFC] [PATCH] FLC/FLX/DTA encoder

[Michael Niedermayer]

Hi

On Sun, Feb 18, 2007 at 02:19:11AM +0100, Alex Beregszaszi wrote:
> Hi,
> 
> attached is a huge patch adding three decoders: FLC, FLX and DTA.
> 
> FLC is the 8bit RLE based format, FLX supports 15bits, while DTA adds
> new compression methods to FLX.
> 
> The IFR (interframe reoder) code in palette.c reorders a frame in order
> to achieve only palette changes, thus possibly never emit any newly
> coded frames, but only palette differences. It is used in the encoder.
> 
> Known:
> * current patch has hard tabs and maybe identation issues (but any
> cosmetic recommendations are welcomed)
> 
> Question:
> * should it be splitted into flicenc.c ?

the more spliting the better, i would also prefer if the patch where split
a little reviewing these huge patches is not fun ...

VERY incomplete review below (ill do a real review tommorow ...)


[...]

> Index: libavutil/intreadwrite.h
> ===================================================================
> --- libavutil/intreadwrite.h	(revision 8012)
> +++ libavutil/intreadwrite.h	(working copy)
> @@ -27,39 +27,39 @@
>  
>  /* endian macros */
>  #define AV_RB8(x)  (((uint8_t*)(x))[0])
> -#define AV_WB8(p, d)  { ((uint8_t*)(p))[0] = (d); }
> +#define AV_WB8(p, d)  { ((uint8_t*)(p))[0] = (d)&0xff; }

unrelated same applies to the rest in this file

theres trailing whitespace and tabs in the patch ...


[...]
> +static int palette_sort(const void *a, const void *b) {
> +    int ia = *(int*)a, ib = *(int*)b;
> +    
> +    if (ia < ib) return -1;
> +    else if (ia == ib) return 0;
> +    return 1;

doesnt 
return *(int*)a - *(int*)b;
work? (yes it can overflow so i dunno ...)


> +}
> +
> +static int encode_palette_full(AVCodecContext *avctx, AVFrame *p, unsigned char *buf) {
> +    int pos = 10, i;
> +
> +    AV_WL16(buf+4, FLI_256_COLOR); // type
> +
> +    // emit full 256 palette
> +    AV_WL16(buf+6, 1); // one color packet
> +    AV_WL8(buf+8, 0); // from first entry
> +    AV_WL8(buf+9, 0); // 256 color changes
> +    for (i = 0; i < 256; i++) {
> +        buf[pos++] = p->data[1][i*4+2]&0xff;
> +        buf[pos++] = p->data[1][i*4+1]&0xff;
> +        buf[pos++] = p->data[1][i*4+0]&0xff;

palettes in AVFrame..data[1] are stored in native endian, i would definitly
be in favor of unifying the format with mplayer but thats a seperate issue


[...]
> +    for (i = 0; i < 256; i++)
> +        if (pal[i] != oldpal[i])
> +            changetable[i] = i+1; // slot 0 may change too
> +        else
> +            changetable[i] = 0;
> +    changetable[256] = 256;
> +
> +    // sort changes
> +    qsort(&changetable, 256, sizeof(int), palette_sort);
> +    
> +    // skip while zero
> +    for (i = 0; i < 256 && changetable[i] == 0; i++);
> +    lastpos = i;

messy ....

something like:

j=0;
for (i = 0; i < 256; i++)
    if (pal[i] != oldpal[i])
        changetable[j++] = i+1; // slot 0 may change too

looks nicer ...


[...]
> @@ -51,6 +59,10 @@
>      int video_stream_index;
>  } FlicDemuxContext;
>  
> +typedef struct FlicMuxContext {
> +    int framenumber, frame0, frame1;
> +} FlicMuxContext;
> +
>  static int flic_probe(AVProbeData *p)
>  {
>      int magic_number;
> @@ -212,6 +224,101 @@
>      return 0;
>  }
>  
> +static int flic_write_header(AVFormatContext *s)
> +{
> +    FlicMuxContext *flic = (FlicMuxContext *)s->priv_data;
> +
> +    ByteIOContext *pb = &s->pb;
> +    AVCodecContext *enc = NULL;
> +    int i;
> +    
> +    for (i = 0; i < s->nb_streams; i++) {
> +        AVCodecContext *stream = s->streams[i]->codec;
> +        if (stream->codec_type == CODEC_TYPE_VIDEO &&
> +            (stream->codec_id == CODEC_ID_FLIC ||
> +            stream->codec_id == CODEC_ID_FLIX ||
> +            stream->codec_id == CODEC_ID_FLIDTA)) {
> +            enc = stream;
> +        }
> +    }
> +    
> +    if (!enc)
> +        return 1; // error
> +
> +    // the container level fields will be patched in write_trailer
> +    put_buffer(pb, enc->extradata, enc->extradata_size);

iam protesting against this missuse of extradata
its the muxers job to write the header from width/height/... not to missuse
extradata, and yes the demuxer must be fixed too, if there are no volunteers
ill fix the demuxer and decoder
from a quick look i dont see why fl* would need to touch extradata at all
width/height/codec_tag/bits_per_sample seem enough


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070218/299c8318/attachment.pgp>