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

[Vladimir Voroshilov]

2009/6/28 Michael Niedermayer <michaelni at gmx.at>:
> On Sat, Jun 27, 2009 at 09:43:27AM +0700, Vladimir Voroshilov wrote:
>> 2009/6/27 Michael Niedermayer <michaelni at gmx.at>:
>> > On Sat, Jun 27, 2009 at 02:31:40AM +0700, Vladimir Voroshilov wrote:
>> >> subj
>> > [...]
>> >> @@ -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?
>>
>> Well. Then i have to ask (you and other devs): "Does FFmpeg is
>> interested in having G.729 decoder
>> binary exact with reference one" (this means, capable of passing all
>> ITU's tests) .
>
> we need a decoder that conforms to all mandatory parts of the spec.
> specs generally contain mandatory and non mandatory parts
> and tests that are designed to test a single specific implementation instead
> of testing the conformance to a spec, that is tests that say, well on
> x86 it returned that so it should on other platforms are not usefull to
> us.

ITU's code works not only on x86.
I'm not sure that understand this long sentence correctly, though.

> It would be silly to be limited by more than what one MUST follow or SHOULD
> follow, simply copy and pasting the ITU implementation is not my goal.

1. i didn't find any separation to mandatory and non mandatory parts
in G.729 spec.
In this case i prefer treating all spec as mandatory.

2. If i'm not wrong, frame erasure parts will affect future good
frames. Thus, following ITU
spec for erasure case looks enough reasonable for me.

>> Currently this is still possible to keep it such.
>>
>
>> Or "just decode speech in any way, no mater what we get, if result is
>> understandable" is enough for you ?
>
> this seems a confused statement
> -> conform to the spec & have a clean & efficient & simple implementation
>
> -> be binary identical to a specific implementation for cases outside the
> ? spec, make little sense to me.

In other word if spec clearly defines something, decoder should follow it.
In all other cases we free to select any solution. Am i right?

Note that this part (presudo-random number generator usage) is cleanly
defined in spec. Formula itself, initialization value, first and second call
to generator (and how many bits will be got wron each call).

>> Replacing pseudo-random generator with FFmpeg's internal one or
>> merging above two lines, definitely breaks
>> passing "erasure" ITU test.
>
> which AFAIK is a test for ITUs own implementation not the ITU spec, but
> maybe i missremember, in which case please correct me ...
>
>
>>
>> Note also, that if i break bitexactness, i'll never have enough good
>> test for checking introduced bugs.
>

> it seems we can test other decoders too without bitexactness, i assume
> you have ears and can hear if things are equal or not, besides bitexactness
> stuff could maybe be kept under #ifdef

Internal "make test" uses sample->encoder->decoder->sample2 process.
Which is not applicapable without encoder. And we can't hope that
"sample" will not differ "sample2"  if decoder does not follow some
parts of spec.

>
> the only question is if the g729 spec requires bitexact handling of damaged
> frames or not. if not theres no need for us to follow the g729 reference
> implementation

As i already said, i didn't find any separation to required and
non-required parts.
And i prefer to keep bitexactness everywhere where possible and if this is not
significantly hurt performance and code readability.

P.S. If i'm not wrong we are currently fighting for two
multiplications per 10ms and one extra
assignment in decoder initialization. Other occurance of frame erasure code is
more comples, though.

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