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

[Michael Niedermayer]

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.
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.


> 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.


> 
> 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

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

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

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090627/fa12dc5c/attachment.pgp>