[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