[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