[FFmpeg-devel] [PATCH] Indeo5 decoder
Maxim
max_pole
Fri Mar 27 16:22:40 CET 2009
Reimar D?ffinger schrieb:
> 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.
>
Ok, good. Could you give me an example please, how to extract and to use
a descryption key in a decoder?
Does such a key come from the command line?
Next problem: frame data needed to be replaced during the decryption!
Unfortunately a decoder receives a const pointer to the input data, so
no replacement can be done. One possible solution for that can be
copying the encrypted part to a extra buffer, decrypt it and then use
the bitstream reader on it. This is no a good solution though because
the length of the decrypted part is not constant (unknown at all) and
allocation/copying bring a significant overhead. Any suggestions?
>
>> +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?
>
moved out of the context, the function is significantly simplified
>
>> + /* 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);
>
changed
>
>> + /* 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.
>
aligned
>
>> + 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.
>
changed here and anywhere else...
>
>> + 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.
>
rewritten - now it uses a function that compares the structs by their
members...
>
>> + for (i = 0; i < ((!p) ? pic_conf.luma_bands : pic_conf.chroma_bands); i++) {
>>
>
> That's a few () too many IMO.
>
removed
>
>> + 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?
>
this make the code less readable IMHO. Therefore leaved as is...
>
>> + /* 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) ...
>
changed here and at some other places...
>
>> + 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 ?
>
changed
>
>> + 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)
>
They don't need to be 0/1, but I like it so. Therefore changed to
"!!(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);
>
simplified
>
>> + 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?
>
separate into a function... The comment "XXX: untested" is added because
I don't have any sample uses that...
>> + 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.
>
changed
>
>> + /* 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?
>
changed
>> + 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 ( ?
>
removed
>> + /* 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).
>
completely removed because the dequant tables are static for now...
>
>> +/**
>> + * inverse "nbits" bits of the value "val" and return the result right-justified
>>
>
> Huh? inverse would usually mean swapping 0 and 1.
>
spelling corrected...
>
>> + */
>> +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;
>
recoded to use "ff_reverse"
>
>> +/**
>> + * 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.
>
now it uses "memcpy". "ff_" prefix added here and to all others global
functions...
>> + /* 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.
>
removed
>> + 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.
>
changed
>> + 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.
>
removed
>
>> + 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?
>
moved into the following loop...
> I am also unsure if you don't end up leaking memory in all those error return
> cases...
>
all checks and returns removed...
The updated patch is in the same thread but a separate post. Thank you!
Regards
Maxim
More information about the ffmpeg-devel
mailing list