[FFmpeg-devel] [PATCH] Chinese AVS encoder

[Stefan Gehrer]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi

Michael Niedermayer 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 ...
>>>
>> why? It seems clear to me that coding a macroblock as skipped is so much
>> cheaper than coding type/cbp/mv/coefficients that it might be worth
>> throwing some real information away. By real I mean coefficients that
>> come out of a fully correct quantization code, though I am not claiming
>> my code to be that.
> 
> you should just make RD optimal decissions (IIRC you do calculate the RD all
> over the place anyway), if you do no heuristic like above will help

Okay, I think I see what you mean. Currently I use RD-decisions to
select intra prediction modes and for the motion estimation, but for
the macroblock mode decision there are some heuristics roughly like
this:

if(only insignificant coefficients)
  encode mb as skipped
else
  RD = P motion estimation
  if(RD < threshold)
    encode mb as p
  else
    encode mb as intra

By using RD optimal decisions do you mean I should encode every
macroblock in all three modes (skipped/p/intra) and select the
best of three, or rather I should try one mode and if its RD
cost is below a certain threshold I should take it, otherwise
I should move on to the next mode?

> 
>>> [...]
>>>>>> +    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
>>>
>> It is in the else branch and only executed every 32 bits of bitstream
>> writing, so I don't think the slowdown will be "huge". But if it's
>> not acceptable I will think about adding a put_bits_long again.
> 
> the slowdown would affect all codecs, and i think it would be a significant
> slowdown
> its totally unacceptable just so you can replace
> put_bits(16); put_bits(16);
> by
> put_bits(32)
> in 1 or 2 codecs
> and without any benchmarks ...
> 

So the problem is:
h263.c has put_bits_(16);put_bits(16);
cavsenc.c has put_bits_(16);put_bits(16);
vorbis_enc.c has it's own put_bits implementation

Solutions:
keep as is (you complained)
fix as I proposed (too slow)
create a proper put_bits_long function (seems overkill)

what about a silly and untested
void put_bits_long(PutBitContext *s, int n, uint64_t value) {
  int rem = n>>1;
  put_bits(s,n-rem,value>>rem);
  put_bits(s,rem,value&((1<<rem)-1));
}
to encode 0..62 bits ?

> 
> [...]
>>> [...]
>>>> +    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 ?
>>>
>> It is not speed critical, and then I find the modulo more readable.
> 
> its +% vs &, ( 2 vs. 1 operations) so the modulo likely does make the
> binary by a byte or 2 larger ...
> 
> 
> [...]
>>> [...]
>>>> +        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?
>> It keeps the context smaller when only the decoder is running.
> 
> hmm, ok, still its not ideal, as this requires an extra pointer
> dereference to access

Ok, I will change this in the next round.

Stefan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGqQVHYpYOlpT3vNMRAsF+AKCXSeEzTDNKssm3PkHFHsVQpzXgJwCg5nI2
F7ydbEy1vt/XaBQjvA34GKs=
=4EuI
-----END PGP SIGNATURE-----