[FFmpeg-devel] [PATCH] G.729 Frame erasure support for fixed-codebook vector decoding

[Vladimir Voroshilov]

2009/6/29 Michael Niedermayer <michaelni at gmx.at>:
> On Sun, Jun 28, 2009 at 11:34:51AM +0700, Vladimir Voroshilov wrote:
>> 2009/6/28 Michael Niedermayer <michaelni at gmx.at>:
>> > On Sun, Jun 28, 2009 at 02:01:54AM +0700, Vladimir Voroshilov wrote:
> [...]
>> > Also to say it in case i didnt yet, iam not arguing for any specific thing
>> > or even for ignooring bitexactness i just think the arguments you use to
>> > make design decissions are the wrong ones. I mean like
>> > "lets keep it bitexact because its easier" vs. "ive tested your suggestions
>> > and they sound worse"
>>
>> Nope. "it easy to track introduced bugs at this stage".
>> Also I don't understand what should i treat as "worse".
>> Breaking bitexactnes is worse for me but not for you.
>>
>> I'm testing each of your suggestion and trying to argue ?why i don't like it,
>> instead of simple "it is worse" (at least i'm sure i do). I know that you
>> never asks to change code without a reason.
>>
>> And as i already said, hearing the result is unreliable test for me.
>> I'm not musician with perfect hearing. I'm not able to find exact
>> place in code containing
>> a bug after spotting one extra "click" in decoded sound.
>
> if you hear a click you know there is a bug so you could go back to the
> previous revission where you did not hear the click. Its a simple thing,
> test each with ears and a PSNR tool and problems should be spoted quickly
>
>
>>
>> P.S. looks like discussing patch converts into long flaming :(
>
> yes
>
>
>> I'm sorry for this.
>>
>> Returing to the first you comment:
>>
>>
>> >> @@ -224,6 +225,9 @@ static av_cold int decoder_init(AVCodecContext * avctx)
>> >> ? ? ?ctx->lsp[1] = ctx->lsp_buf[1];
>> >> ? ? ?memcpy(ctx->lsp[0], lsp_init, 10 * sizeof(int16_t));
>> >>
>> >> + ? ?/* random seed initialization */
>> >> + ? ?ctx->rand_value = 21845;
>>
>> >the word pointless comes to mind, at least if this is just used for
>> >frame erasure handling
>>
>> >> +
>> >> ? ? ?return 0;
>> >> ?}
>> >>
>> >> @@ -336,6 +340,15 @@ static int decode_frame(AVCodecContext *avctx, void *data, int *data_size,
>> >> ? ? ? ? ?/* Round pitch delay to nearest (used everywhere except ff_acelp_interpolate). */
>> >> ? ? ? ? ?pitch_delay_int ?= (pitch_delay_3x + 1) / 3;
>> >>
>> >> + ? ? ? ?if (frame_erasure) {
>> >> + ? ? ? ? ? ?ctx->rand_value = g729_prng(ctx->rand_value);
>> >> + ? ? ? ? ? ?fc_indexes ? = ctx->rand_value & ((1 << format.fc_indexes_bits) - 1);
>> >> +
>> >> + ? ? ? ? ? ?ctx->rand_value = g729_prng(ctx->rand_value);
>> >> + ? ? ? ? ? ?pulses_signs = ctx->rand_value & ((1 << format.fc_signs_bits) - 1);
>> >> + ? ? ? ?}
>>
>> > a single call to the prng is probably enough also is the & really needed?
>>
>> As i said before, seed initialization as long as double call to
>> g729_prng and "&" are defined
>> in main body of G.729, part 4.4.4 "Generation of the replacement
>> excitation". Is this
>> enough reason ?to keep them ?
>
> no, its illogic for the handling of damaged frames to require specific bits
> of a specific PRNG to be used
> that said, are the & needed or not?

Quote from there: "The fixed-codebook index is derived from the 13 least
significant bits of the next random number. The fixed-codebook sign is
derived from the 4 least
significant bits of the next random number. "

fc_indexes_bits == 13 for G.729 @8k
fc_signs_bits == 4  for G.729 @8k

code inside ff_acelp_fc_pulse_per_track is like (mask == 0x7 here and
loop is called three times)

    for(){
     ...  tab1[pulse_indexes & mask] ..
          pulse_sindexes >> mask
    }
    ...tab2[pulse_indexes] ...

The latter cause index-out-of-bounds error if entire pulse_indexes are
not under "& 13".
Note also that number of remaning bits at last line is different  for
@8k and @6.4k

Thus either additional parameter required to above routine (number of
remaining bits) or keeping "&" outside.

P.S. "&" applied to pulse_signs can be safely removed, since extra
bits are just ignored.

-- 
Regards,
Vladimir Voroshilov     mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719