[FFmpeg-devel] [PATCH] RV30/40 decoder splitted
Kostya
kostya.shishkov
Thu Sep 20 07:27:02 CEST 2007
On Wed, Sep 19, 2007 at 11:05:35PM +0200, Michael Niedermayer wrote:
> Hi
>
> On Wed, Sep 19, 2007 at 01:35:43PM +0300, Kostya wrote:
> > Here are split rv30 and rv40 decoders.
> > Due to the lack of time I have not committed all of this to rv40 SVN
> > and this patch lacks documentation. But all code should be here.
> >
> > Please comment.
>
> [...]
> > + int maxbits = 0, realsize;
> > + int ret;
> > +
> > + realsize = 0;
>
> the int and =0 is inconsistent
fixed
> [...]
> > + for(i = 0; i < NUM_INTRA_TABLES; i++){
> > + for(j = 0; j < 2; j++)
> > + rv34_gen_vlc(rv34_intra_cbppatvlc_pointers[i][j], CBPPAT_VLC_SIZE, &intra_vlcs[i].cbppattern[j]);
> > + for(j = 0; j < 2; j++)
> > + for(k = 0; k < 4; k++)
> > + rv34_gen_vlc(rv34_intra_cbpvlc_pointers[i][j][k], CBP_VLC_SIZE, &intra_vlcs[i].cbp[j][k]);
> > + for(j = 0; j < 4; j++)
> > + rv34_gen_vlc(rv34_intra_firstpatvlc_pointers[i][j], FIRSTBLK_VLC_SIZE, &intra_vlcs[i].first_pattern[j]);
> > + for(j = 0; j < 2; j++)
> > + rv34_gen_vlc(rv34_intra_secondpatvlc_pointers[i][j], OTHERBLK_VLC_SIZE, &intra_vlcs[i].second_pattern[j]);
> > + for(j = 0; j < 2; j++)
> > + rv34_gen_vlc(rv34_intra_thirdpatvlc_pointers[i][j], OTHERBLK_VLC_SIZE, &intra_vlcs[i].third_pattern[j]);
>
> all the for(j = 0; j < 2; j++) loops can be merged
done
> > + rv34_gen_vlc(rv34_intra_coeffvlc_pointers[i], COEFF_VLC_SIZE, &intra_vlcs[i].coefficient);
> > + }
> > +
> > + for(i = 0; i < NUM_INTER_TABLES; i++){
> > + rv34_gen_vlc(rv34_inter_cbppatvlc_pointers[i], CBPPAT_VLC_SIZE, &inter_vlcs[i].cbppattern[0]);
> > + for(j = 0; j < 4; j++)
> > + rv34_gen_vlc(rv34_inter_cbpvlc_pointers[i][j], CBP_VLC_SIZE, &inter_vlcs[i].cbp[0][j]);
> > + for(j = 0; j < 2; j++)
> > + rv34_gen_vlc(rv34_inter_firstpatvlc_pointers[i][j], FIRSTBLK_VLC_SIZE, &inter_vlcs[i].first_pattern[j]);
> > + for(j = 0; j < 2; j++)
> > + rv34_gen_vlc(rv34_inter_secondpatvlc_pointers[i][j], OTHERBLK_VLC_SIZE, &inter_vlcs[i].second_pattern[j]);
> > + for(j = 0; j < 2; j++)
> > + rv34_gen_vlc(rv34_inter_thirdpatvlc_pointers[i][j], OTHERBLK_VLC_SIZE, &inter_vlcs[i].third_pattern[j]);
>
> same here
merged
> [...]
> > +/**
> > + * Real Video 4.0 inverse transform
> > + * Code is almost the same as in SVQ3, only scaling is different
> > + */
> > +static void rv34_intra_inv_transform(DCTELEM *block, const int offset){
> > + int temp[16];
> > + unsigned int i;
>
> hmm
I will make patch to use SVQ3 transform with inplace dequantization and use it
for both SVQ3 and RV40 but it will take some time.
> [...]
> > + for(mask = 8; mask; mask >>= 1, curshift++){
> > + if(!(pattern & mask)) continue;
> > + t = get_vlc2(gb, vlc->cbp[table][table2].table, vlc->cbp[table][table2].bits, 1);
> > + cbp |= rv34_cbp_code[t] << curshift[0];
> > + }
>
> i think:
>
> v= vlc->cbp[table][table2];
> for(i=0; i<4; i++){
> if(pattern & (8>>i)){
> t = get_vlc2(gb, v->table, v->bits, 1);
> cbp |= rv34_cbp_code[t] << shifts[i];
> }
> }
>
> is clearer, though your variant is fine as well
I will leave it as is for now.
> [...]
> > +/**
> > + * Decode quantizer difference and return modified quantizer
> > + */
> > +static inline int rv34_decode_dquant(GetBitContext *gb, int quant)
> > +{
>
> > + if(get_bits1(gb))
> > + return av_clip(quant + rv34_dquant_tab[quant * 2 + get_bits1(gb)], 0, 31);
>
> the cliping should be unneeded as the table already can ensure that no
> overflow happens
You are right, dropped av_clip()
> [...]
> > +/**
> > + * Decode motion vector differences
> > + * and perform motion vector reconstruction and motion compensation.
> > + */
> > +static int rv34_decode_mv(RV34DecContext *r, int block_type)
> > +{
> > + MpegEncContext *s = &r->s;
> > + GetBitContext *gb = &s->gb;
> > + int i, j;
> > +
> > + switch(block_type){
> > + case RV34_MB_TYPE_INTRA:
> > + case RV34_MB_TYPE_INTRA16x16:
> > + for(j = 0; j < 2; j++){
> > + for(i = 0; i < 2; i++){
> > + s->current_picture_ptr->motion_val[0][s->mb_x * 2 + s->mb_y * 2 * s->b8_stride + i + j*s->b8_stride][0] = 0;
> > + s->current_picture_ptr->motion_val[0][s->mb_x * 2 + s->mb_y * 2 * s->b8_stride + i + j*s->b8_stride][1] = 0;
> > + }
> > + }
> > + return 0;
> > + case RV34_MB_SKIP:
> > + r->dmv[0][0] = 0;
> > + r->dmv[0][1] = 0;
> > + if(s->pict_type == P_TYPE){
> > + rv34_pred_mv(r, block_type, 0);
> > + rv34_mc(r, block_type, 0, 0, 0, 2, 2);
> > + break;
> > + }
> > + case RV34_MB_B_INTERP:
> > + r->dmv[0][0] = 0;
> > + r->dmv[0][1] = 0;
> > + r->dmv[1][0] = 0;
> > + r->dmv[1][1] = 0;
> > + rv34_pred_mv_b (r, RV34_MB_B_INTERP);
> > + rv34_mc_b (r, RV34_MB_B_INTERP);
> > + rv34_mc_b_interp(r, RV34_MB_B_INTERP);
> > + break;
> > + case RV34_MB_P_16x16:
> > + case RV34_MB_P_MIX16x16:
> > + r->dmv[0][0] = rv34_get_omega_signed(gb);
> > + r->dmv[0][1] = rv34_get_omega_signed(gb);
> > + rv34_pred_mv(r, block_type, 0);
> > + rv34_mc(r, block_type, 0, 0, 0, 2, 2);
> > + break;
> > + case RV34_MB_B_FORWARD:
> > + case RV34_MB_B_BACKWARD:
> > + r->dmv[0][0] = rv34_get_omega_signed(gb);
> > + r->dmv[0][1] = rv34_get_omega_signed(gb);
> > + rv34_pred_mv_b (r, block_type);
> > + rv34_mc_b (r, block_type);
> > + break;
> > + case RV34_MB_P_16x8:
> > + case RV34_MB_P_8x16:
> > + case RV34_MB_B_DIRECT:
> > + r->dmv[0][0] = rv34_get_omega_signed(gb);
> > + r->dmv[0][1] = rv34_get_omega_signed(gb);
> > + r->dmv[1][0] = rv34_get_omega_signed(gb);
> > + r->dmv[1][1] = rv34_get_omega_signed(gb);
> > + rv34_pred_mv(r, block_type, 0);
> > + rv34_pred_mv(r, block_type, 1);
> > + if(block_type == RV34_MB_P_16x8){
> > + rv34_mc(r, block_type, 0, 0, 0, 2, 1);
> > + rv34_mc(r, block_type, 0, 8, s->b8_stride, 2, 1);
> > + }
> > + if(block_type == RV34_MB_P_8x16){
> > + rv34_mc(r, block_type, 0, 0, 0, 1, 2);
> > + rv34_mc(r, block_type, 8, 0, 1, 1, 2);
> > + }
> > + if(block_type == RV34_MB_B_DIRECT){
> > + rv34_pred_mv_b (r, block_type);
> > + rv34_mc_b (r, block_type);
> > + rv34_mc_b_interp(r, block_type);
> > + }
> > + break;
> > + case RV34_MB_P_8x8:
> > + for(i=0;i< 4;i++){
> > + r->dmv[i][0] = rv34_get_omega_signed(gb);
> > + r->dmv[i][1] = rv34_get_omega_signed(gb);
> > + rv34_pred_mv(r, block_type, i);
> > + rv34_mc(r, block_type, (i&1)<<3, (i&2)<<2, (i&1)+(i>>1)*s->b8_stride, 1, 1);
> > + }
> > + break;
> > + }
> > +
> > + return 0;
> > +}
>
> mv_count= tab[block_type];
> for(i=0; i<mv_count; i++){
> r->dmv[i][0] = rv34_get_omega_signed(gb);
> r->dmv[i][1] = rv34_get_omega_signed(gb);
> }
Will do this way.
> [...]
> > + if(no_up && no_left)
> > + itype = DC_128_PRED8x8;
> > + else if(no_up){
> > + if(itype == PLANE_PRED8x8)itype = HOR_PRED8x8;
> > + if(itype == VERT_PRED8x8) itype = HOR_PRED8x8;
> > + if(itype == DC_PRED8x8) itype = LEFT_DC_PRED8x8;
> > + }else if(no_left){
> > + if(itype == PLANE_PRED8x8)itype = VERT_PRED8x8;
> > + if(itype == HOR_PRED8x8) itype = VERT_PRED8x8;
> > + if(itype == DC_PRED8x8) itype = TOP_DC_PRED8x8;
>
> > + }
> > + r->h.pred16x16[itype](Y, s->linesize);
> > + dsp->add_pixels_clamped(s->block[0], Y, s->current_picture.linesize[0]);
> > + dsp->add_pixels_clamped(s->block[1], Y + 8, s->current_picture.linesize[0]);
> > + Y += s->current_picture.linesize[0] * 8;
> > + dsp->add_pixels_clamped(s->block[2], Y, s->current_picture.linesize[0]);
> > + dsp->add_pixels_clamped(s->block[3], Y + 8, s->current_picture.linesize[0]);
> > +
> > + itype = ittrans16[intra_types[0]];
> > + if(itype == PLANE_PRED8x8) itype = DC_PRED8x8;
>
> > + if(no_up && no_left)
> > + itype = DC_128_PRED8x8;
> > + else if(no_up){
> > + if(itype == VERT_PRED8x8) itype = HOR_PRED8x8;
> > + if(itype == DC_PRED8x8) itype = LEFT_DC_PRED8x8;
> > + }else if(no_left){
> > + if(itype == HOR_PRED8x8) itype = VERT_PRED8x8;
> > + if(itype == DC_PRED8x8) itype = TOP_DC_PRED8x8;
> > + }
>
> duplicate
Actually it's similar but not duplicate - case 3(PLANE_PRED) is treated as DC
prediction for chroma blocks, so on corner cases it results in different types
of prediction.
> [...]
> > + if(!r->si.type && !r->rv30){
> > + r->is16 = 0;
> > + switch(decode210(gb)){
> > + case 0: // 16x16 block
> > + r->is16 = 1;
> > + break;
> > + case 1:
> > + break;
> > + case 2:
> > + av_log(s->avctx, AV_LOG_ERROR, "Need DQUANT\n");
> > + // q = decode_dquant(gb);
> > + break;
> > + }
> > + s->current_picture_ptr->mb_type[mb_pos] = r->is16 ? MB_TYPE_INTRA16x16 : MB_TYPE_INTRA;
> > + r->block_type = r->is16 ? RV34_MB_TYPE_INTRA16x16 : RV34_MB_TYPE_INTRA;
> > + }else if(!r->si.type && r->rv30){
> > + r->is16 = get_bits1(gb);
> > + s->current_picture_ptr->mb_type[mb_pos] = r->is16 ? MB_TYPE_INTRA16x16 : MB_TYPE_INTRA;
> > + r->block_type = r->is16 ? RV34_MB_TYPE_INTRA16x16 : RV34_MB_TYPE_INTRA;
> > + }else{
>
> if(!r->si.type){
> r->is16= get_bits1(gb))
> if(!r->is16 && !r->rv30){
> int t=get_bits1(gb);
> if(!t)
> av_log(s->avctx, AV_LOG_ERROR, "Need DQUANT\n");
> }
> s->current_picture_ptr->mb_type[mb_pos] = r->is16 ? MB_TYPE_INTRA16x16 : MB_TYPE_INTRA;
> r->block_type = r->is16 ? RV34_MB_TYPE_INTRA16x16 : RV34_MB_TYPE_INTRA;
> }else{
Will do this way.
> [...]
> > + if(!r->is16){
> > + if(r->decode_intra_types(r, gb, intra_types) < 0)
> > + return -1;
> > + r->chroma_vlc = 0;
> > + r->luma_vlc = 1;
> > + }else{
> > + t = get_bits(gb, 2);
> > + for(i = 0; i < 16; i++)
> > + intra_types[(i & 3) + (i>>2) * r->intra_types_stride] = t;
> > + r->chroma_vlc = 0;
> > + r->luma_vlc = 2;
> > + }
>
> chroma_vlc can be factored out of that if()
ok
> [...]
> > +/**
> > + * Initialize decoder
> > + * @todo Maybe redone in some other way
> > + */
> > +int rv34_decode_init(AVCodecContext *avctx)
> > +{
> > + RV34DecContext *r = avctx->priv_data;
> > + MpegEncContext *s = &r->s;
> > +
> > + static int tables_done = 0;
> > +
> > + r->rv30 = (avctx->codec_id == CODEC_ID_RV30);
>
> you set this to 0/1 in the specific init functions and here again ...
Leftover, dropped.
> > + MPV_decode_defaults(s);
> > + s->avctx= avctx;
> > + s->out_format = FMT_H263;
> > + s->codec_id= avctx->codec_id;
> > +
> > + s->width = avctx->width;
> > + s->height = avctx->height;
> > +
> > + r->s.avctx = avctx;
> > + avctx->flags |= CODEC_FLAG_EMU_EDGE;
> > + r->s.flags |= CODEC_FLAG_EMU_EDGE;
> > + avctx->pix_fmt = PIX_FMT_YUV420P;
> > + avctx->has_b_frames = 1;
> > + s->low_delay = 0;
> > +
> > + if (MPV_common_init(s) < 0)
> > + return -1;
> > +
> > + ff_h264_pred_init(&r->h, CODEC_ID_RV40);
>
> shouldnt that be avctx->codec_id ?
For now there's no handling of CODEC_ID_RV30 in h264pred.
> > +
> > + r->intra_types_stride = (s->mb_width + 1) * 4;
> > + r->intra_types_hist = av_malloc(r->intra_types_stride * 4 * 2 * sizeof(int));
> > + r->intra_types = r->intra_types_hist + r->intra_types_stride * 4;
> > +
>
> > + r->mb_type = av_mallocz(r->s.mb_stride * r->s.mb_height * sizeof(int));
>
> please use sizeof(*r->mb_type) it really can safe time if someone ever changes
> the type ...
done
> [...]
> > + //rv40_postprocess(r);
>
> forgotten?
Left as a remainder that it should be implemented but it is not working.
> [...]
> > +/**
> > + * RV30 and RV40 Macroblock types
> > + * @{
> > + */
> > +enum RV40BlockTypes{
> > + RV34_MB_TYPE_INTRA, ///< Intra macroblock
> > + RV34_MB_TYPE_INTRA16x16, ///< Intra macroblock with DCs in a separate 4x4 block
> > + RV34_MB_P_16x16, ///< P-frame macroblock, one motion frame
> > + RV34_MB_P_8x8, ///< P-frame macroblock, 8x8 motion compensation partitions
> > + RV34_MB_B_FORWARD, ///< B-frame macroblock, forward prediction
> > + RV34_MB_B_BACKWARD, ///< B-frame macroblock, backward prediction
> > + RV34_MB_SKIP, ///< Skipped block
> > + RV34_MB_B_INTERP, ///< Bidirectionally predicted B-frame macroblock, no motion vectors
> > + RV34_MB_P_16x8, ///< P-frame macroblock, 16x8 motion compensation partitions
> > + RV34_MB_P_8x16, ///< P-frame macroblock, 8x16 motion compensation partitions
> > + RV34_MB_B_DIRECT, ///< Bidirectionally predicted B-frame macroblock, two motion vectors
> > + RV34_MB_P_MIX16x16, ///< P-frame macroblock with DCs in a separate 4x4 block, one motion vector
> > + RV34_MB_TYPES
> > +};
> > +/** @} */
>
> for what is the @{} good here ?
looks like it does effectively nothing, removed
> [...]
> > +/** Decoder context */
> > +typedef struct RV34DecContext{
> > + MpegEncContext s;
> > + int mb_bits; ///< bits needed to read MB offet in slice header
> > + int *intra_types_hist; ///< old block types, used for prediction
> > + int *intra_types; ///< block types
>
> > + int intra_types_stride; ///< stride for block types data
>
> hmm, ignoring my reviews must be contagious
> expect to be flamed next time
> note: either fix what i complain about or say why it cant be done or that you
> dont want to do it, dont just ignore things
My changes have a great lag sometimes. Changed to b4_stride.
> [...]
> > +/**
> > + * Common decoding functions
> > + */
> > +int rv34_get_start_offset(GetBitContext *gb, int blocks);
> > +int rv34_get_omega(GetBitContext *gb);
> > +int rv34_decode_init(AVCodecContext *avctx);
> > +int rv34_decode_frame(AVCodecContext *avctx, void *data, int *data_size, uint8_t *buf, int buf_size);
> > +int rv34_decode_end(AVCodecContext *avctx);
>
> ff prefixes please we arent the official real video people for whom a rv
> prefix would maybe be ok
already done in SVN
> [...]
> > +/**
> > + * This table is used for retrieving current intra type
> > + * basing on its neighbours and adjustment provided by
> > + * code read and decoded before.
> > + *
> > + * This is really three-dimensional matrix with dimensions
> > + * [-1..9][-1..9][0..9], first and second coordinates
> > + * are detemined by top and left neighbours (-1 if unavailable).
> > + */
> > +static const uint8_t rv30_itype_from_context[900] = {
> > + 0, 9, 9, 9, 9, 9, 9, 9, 9,
>
> see previous reviews
It is not desirable. rv30_itype_code may be compacted easily because
values are read by pairs from it but this one has its data read from
different positions depending on rv30_itype_code, for example last
offsets may be 3 and 0 or 0 and 4 so packing it would be counterproductive.
> [...]
> > +/**
> > + * Get stored dimension from bitstream
> > + *
> > + * If the width/height is the standard one then it's coded as 3-bit index.
> > + * Otherwise it is coded as escaped 8-bit portions.
> > + */
> > +static int get_dimension(GetBitContext *gb, const int *dim1, const int *dim2)
> > +{
> > + int val, t;
> > +
> > + t = get_bits(gb, 3);
> > + val = dim1[t];
> > + if(!val && dim2)
> > + val = dim2[(t*2 | get_bits1(gb)) & 3];
> > + if(!val){
> > + do{
> > + t = get_bits(gb, 8);
> > + val += t << 2;
> > + }while(t == 0xFF);
> > + }
> > + return val;
> > +}
>
> this function is duplicated, please remove ALL duplicated functions
Leftover from rv40, dropped. I think there are no more duplicated functions left.
> [...]
> > +static int rv40_parse_slice_header(RV34DecContext *r, GetBitContext *gb, SliceInfo *si)
> > +{
> > + int t, mb_bits;
> > + int w = r->s.width, h = r->s.height;
> > + int mb_size;
> > +
> > + memset(si, 0, sizeof(SliceInfo));
>
> > + si->type = -1;
> > + if(get_bits1(gb))
> > + return -1;
> > + si->type = get_bits(gb, 2);
>
> hmm
All changes are and will be reflected in SOC-SVN, I see no sense submitting it here
unless RM demuxer problem is resolved.
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
More information about the ffmpeg-devel
mailing list