[FFmpeg-devel] [PATCH 1/3] Indeo 5 decoder: common functions

Reimar Döffinger Reimar.Doeffinger
Sun Jan 17 21:25:36 CET 2010


On Fri, Jan 15, 2010 at 11:45:00AM +0200, Kostya wrote:
> #define IVI_DEBUG

Probably should not be enabled in the final version.
Also I'd suggest to have it always defined and only
change the value between 0 and 1, and wherever possible
use
if (IVI_DEBUG)
thus ensuring the debugging code will not break completely.

> /* see ivi_common.c for a definition */

"a" definition? Also isn't this ivi_common.h, making ivi_common.c
the obvious place to look?

> extern const RVMapDesc ff_ivi_rvmap_tabs[9]; /* defined in ivi_common.c */

Where else should it be "defined"?

> typedef struct {
>     uint8_t         plane;          ///< plane number this band belongs to
>     uint8_t         band_num;       ///< band number
>     uint32_t        width;
>     uint32_t        height;
>     const uint8_t   *data_ptr;      ///< ptr to the first byte of the band data
>     uint32_t        data_size;      ///< size of the band data
>     int16_t         *buf;           ///< pointer to the output buffer for this band
>     int16_t         *ref_buf;       ///< pointer to the reference frame buffer (for motion compensation)
>     int16_t         *bufs[3];       ///< array of pointers to the band buffers
>     uint32_t        pitch;          ///< pitch associated with the buffers above
>     uint8_t         is_empty;       ///< = 1 if this band doesn't contain any data
>     uint8_t         mb_size;        ///< macroblock size
>     uint8_t         blk_size;       ///< block size
>     uint8_t         is_halfpel;     ///< precision of the motion compensation: 0 - fullpel, 1 - halfpel
>     int8_t          inherit_mv;     ///< tells if motion vector is inherited from reference macroblock
>     int8_t          inherit_qdelta; ///< tells if quantiser delta is inherited from reference macroblock
>     int8_t          qdelta_present; ///< tells if Qdelta signal is present in the bitstream (Indeo5 only)
>     uint8_t         quant_mat;      ///< dequant matrix index
>     uint8_t         glob_quant;     ///< quant base for this band

Do all those exact bit sizes make sense? They might be a good bit
slower on some architectures, also if size is a concern you'd want
to reorder this a bit to not waste quite as much on padding.

> 
> static inline int ivi_pic_config_cmp(IVIPicConfig *str1, IVIPicConfig *str2)

No documentation.

> /** convert unsigned values into signed ones (the sign is in the LSB) */
> /* TODO: find a way to calculate this without the conditional using bit magic */
> #define IVI_TOSIGNED(val) (((val) & 1) ? ((val) + 1) >> 1 : -(((val) + 1) >> 1))

I'm sure this kind of code is already in other decoders.
However the + 1 in the part after : sure is useless.
Also I strongly suggest to make this a static inline function.
And bit magic to do it should be
((sign_extend(val & 1, 1) ^ val) >> 1) + 1
I think the +1 is necessary, but I can't say for sure since my request
to document it when it was added was just ignored.

> /** divide the motion vector mv by 4 */
> #define IVI_MV_DIV4(mv) (((mv) + 1 + ((mv) > 0))>>2)
> 
> /** divide the motion vector mv by 2 */
> #define IVI_MV_DIV2(mv) (((mv) + ((mv) > 0))>>1)

Since the mv == 0 case does not matter, you should be able to avoid
the comparison by
static inline int32_t ivi_mv_div4(int32_t mv)
{
    return (mv + 2 - ((uint32_t)mv >> 31)) >> 2;
}
And yes, use static inline functions if you can.

>  *  @param pOut [out] where to place the generated VLC table

pOut? Do we really want that kind of naming?

> int  ff_ivi_init_tiles(IVIPlaneDesc *planes, const int tile_width, const int tile_height);

Drop the useless const.

>  *  @param gb   [in,out] the GetBit context
>  *  @param band [in]     pointer to the band descriptor
>  *  @param tile [in]     pointer to the tile descriptor
>  *  @return     result code: 0 - OK, -1 = error (corrupted blocks data)
>  */
> int  ff_ivi_decode_blocks(GetBitContext *gb, IVIBandDesc *band, IVITile *tile);

Why no const for the ins this time?

> void ff_ivi_process_empty_tile(AVCodecContext *avctx, IVIBandDesc *band, IVITile *tile, int32_t mv_scale);
> void ff_ivi_output_plane(const IVIPlaneDesc *plane, uint8_t *dst, const int dst_pitch);

If there is any sense or sanity behind how const is placed I don't see it.

> #ifdef IVI_DEBUG
> /**
>  *  Calculates band checksum from band data.
>  */
> uint16_t ivi_calc_band_checksum (IVIBandDesc *band);
> 
> /**
>  *  Verifies that band data lies in range.
>  */
> int ivi_check_band (IVIBandDesc *band, uint8_t *ref, int pitch);

Likely could use const?

> /**
>  *  Reverses "nbits" bits of the value "val" and returns the result
>  *  right-justified.

"in the least significant bits" IMO is clearer.

> static uint16_t inv_bits(const uint16_t val, const int nbits)
> {
>     uint16_t res;
> 
>     if (nbits <= 8) {
>         res = av_reverse[val] >> (8-nbits);
>     } else
>         res = ((av_reverse[val & 0xFF] << 8) + (av_reverse[val >> 8])) >> (16-nbits);
> 
>     return res;
> }

Useless const.
And a 16-bit reverse function shared with pcm.c would be welcome in mathematics.h I think.

>         last_row      = !!(i - cb->num_rows + 1); /* = 0 for the last row */

Obfuscate much?
i != cb->num_rows + 1
or maybe better i <= cb->num_rows
IMO a case where the need for a comment hints at a problem with the code.

>         prefix        = ((1 << i) - 1) << (cb->xbits[i] + last_row);

Hm. Maybe some hint at what this is would be good.
Also maybe write as (assuming I am correct)
(1 << i) - (1 << (cb->xbits[i] + last_row))
?

>         /* luma   band buffers will be aligned on 16x16 (max macroblock size) */
>         /* chroma band buffers will be aligned on   8x8 (max macroblock size) */
>         align_fac       = p ? 7 : 15;
>         width_aligned   = (b_width  + align_fac) & ~align_fac;
>         height_aligned  = (b_height + align_fac) & ~align_fac;

Maybe
align_fac = p ? 8 : 16; (or 8 << !!p possibly?)
FFALIGN(b_width, align_fac)
?

>         buf_size        = width_aligned * height_aligned * sizeof(int16_t);

sizeof(*planes[0].bands[0].bufs[0]) maybe?
I agree, a bit ugly

>             band->bufs[0]  = av_malloc(buf_size);
>             band->bufs[1]  = av_malloc(buf_size);
>             if (!band->bufs[0] || !band->bufs[1])
>                 return AVERROR(ENOMEM);

memleak?

>             /* allocate the 3rd band buffer for scalability mode */
>             if (cfg->luma_bands > 1) {
>                 band->bufs[2] = av_malloc(buf_size);
>                 if (!band->bufs[2])
>                     return AVERROR(ENOMEM);

More memleak?
Or is this caught somewhere else?
I'd still find it easier to verify if each function cleans up it own mess.
If not, _document_ that you need to call free on error.

>                     tile->mbs = av_malloc(tile->num_MBs * sizeof(IVIMbInfo));
>                     if (!tile->mbs)
>                         return AVERROR(ENOMEM);

The same

>     blk_size   = band->blk_size;
>     col_mask   = (blk_size == 8) ? 7 : 3; /* column mask for tracking non-zero coeffs */

That's not actually blk_size - 1 ?

>     num_coeffs = 16 << ((blk_size >> 2) & 2); /* 64 - for 8x8 block, 16 - for 4x4 */

What?
Make the comment into code and get rid of the comment.
And give me a knock on the head every time I suggest such an obfuscated code
(I know I do sometimes during reviews).

>         base_tab  = (is_intra) ? band->intra_base  : band->inter_base;
>         scale_tab = (is_intra) ? band->intra_scale : band->inter_scale;

What's up with the ()

>                 memset(trvec, 0, num_coeffs*sizeof(int32_t)); /* zero transform vector */

sizeof(*trvec) ?

>                 #ifdef IVI_DEBUG
>                     if (!val)
>                         av_log(NULL, AV_LOG_ERROR, "Val = 0 encountered!\n");
>                 #endif

As suggested
if (IVI_DEBUG && !val)

>                     q = (base_tab[pos] * scale_tab[quant]) >> 8;
>                     q += !q; // ensure the q always >= 1
>                     if (q > 1) {
>                         val = val * q + FFSIGN(val) * ((q >> 1) - (q & 1));
>                     }

You said the += !q was needed because q was still used later, but I can't see where?
Also unnecessary {}

>     void (*mc_no_delta_func)(int16_t *buf, int16_t *ref_buf, uint32_t pitch,
>                              int mc_type);

ref_buf is not const on purpose?

>         /* copy data from the reference tile into the current one */
>         src = &band->ref_buf[tile->ypos * band->pitch + tile->xpos];
>         dst = &band->buf[tile->ypos * band->pitch + tile->xpos];

Maybe & [] -> +

>             memcpy(dst, src, tile->width*sizeof(int16_t));

You know...
And that's really enough for today.



More information about the ffmpeg-devel mailing list