[FFmpeg-devel] Indeo3 replacement, part 2

[Diego Biurrun]

On Tue, Sep 22, 2009 at 12:09:23AM +0200, Maxim wrote:
> 
> Waiting for further reviews (please cosmetics in a last step!)

Bah, I had a moment, I started ;)

Say, what happened to Indeo 4 and 5?

>             a = delta_rom[i*2];
>             b = delta_rom[i*2+1];

I"d use some apaces around operators here.

> #ifdef HAVE_BIGENDIAN
>             corr = (a << 8) + b;
> #else
>             corr = (b << 8) + a;
> #endif
> #ifdef HAVE_BIGENDIAN
>             corr = (a << 8) + a;
>             corr = (corr << 8) + b;
>             corr = (corr << 8) + b;
> #else
>             corr = (b << 8) + b;
>             corr = (corr << 8) + a;
>             corr = (corr << 8) + a;
> #endif

I think you could use a macro for this.  This way you could also avoid
littering the code with #ifs.

This probably applies to all other such cases.

> /**
>  *  Requant a 8 pixel line by duplicating each even pixel as follows:
>  *  ABCDEFGH -> AACCEEGG
>  */
> #ifdef HAVE_BIGENDIAN
>     #define REQUANT_64(hi, lo) {\
>         (hi)  = (hi) & 0xFF00FF00;\
>         (hi) |= (hi) >> 8;\
>         (lo)  = (lo) & 0xFF00FF00;\
>         (lo) |= (lo) >> 8;\
>     }
> #else
>     #define REQUANT_64(hi, lo) {\
>         (hi)  = (hi) & 0x00FF00FF;\
>         (hi) |= (hi) << 8;\
>         (lo)  = (lo) & 0x00FF00FF;\
>         (lo) |= (lo) << 8;\
>     }
> #endif

Untested:

#define REQUANT_64_VALUE(hi, lo, value) {\
    (hi)  = (hi) & value;\
    (hi) |= (hi) >> 8;\
    (lo)  = (lo) & value;\
    (lo) |= (lo) >> 8;\

#if HAVE_BIGENDIAN
#define REQUANT_64 REQUANT_64_VALUE(hi, lo, 0xFF00FF00)
#else
#define REQUANT_64 REQUANT_64_VALUE(hi, lo, 0x00FF00FF)
#endif

You might or might not consider this more elegant.

>                         case RLE_ESC_FF: /* apply null delta to all lines up to the 2nd line */
>                             if (line) ERR_BAD_RLE(avctx, mode);
> 
>                         case RLE_ESC_FE: /* apply null delta to all lines up to the 3rd line */
>                             if (line >= 2) ERR_BAD_RLE(avctx, mode);

I'd prefer the block on the next line after the if.  It's what you do in
the other case statements.

>                             case RLE_ESC_FF: /* apply null delta to all lines up to the 2nd line */
>                                 if (line) ERR_BAD_RLE(avctx, mode);
>                                 fill_64(ref + row_offset, ref_lo, ref_hi, 3, row_offset);
>                                 AVG_64 (ref, ref - row_offset, ref + row_offset);
> 
>                             case RLE_ESC_FE: /* apply null delta to all lines up to the 3rd line */
>                                 if (line >= 2) ERR_BAD_RLE(avctx, mode);
>                                 if (is_top_of_cell) {
>                                     fill_64(ref + row_offset, ref_lo, ref_hi, 5, row_offset);
>                                     AVG_64 (ref, ref - row_offset, ref + row_offset);
> 
>                             case RLE_ESC_FD: /* apply null delta to all remaining lines of this block */
>                                 if (is_top_of_cell) {
>                                     fill_64(ref + row_offset, ref_lo, ref_hi, 7, row_offset);
>                                     AVG_64 (ref, ref - row_offset, ref + row_offset);
> 
>                                 /* apply null delta to all remaining lines of this block */
>                                 if (is_top_of_cell) {
>                                     fill_64(ref + row_offset, ref_lo, ref_hi, 7, row_offset);
>                                     AVG_64 (ref, ref - row_offset, ref + row_offset);
>                                 } else
>                                     fill_64(ref, ref_lo, ref_hi, (4 - line) << 1, row_offset);
>                                 num_lines = 4 - line; /* go to process next block */
>                                 break;

The space after the macro name looks a bit weird and you are not really
aligning anything..

>                     if (rle_blocks > 0) {
>                         /* skip the whole next block */
>                         rle_blocks--;
>                     } else {
>                         for(line = 0; line < 4;) {

for (

>                             case RLE_ESC_FF: /* skip all lines up to the 2nd one */
>                                 if (line) ERR_BAD_RLE(avctx, mode);
> 
>                             case RLE_ESC_FE: /* skip all lines up to the 3rd one */
>                                 if (line >= 2) ERR_BAD_RLE(avctx, mode);
> 
>                             case RLE_ESC_FA: /* skip this block */
>                                 if (line) ERR_BAD_RLE(avctx, mode);

see above

>                 skip_bits += bytes_used << 3;
>                 next_cell_data += bytes_used;

align

>     /* check frame dimensions */
>     height = bytestream_get_le16(&buf_ptr);
>     width  = bytestream_get_le16(&buf_ptr);
>     if(avcodec_check_dimensions(avctx, width, height))

if (

> static int decode_frame(AVCodecContext *avctx,
>                                void *data, int *data_size,
>                                AVPacket *avpkt)

indentation

Diego