text: . > > 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. > /**; } 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. > * ? > /** > * 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 > > 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 ? > () > 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.