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

[Michael Niedermayer]

On Sun, Jun 28, 2009 at 02:01:54AM +0700, Vladimir Voroshilov wrote:
> 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.

so you plan to implement a decoder that is bitidentical to 2 decoders that
are not bitidentical to each other?
theres a float and a fixed point decoder ...


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

decoder development generally involves the developer working on the decoder
to perform whichever tests are neccessary to ensure that the changes are
correct. That may sometimes match the regression test, though as you note
we have no encoder so no reg test. We have many other decoders that lack
reg tests still they are being tested during development


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

ffmpegs native de and encoders are supposed to share code where possibe and
there are things that are similar to g729. Now if bitexactness is required
thats fine but otherwise its a constraint thats not going to help readability.
Now considering that there is a reference float implementation that is not
bitidentical to the fixed point it does not seem the spec requires it.

To write a decoder for ffmpeg, the author needs to understand the algorithms
used in it at least to some extend. Writing a decoder for ffmpeg is about
implementing the stuff writen in a spec cleanly&efficiently, its not about
writing a 1:1 translation of another implementation. Iam not particularely
fond of the NIH syndrome, we can just use the reference in that case.

And id like to clarify that "its not that bad" style comments are not an
excuse or argument. We arent littering our code by x++; x--; because its
not that bad at that point.
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"

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

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
-------------- 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/20090628/23b502c7/attachment.pgp>