[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