[FFmpeg-devel] [PATCH] Chinese AVS encoder

[Michael Niedermayer]

Hi

On Wed, Jul 25, 2007 at 07:50:17AM +0200, Stefan Gehrer wrote:
[...]
> > 
> > [...]
> >> +/**
> >> + * eliminate residual blocks that only have insignificant coefficients,
> >> + * inspired from x264 and JVT-B118
> >> + */
> >> +static inline int decimate_block(uint8_t *run, DCTELEM *level, int count) {
> >> +    static const uint8_t run_score[30] = {
> >> +        0,3,3,3,3,2,2,2,2,2,2,2,2,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,0 };
> >> +    int i;
> >> +    int score = 0;
> >> +
> >> +    if(count>4)
> >> +        return 9;
> >> +    for(i=0;i<count;i++) {
> >> +        int abslevel = FFABS(level[i]);
> >> +        if(abslevel > 1)
> >> +            return 9;
> >> +        score += run_score[FFMIN(run[i],29)];
> >> +    }
> >> +    return score;
> >> +}
> > 
> > how much psnr/bitrate is gained by this? if none then please drop it
> 
> I tested on the popular foreman (300 CIF frames). When encoding at
> around 800kbps, roughly 0.1dB are gained by this. When encoding at
> around 200kbps, this increases to a gain of around 0.15dB.
> So I would like to keep it.

ok, but i think there is something wrong elsewhere if there is such a
large gain from this ... 
maybe a simple change to the bias would help? or maybe there is something
else wrong with the quantization code ...


[...]
> > 
> >> +    for(i=0;i<15;i++)
> >> +        if((ff_frame_rate_tab[i].den == frame_rate.den) &&
> >> +           (ff_frame_rate_tab[i].num == frame_rate.num))
> >> +            frame_rate_code = i;
> >> +    if(frame_rate_code < 0) {
> >> +        av_log(h->s.avctx, AV_LOG_ERROR, "unsupported framerate %d/%d\n",
> >> +               frame_rate.num, frame_rate.den);
> >> +        return -1;
> >> +    }
> > 
> > 
> > [...]
> >> +        put_bits(&s->pb,16,0);
> >> +        put_bits(&s->pb,16,CAVS_START_CODE);
> > 
> > add a put_bits_long() to bitstrea.c, document the issue with 32bits and
> > fix vorbis_enc.c :)
> 
> I looked a bit into this and I think a better way would be to fix
> put_bits() to support up to 32 bits instead of up to 31 bits.
> I am not so sure, but after a bit of checking in vorbis_enc.c it
> seems there is never any writing with more than 32 bits even
> though the local put_bits() function takes a 64bit argument.

argh, ive not noticed that vorbis is using a private put_bits
iam starting to hate vorbis_enc.c ...


> 
> Attached a new encoder patch 

> and a proposed fix for put_bits().

the fix to put_bits is rejected due to a likely huge slowdown


[...]
>  /*****************************************************************************
>   *
> + * quantization
> + *
> + ****************************************************************************/
> +
> +static void cavs_quant_c(DCTELEM *block, const uint16_t *norm, int mul,
> +                         int bias) {
> +    int i;
> +
> +    for(i=0;i<64;i++) {
> +        if(block[i] > 0)
> +            block[i] =   ((((1<<18) + block[i]*norm[i])>>19)*mul+bias)>>15;
> +        else
> +            block[i] = -(((((1<<18) - block[i]*norm[i])>>19)*mul+bias)>>15);
> +    }

this is a duplicate of dct_quantize*
its just much slower and smaller


[...]
> +        int level = abs(level_buf[coeff_num])-1;
> +        int sign = (level_buf[coeff_num]>>31)&1;

MASK_ABS()


[...]

> +    h->lambda = h->qp/3;

if i understand it correctly then qp is a logarithmic scale, if so this is
likely completely wrong

h->lambda = C*quant_mul[h->qp];
might be more correct


[...]
> +            mode = (i == 4) ? predpred : i + (i >= predpred);
> +            mode = modify(h->flags, block, mode);
> +            if(mode >= 0) {
> +                h->intra_pred_l[mode](d, top, left, h->l_stride);
> +                diff = h->s.dsp.sse[1](NULL, s, d, h->l_stride, 8);
> +                if((bitcost[i]*h->lambda + diff) < min_cost) {

with SSE its likely lambda*lambda with the above definition of lambda


[...]
> +    h->dist[0] = (h->picture.poc - h->DPB[0].poc  + 512) % 512;
> +    h->dist[1] = (h->picture.poc - h->DPB[1].poc  + 512) % 512;

&511 ?


[...]
> +    memcpy(&h->DPB[1], &h->DPB[0], sizeof(Picture));

h->DPB[1]= h->DPB[0];


[...]
> +        h->levels[0] = av_mallocz(6*64*sizeof(DCTELEM));
> +        h->runs[0]   = av_mallocz(6*64);
> +        for(i=1;i<6;i++) {
> +            h->levels[i] = h->levels[i-1] + 64;
> +            h->runs[i]   = h->runs[i-1]   + 64;
> +        }

why is this allocated with malloc instead of being part of the context?


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070726/ed810957/attachment.pgp>