[FFmpeg-devel] [PATCH] Indeo5 decoder
Reimar Döffinger
Reimar.Doeffinger
Fri Mar 20 13:09:03 CET 2009
On Fri, Mar 20, 2009 at 12:13:56PM +0100, Maxim wrote:
> - the clip was protected using a numeric password. We need to find a way
> to decode such a stuff, because indeo5 encrypts the frame data
> internally. It's needed to provide either a possibility to input the
> correct password or a hack for this. Otherwise such encrypted videos
> cannot be decoded properly...
On the demuxer level we already provide a decryption key, I guess it
could be duplicated for decoders.
> +static int ivi5_build_dequant_tables (IVI5DecContext *ctx)
> +{
> + int mat, i, lev;
> + uint32_t q1, q2, sf1, sf2;
> + uint16_t *deq_intra, *deq_inter;
> +
> + /* allocate and build tables for all 6 matrices */
> + for (mat = 0; mat < 6; mat++) {
> + if (mat < 5) {
> + /* allocate 8x8 tables */
> + ctx->deq8x8_intra[mat] = deq_intra = av_malloc(24*64*sizeof(uint16_t));
> + ctx->deq8x8_inter[mat] = deq_inter = av_malloc(24*64*sizeof(uint16_t));
> + if (deq_intra == NULL || deq_inter == NULL)
> + return AVERROR(ENOMEM);
> + }
> + else {
> + /* allocate 4x4 tables */
> + ctx->deq4x4_intra = deq_intra = av_malloc(24*16*sizeof(uint16_t));
> + ctx->deq4x4_inter = deq_inter = av_malloc(24*16*sizeof(uint16_t));
> + if (deq_intra == NULL || deq_inter == NULL)
> + return AVERROR(ENOMEM);
> + }
> +
> + if (mat < 5) {
> + /* build 8x8 intra/inter tables for all 24 quant levels */
> + for (lev = 0; lev < 24; lev++) {
> + sf1 = ivi5_scale_quant_8x8_intra[mat][lev];
> + sf2 = ivi5_scale_quant_8x8_inter[mat][lev];
> +
> + for (i = 0; i < 64; i++) {
> + q1 = (ivi5_base_quant_8x8_intra[mat][i] * sf1) >> 8;
> + q2 = (ivi5_base_quant_8x8_inter[mat][i] * sf2) >> 8;
> + deq_intra[lev*64 + i] = av_clip(q1,1,255);
> + deq_inter[lev*64 + i] = av_clip(q2,1,255);
> + }
> + }
> + }
> + else {
> + /* build 4x4 intra/inter tables for all 24 quant levels */
> + for (lev = 0; lev < 24; lev++) {
> + sf1 = ivi5_scale_quant_4x4_intra[lev];
> + sf2 = ivi5_scale_quant_4x4_inter[lev];
> +
> + for (i = 0; i < 16; i++) {
> + q1 = (ivi5_base_quant_4x4_intra[i] * sf1) >> 8;
> + q2 = (ivi5_base_quant_4x4_inter[i] * sf2) >> 8;
> + deq_intra[lev*16 + i] = av_clip(q1,1,255);
> + deq_inter[lev*16 + i] = av_clip(q2,1,255);
> + }
> + }
> + }
> + } // for mat
This seems needlessly convoluted, why the mat == 5 case not separate
after the loop?
Also these tables seem to be always the same, why are they in the
context instead of static like the VLC tables?
> + /* get 32-bit lock word if present */
> + if (ctx->gop_flags & 0x20) {
> + ctx->lock_word = get_bits_long(&ctx->gb, 32);
> + ctx->is_protected = 1;
> + }
> + else
> + ctx->is_protected = 0;
Maybe
ctx->is_protected = !!(ctx->gop_flags & 0x20);
if (ctx->is_protected)
ctx->lock_word = get_bits_long(&ctx->gb, 32);
> + /* decode number of wavelet bands */
> + pic_conf.luma_bands = get_bits(&ctx->gb,2) * 3 + 1;
> + pic_conf.chroma_bands = get_bits1(&ctx->gb) * 3 + 1;
Align those.
> + if (pic_conf.luma_bands != 1 || pic_conf.chroma_bands != 1) {
> + av_log(avctx,AV_LOG_ERROR,"Scalability is not supported yet!\n");
> + ctx->is_scalable = 1;
> + return -1;
> + }
> + else
> + ctx->is_scalable = 0;
Again:
ctx->is_scalable = pic_conf.luma_bands != 1 || pic_conf.chroma_bands != 1;
if (ctx->is_scalable) ...
> + }
> + else {
} else {
is usually preferred around here.
> + pic_conf.chroma_height = (pic_conf.pic_height + 3)/4;
> + pic_conf.chroma_width = (pic_conf.pic_width + 3)/4;
Align.
> + /* check if picture layout was changed and reallocate buffers */
> + if (memcmp(&pic_conf, &ctx->pic_conf, sizeof(IVIPicConfig))) { /* FIXME: It's ugly I know... */
sizeof(pic_conf) eases at least renaming.
But, unfortunately, it is not only ugly but plain wrong, structures can
have padding and that padding does not necessarily have any specific
value.
> + for (i = 0; i < ((!p) ? pic_conf.luma_bands : pic_conf.chroma_bands); i++) {
That's a few () too many IMO.
> + switch (get_bits(&ctx->gb,2)) {
> + case 0:
> + mb_size = 16;
> + blk_size = 8;
> + break;
> + case 1:
> + mb_size = 8;
> + blk_size = 8;
> + break;
> + case 2:
> + mb_size = 8;
> + blk_size = 4;
> + break;
> + case 3:
> + mb_size = 4;
> + blk_size = 4;
> + break;
> + }
mb_size = blk_size = 4;
if (!get_bits1(...)) {
mb_size <<= 1;
blk_size <<= 1;
}
if (!get_bits1(...))
mb_size <<= 1;
Maybe?
> + /* detect block size changes */
> + if (mb_size != band->mb_size || blk_size != band->blk_size) {
> + band->mb_size = mb_size;
> + band->blk_size = blk_size;
> + blk_size_changed = 1;
> + }
I suggest again
blk_size_changed = mb_size != band->mb_size || blk_size != band->blk_size;
if (blk_size_changed) ...
> + band->inv_transform = (p == 0) ? ivi_inverse_slant_8x8 : ivi_inverse_slant_4x4;
> + band->scan = (p == 0) ? &ivi5_scans8x8[0][0] : ivi5_scan4x4;
!p ? ...
Also isn't &ivi5_scans8x8[0][0] == ivi5_scans8x8 ?
> + band->inherit_mv = (band_flags >> 1) & 1;
> + band->inherit_qdelta = (band_flags >> 3) & 1;
> + band->qdelta_present = (band_flags >> 2) & 1;
Do these absolutely need to be 0/1?
If not, use band_flags & 2, else !!(band_flags & 2)
> + /* read correction pairs */
> + for (i = 0; i < band->num_corr; i++) {
> + band->corr[i*2] = get_bits(&ctx->gb,8);
> + band->corr[i*2+1] = get_bits(&ctx->gb,8);
> + }
Why not just
for (i = 0; i < 2 * band->num_corr; i++)
band->corr[i] = get_bits(&ctx->gb,8);
> + do {
> + len = get_bits(&ctx->gb,8);
> + for (i = 0; i < len; i++) skip_bits(&ctx->gb,8);
> + } while(len);
I've seen that one already somewhere, maybe make it a function?
> + if (band->plane == 0 && band->band_num == 0 && (ctx->frame_flags & 8)) {
> + mb->q_delta = get_vlc2(&ctx->gb, ctx->mb_vlc->table, IVI_VLC_BITS, 1);
> + mb->q_delta = IVI_TOSIGNED(mb->q_delta);
> + }
> + else
> + mb->q_delta = 0;
> +
> + if (band->inherit_mv){
> + /* motion vector inheritance */
> + switch (mv_scale) {
> + case 0:
> + case 1:
> + av_log(avctx,AV_LOG_ERROR,"Unsupported MV scaling!\n");
> + return -1;
> + case 2:
> + mb->mv_x = IVI_MV_FOURTH(ref_mb->mv_x);
> + mb->mv_y = IVI_MV_FOURTH(ref_mb->mv_y);
> + break;
> + }
> + }
> + else
> + mb->mv_x = mb->mv_y = 0; /* no motion vector coded */
There are a few more places like that, at least if it is not absolutely
performance-critical, I consider
mb->q_delta = 0;
if (...)
mb->q_delta = ...
more readable than the if/else variant.
> + if (band->inherit_mv){
> + mb->type = ref_mb->type; /* copy mb_type from corresponding reference mb */
> + }
> + else {
> + if (ctx->frame_type == IVI5_FRAMETYPE_INTRA)
> + mb->type = 0; /* mb_type is always INTRA for intra-frames */
> + else
> + mb->type = get_bits1(&ctx->gb); /* get mb_type from bitstream */
> + }
if (band->inherit_mv)
mb->type = ref_mb->type;
else if (ctx->frame_type == IVI5_FRAMETYPE_INTRA)
mb->type = 0;
else
mb->type = get_bits1(&ctx->gb);
Seems more readable to me.
> + /* decode cbp (coded block pattern) */
> + if (band->mb_size != band->blk_size)
> + mb->cbp = get_bits(&ctx->gb,4); /* there are 4 blocks in a mb therefore cpb is 4 bits long */
> + else
> + mb->cbp = get_bits1(&ctx->gb); /* there is only 1 block in a mb therefore cpb is 1 bit long */
blks_per_mb = band->mb_size != band->blk_size ? 4 : 1;
mb->cbp = get_bits(&ctx->gb, blks_per_mb);
maybe?
> + ctx->pic_conf.pic_width = avctx->width;
> + ctx->pic_conf.pic_height = avctx->height;
> + ctx->pic_conf.chroma_width = (avctx->width + 3)/4;
> + ctx->pic_conf.chroma_height = (avctx->height + 3)/4;
> + ctx->pic_conf.tile_width = avctx->width;
> + ctx->pic_conf.tile_height = avctx->height;
> + ctx->pic_conf.luma_bands = ctx->pic_conf.chroma_bands = 1;
I haven't mentioned it in many places, but where ever similar code
appears in consecutive lines, it should be aligned to match up, like
> + ctx->pic_conf.chroma_width = (avctx->width + 3)/4;
> + ctx->pic_conf.chroma_height = (avctx->height + 3)/4;
> + ivi_output_plane (&ctx->planes[0], ctx->frame.data[0], ctx->frame.linesize[0]);
> + ivi_output_plane (&ctx->planes[2], ctx->frame.data[1], ctx->frame.linesize[1]);
> + ivi_output_plane (&ctx->planes[1], ctx->frame.data[2], ctx->frame.linesize[2]);
Why suddenly those spaces before ( ?
> + /* free dequant tables */
> + for (i = 0; i < 5; i++) {
> + if (ctx->deq8x8_intra[i])
> + av_free(ctx->deq8x8_intra[i]);
> + if (ctx->deq8x8_inter[i])
> + av_free(ctx->deq8x8_inter[i]);
> + }
> +
> + if (ctx->deq4x4_intra)
> + av_free(ctx->deq4x4_intra);
> + if (ctx->deq4x4_inter)
> + av_free(ctx->deq4x4_inter);
The ifs are pointless. Also consider av_freep (not sure if it makes
sense here).
> +/**
> + * inverse "nbits" bits of the value "val" and return the result right-justified
Huh? inverse would usually mean swapping 0 and 1.
> + */
> +static uint16_t ivi_inv_bits (uint16_t val, const int nbits)
> +{
> + uint16_t res = 0;
> + int i;
> +
> + for (i = 0; i < nbits; i++) {
> + res |= (val & 1) << (nbits-i-1);
> + val >>= 1;
> + }
> + return res;
I think you should use ff_reverse, something like
res = (ff_reverse[val & 0xff] << 16) + ff_reverse[val >> 16];
assert(nbits);
res >>= 16 - nbits;
> +/**
> + * copy huffman codebook descriptors
> + *
> + * @param dst [out] ptr to the destination descriptor
> + * @param src [in] ptr to the source descriptor
> + */
> +void ivi_huff_desc_copy (IVIHuffDesc *dst, const IVIHuffDesc *src)
> +{
> + int i;
> +
> + dst->num_rows = src->num_rows;
> +
> + for (i = 0; i < src->num_rows; i++)
> + dst->xbits[i] = src->xbits[i];
memcpy?
Also note that all global symbols need a ff_ prefix.
> + /* allocate luma buffers (aligned on 16x16 for simplicity) */
> + if (planes[0].buf1)
> + av_free(planes[0].buf1);
> + if (planes[0].buf2)
> + av_free(planes[0].buf2);
Pointless ifs.
> + planes[0].pitch = width_aligned = (cfg->pic_width + 15) & -16;
> + height_aligned = (cfg->pic_height + 15) & -16;
-16 assumes two's complement representation for int.
While FFmpeg needs that anyway, ~15 is better style.
> + if (planes[1].buf1 == NULL)
> + return AVERROR(ENOMEM);
> + planes[2].buf1 = av_malloc(buf_size);
> + if (planes[2].buf1 == NULL)
> + return AVERROR(ENOMEM);
> + planes[1].buf2 = av_malloc(buf_size);
> + if (planes[1].buf2 == NULL)
> + return AVERROR(ENOMEM);
> + planes[2].buf2 = av_malloc(buf_size);
> + if (planes[2].buf2 == NULL)
> + return AVERROR(ENOMEM);
The == NULL parts are pointless.
> + if (planes[0].bands)
> + av_free(planes[0].bands);
> + planes[0].bands = av_mallocz(cfg->luma_bands * sizeof(IVIBandDesc));
> + if (planes[0].bands == NULL)
> + return AVERROR(ENOMEM);
> +
> + if (planes[1].bands)
> + av_free(planes[1].bands);
> + planes[1].bands = av_mallocz(cfg->chroma_bands * sizeof(IVIBandDesc));
> + if (planes[1].bands == NULL)
> + return AVERROR(ENOMEM);
> +
> + if (planes[2].bands)
> + av_free(planes[2].bands);
> + planes[2].bands = av_mallocz(cfg->chroma_bands * sizeof(IVIBandDesc));
> + if (planes[2].bands == NULL)
> + return AVERROR(ENOMEM);
And anyway, why isn't that inside the following loop?
I am also unsure if you don't end up leaking memory in all those error return
cases...
More information about the ffmpeg-devel
mailing list