[FFmpeg-devel] [PATCH 1/3] Indeo 5 decoder: common functions
Kostya
kostya.shishkov
Mon Jan 18 09:12:33 CET 2010
On Sun, Jan 17, 2010 at 09:25:36PM +0100, Reimar D?ffinger wrote:
> 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.
done
> > /* 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"?
dropped those
> > 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.
changed to int where possible
> >
> > static inline int ivi_pic_config_cmp(IVIPicConfig *str1, IVIPicConfig *str2)
>
> No documentation.
added
> > /** 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.
I've left it as is for now
> > /** 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.
remade it into single function
> > * @param pOut [out] where to place the generated VLC table
>
> pOut? Do we really want that kind of naming?
renamed
> > int ff_ivi_init_tiles(IVIPlaneDesc *planes, const int tile_width, const int tile_height);
>
> Drop the useless const.
dropped, in other places too
> > * @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.
made it more uniform
> > #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?
yep, added
> > /**
> > * Reverses "nbits" bits of the value "val" and returns the result
> > * right-justified.
>
> "in the least significant bits" IMO is clearer.
changed
> > 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.
dropped
> > 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.
renamed and made it clearer
> > 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))
> ?
this form is clearer for me since it shows a prefix of ones shifted to
the left
> > /* 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)
> ?
FFALIGNed
> > buf_size = width_aligned * height_aligned * sizeof(int16_t);
>
> sizeof(*planes[0].bands[0].bufs[0]) maybe?
> I agree, a bit ugly
left as is
> > 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
we have another function which frees all data, doing it right here would
be too messy too
> > 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 ?
it is, changed
> > 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 ()
dropped
> > memset(trvec, 0, num_coeffs*sizeof(int32_t)); /* zero transform vector */
>
> sizeof(*trvec) ?
yes, changed
> > #ifdef IVI_DEBUG
> > if (!val)
> > av_log(NULL, AV_LOG_ERROR, "Val = 0 encountered!\n");
> > #endif
>
> As suggested
> if (IVI_DEBUG && !val)
it is now
> > 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 {}
it produces different results for q = 1 and that affects picture a bit
> > 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?
not at all, added const
> > /* 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 & [] -> +
changed
> > memcpy(dst, src, tile->width*sizeof(int16_t));
>
> You know...
yes, changed
> And that's really enough for today.
Good review anyway. Here is new version.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ivi_common.h
Type: text/x-chdr
Size: 12391 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100118/d68d1d62/attachment.h>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ivi_common.c
Type: text/x-csrc
Size: 43771 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100118/d68d1d62/attachment.c>
More information about the ffmpeg-devel
mailing list