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

[Michael Niedermayer]

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?

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

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- 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/9c438105/attachment.pgp>