[FFmpeg-devel] [PATCH][RFC] Indeo3 replacement

[Vitor Sessak]

Maxim wrote:
> Hi crews,
> 
> as already volunteered I'd like to maintain indeo3 decoder in the
> future. Unfortunately the ffmpeg's decoder is unmaintable (Mike, sorry!)
> because nobody understands how it works. Therefore I want to submit a
> patch proposing a new source for this algorithm. Below its advantages in
> short:
> 
> - deobfuscated algorithm
> - heavily commented source
> - decoding tables will be generated dynamically making "indeo3data.h"
> tiny compared to the existing one!
> - one huge code blob was splitted into several functions
> 
> Disadvantages:
> 
> - it was written fast therefore it contains several simplifications can
> be programmed safer
> - may be further splitted
> - it was not tested on PPC yet

I think Mans once said he has a PPC machine people could use for FFmpeg 
development.

> There is already a documentation for this algorithm here: http://wiki.multimedia.cx/index.php?title=Indeo_3
> 
> Attached files are the sources itself because the new sources are VERY different from the existing ones!
> 
> Plz be merciful to me and help me out to make this stuff good-looking!
> Waiting for reviews...

A few comments:

> //@{
> //! vq table selector codes
> #define DELTA_DYAD      0
> #define DELTA_QUAD      1
> #define RLE_ESC_F9      2
> #define RLE_ESC_FA      3
> #define RLE_ESC_FB      4
> #define RLE_ESC_FC      5
> #define RLE_ESC_FD      6
> #define RLE_ESC_FE      7
> #define RLE_ESC_FF      8
> #define RLE_FORBIDDEN   9
> //@}

Does this group Doxy formatting works for you?

> 
> /**
>  *  Build decoder delta tables dynamically.
>  */
> static av_cold void build_delta_tables(void)
> {
>     int             n, i, j, quads;
>     int32_t         a, b, corr;
>     int32_t         *delta_t, *quad_t, *delta_lo, *delta_hi;
>     const vqtEntry  *rom_t;
>     const int8_t    *delta_rom;
>     int8_t          *sel_t;
> 
>     for (n = 0; n < 24; n++) {
>         rom_t     = &vq_delta_rom[n];
>         delta_rom = rom_t->delta_tab;
> 
>         delta_t  = &delta_tabs   [n][0];
>         sel_t    = &selector_tabs[n][0];
>         delta_lo = &delta_m10_lo [n][0];
>         delta_hi = &delta_m10_hi [n][0];
> 
>         /* set all code selectors = forbidden */
>         memset(sel_t, RLE_FORBIDDEN, 249);
> 
>         /* initialize selectors for RLE escape codes (0xF8...0xFF) */
>         sel_t[249] = RLE_ESC_F9;
>         sel_t[250] = RLE_ESC_FA;
>         sel_t[251] = RLE_ESC_FB;
>         sel_t[252] = RLE_ESC_FC;
>         sel_t[253] = RLE_ESC_FD;
>         sel_t[254] = RLE_ESC_FE;
>         sel_t[255] = RLE_ESC_FF;
> 
>         for (i = 0; i < rom_t->num_dyads; i++) {
>             /* get two vq deltas from the ROM table */
>             a = delta_rom[i*2];
>             b = delta_rom[i*2+1];
>             /* concatenate two vq deltas to form a two-pixel corrector (dyad) */
> #ifdef WORDS_BIGENDIAN
>             corr = (a << 8) + b;
> #else
>             corr = (b << 8) + a;
> #endif
>             delta_t[i] = corr;

Are you sure that this is correct? Later on, you do

>         prim_delta   = &delta_tabs   [prim_indx]  [0];

and

>                             delta_tab = (line & 1) ? prim_delta : second_delta;

and

>                                     src16[0] = ref16[0] + delta_tab[*data_ptr++];

Wouldn't this result depends on endianness?

> 
> /**
>  *  Decode a vector-quantized cell.
>  *  It consists of several routines, each of which handles one or more "modes"
>  *  with which a cell can be encoded.

[...]

>  switch (mode) {
>         case 0: /*------------------ MODES 0 & 1 (4x4 block processing) --------------------*/
>         case 1:
>             skip_flag = 0;

[... a lot of code ...]

> 
>         case 3: /*------------------ MODES 3 & 4 (4x8 block processing) --------------------*/
>         case 4:

Are you sure the code is not cleaner if ones puts the code of each case 
statement in a function?

-Vitor