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

Kostya kostya.shishkov
Fri Jan 15 10:45:00 CET 2010


On Fri, Jan 15, 2010 at 12:04:20AM +0100, Michael Niedermayer wrote:
> On Sun, Jan 03, 2010 at 12:55:32PM +0200, Kostya wrote:
> > Since Maxim's decoder seems to work fine and code is reasonably good
> > too, I'm sending it here for review, so it can be polished and
> > committed.
> > 
[...]
> 
> doxy
 
documented missing fields (except obvious width and height)

[...] 
> > } IVIBandDesc;
> 
> 
> and are all thouse structs neeed? cant the bitstream be decoded more
> directly without all the block, tile, ... intermediates?

Of course, but since they reflect logical grouping I'd rather leave
them.
 
> [...]
> > /**
> >  *  Reverses "nbits" bits of the value "val" and returns the result
> >  *  right-justified.
> >  */
> > static uint16_t inv_bits(const uint16_t val, const int nbits)
> > {
> >     uint16_t res;
> > 
> >     if (nbits <= 8) {
> >         res = av_reverse[val & 0xFF] >> (8-nbits);
> 
> is the & ff needed here?

no, removed
 
> [...]
> > int ff_ivi_huff_desc_cmp(const IVIHuffDesc *desc1, const IVIHuffDesc *desc2)
> > {
> >     if (desc1->num_rows != desc2->num_rows ||
> >                            memcmp(desc1->xbits, desc2->xbits, desc1->num_rows))
> >         return 1;
> >     return 0;
> 
> return    desc1->num_rows != desc2->num_rows
>        || memcmp (desc1->xbits, desc2->xbits, desc1->num_rows);

changed 
 
> > }
> > 
> > void ff_ivi_huff_desc_copy(IVIHuffDesc *dst, const IVIHuffDesc *src)
> > {
> >     dst->num_rows = src->num_rows;
> >     memcpy(dst->xbits, src->xbits, src->num_rows);
> > }
> > 
> > int av_cold ff_ivi_init_planes(IVIPlaneDesc *planes, const IVIPicConfig *cfg)
> > {
> >     int         p, b;
> >     uint32_t    b_width, b_height, align_fac, width_aligned, height_aligned, buf_size;
> >     IVIBandDesc *band;
> > 
> >     ff_ivi_free_buffers(planes);
> > 
> >     /* fill in the descriptor of the luminance plane */
> >     planes[0].width     = cfg->pic_width;
> >     planes[0].height    = cfg->pic_height;
> >     planes[0].num_bands = cfg->luma_bands;
> > 
> >     /* fill in the descriptors of the chrominance planes */
> >     planes[1].width     = planes[2].width     = (cfg->pic_width  + 3) >> 2;
> >     planes[1].height    = planes[2].height    = (cfg->pic_height + 3) >> 2;
> >     planes[1].num_bands = planes[2].num_bands = cfg->chroma_bands;
> > 
> >     for (p = 0; p < 3; p++) {
> >         planes[p].bands = av_mallocz(planes[p].num_bands * sizeof(IVIBandDesc));
> > 
> >         /* select band dimensions: if there is only one band then it
> >          *  has the full size, if there are several bands each of them
> >          *  has only half size */
> 
> >         b_width  = planes[p].num_bands == 1 ? planes[p].width  : planes[p].width  >> 1;
> >         b_height = planes[p].num_bands == 1 ? planes[p].height : planes[p].height >> 1;
> 
> is the rounding correct? i mean not +1 )>>1 ?

should not matter (never seen a file with width & 3) but changed to rounding version anyway 
 
> > 
> >         /* 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 ? 15 : 7;
> 
> p ? 7:15

changed
 
> >         width_aligned   = (b_width  + align_fac) & ~align_fac;
> >         height_aligned  = (b_height + align_fac) & ~align_fac;
> >         buf_size        = width_aligned * height_aligned * sizeof(int16_t);
> > 
> >         for (b = 0; b < planes[p].num_bands; b++) {
> >             band = &planes[p].bands[b]; /* select appropriate plane/band */
> >             band->plane    = p;
> >             band->band_num = b;
> >             band->width    = b_width;
> >             band->height   = b_height;
> >             band->pitch    = width_aligned;
> 
> >             band->bufs[0]  = av_malloc(buf_size);
> >             band->bufs[1]  = av_malloc(buf_size);
> > 
> >             /* allocate the 3rd band buffer for scalability mode */
> >             if (cfg->luma_bands > 1)
> >                 band->bufs[2] = av_malloc(buf_size);
> 
> malloc NULL checks

added 
 
> [...]
> > int av_cold ff_ivi_init_tiles(IVIPlaneDesc *planes, const int tile_width,
> >                               const int tile_height)
> > {
> >     int         p, b, x, y, x_tiles, y_tiles, t_width, t_height;
> >     IVIBandDesc *band;
> >     IVITile     *tile, *ref_tile;
> > 
> >     for (p = 0; p < 3; p++) {
> >         t_width  = !p ? tile_width  : (tile_width  + 3) >> 2;
> >         t_height = !p ? tile_height : (tile_height + 3) >> 2;
> > 
> >         for (b = 0; b < planes[p].num_bands; b++) {
> >             band = &planes[p].bands[b];
> 
> >             x_tiles = IVI_NUM_TILES(band->width, t_width);
> >             y_tiles = IVI_NUM_TILES(band->height, t_height);
> 
> vertical align

aligned
 
> [...]
> > void ff_ivi_put_pixels_8x8(int32_t *in, int16_t *out, uint32_t pitch,
> >                            uint8_t *flags)
> 
> missing const
> unused flags

It's for DSP, moved there.
And flags are there for common interface with transform functions.

> [...]
> > int ff_ivi_decode_blocks(GetBitContext *gb, IVIBandDesc *band, IVITile *tile)
> > {
> >     int         mbn, blk, num_blocks, num_coeffs, blk_size, scan_pos, run, val,
> >                 pos, is_intra, mc_type, mv_x, mv_y, col_mask;
> >     uint8_t     col_flags[8];
> >     int32_t     prev_dc, trvec[64];
> >     uint32_t    cbp, sym, lo, hi, quant, buf_offs, q;
> >     IVIMbInfo   *mb;
> >     RVMapDesc   *rvmap = band->rv_map;
> >     void (*mc_with_delta_func)(int16_t *buf, int16_t *ref_buf, uint32_t pitch, int mc_type);
> >     void (*mc_no_delta_func)  (int16_t *buf, int16_t *ref_buf, uint32_t pitch, int mc_type);
> >     const uint8_t   *base_tab, *scale_tab;
> > 
> >     prev_dc = 0; /* init intra prediction for the DC coefficient */
> > 
> >     blk_size   = band->blk_size;
> >     col_mask   = (blk_size == 8) ? 7 : 3; /* column mask for tracking non-zero coeffs */
> >     num_blocks = (band->mb_size != blk_size) ? 4 : 1; /* number of blocks per mb */
> >     num_coeffs = 16 << ((blk_size >> 2) & 2); /* 64 - for 8x8 block, 16 - for 4x4 */
> >     if (blk_size == 8) {
> >         mc_with_delta_func = ff_ivi_mc_8x8_delta;
> >         mc_no_delta_func   = ff_ivi_mc_8x8_no_delta;
> >     } else {
> >         mc_with_delta_func = ff_ivi_mc_4x4_delta;
> >         mc_no_delta_func   = ff_ivi_mc_4x4_no_delta;
> >     }
> > 
> >     for (mbn = 0, mb = tile->mbs; mbn < tile->num_MBs; mb++, mbn++) {
> >         is_intra = !mb->type;
> >         cbp      = mb->cbp;
> >         buf_offs = mb->buf_offs;
> > 
> >         quant = av_clip(band->glob_quant + mb->q_delta, 0, 23);
> > 
> >         base_tab  = (is_intra) ? band->intra_base  : band->inter_base;
> >         scale_tab = (is_intra) ? band->intra_scale : band->inter_scale;
> > 
> >         if (!is_intra) {
> >             mv_x = mb->mv_x;
> >             mv_y = mb->mv_y;
> >             if (!band->mc_resolution) {
> >                 mc_type = 0; /* we have only fullpel vectors */
> >             } else {
> >                 mc_type = ((mv_y & 1) << 1) | (mv_x & 1);
> >                 mv_x >>= 1;
> >                 mv_y >>= 1; /* convert halfpel vectors into fullpel ones */
> >             }
> >         }
> > 
> >         for (blk = 0; blk < num_blocks; blk++) {
> >             /* adjust block position in the buffer according to its number */
> >             if (blk & 1) {
> >                 buf_offs += blk_size;
> >             } else if (blk == 2) {
> >                 buf_offs -= blk_size;
> >                 buf_offs += blk_size * band->pitch;
> >             }
> > 
> >             if (cbp & 1) { /* block coded ? */
> >                 scan_pos = -1;
> >                 memset(trvec, 0, num_coeffs*sizeof(int32_t)); /* zero transform vector */
> >                 memset(col_flags, 0, sizeof(col_flags));      /* zero column flags */
> > 
> 
> >                 while (scan_pos <= num_coeffs) {
> >                     sym = get_vlc2(gb, band->blk_vlc->table, IVI_VLC_BITS, 1);
> >                     if (sym == rvmap->eob_sym)
> >                         break; /* End of block */
> > 
> >                     if (sym == rvmap->esc_sym) { /* Escape - run/val explicitly coded using 3 vlc codes */
> >                         run = get_vlc2(gb, band->blk_vlc->table, IVI_VLC_BITS, 1) + 1;
> >                         lo  = get_vlc2(gb, band->blk_vlc->table, IVI_VLC_BITS, 1);
> >                         hi  = get_vlc2(gb, band->blk_vlc->table, IVI_VLC_BITS, 1);
> >                         val = IVI_TOSIGNED((hi << 6) | lo); /* merge them and convert into signed val */
> >                     } else {
> >                         run = rvmap->runtab[sym];
> >                         val = rvmap->valtab[sym];
> >                     }
> > 
> >                     /* de-zigzag and dequantize */
> >                     scan_pos += run;
> >                     pos = band->scan[scan_pos];
> 
> segfault? run isnt checked
> some tests with a fuzzer might make sense

now it's checked 

> > 
> >                 #ifdef IVI_DEBUG
> >                     if (!val)
> >                         av_log(NULL, AV_LOG_ERROR, "Val = 0 encountered!\n");
> >                 #endif
> > 
> >                     q = (base_tab[pos] * scale_tab[quant]) >> 8;
> 
> >                     q += !q; // ensure the q always >= 1
> >                     if (q != 1) {
> 
> if(q>1) {
> 
> 
> >                         if (val > 0) {
> >                             val = (val * q) + (q >> 1) - (q & 1);
> >                         } else
> >                             val = (val * q) - (q >> 1) + (q & 1);
> 
> val can be stored in abs+sign which would avoid the branch misprediction prone
> check

changed it 
 
> [...]
> >                 /* adjust block position in the buffer according with its number */
> >                 if (blk & 1) {
> >                     offs += band->blk_size;
> >                 } else if (blk == 2) {
> >                     offs -= band->blk_size;
> >                     offs += band->blk_size * band->pitch;
> >                 }
> 
> looks duplicated

rewrote it a bit
 
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ivi_common.h
Type: text/x-chdr
Size: 12492 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100115/b792e8f6/attachment.h>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ivi_common.c
Type: text/x-csrc
Size: 44206 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100115/b792e8f6/attachment.c>



More information about the ffmpeg-devel mailing list