[FFmpeg-devel] [PATCH] Frame erasure case of pitch delay decoding

Sun Jun 28 22:27:10 CEST 2009

On Sun, Jun 28, 2009 at 03:11:31PM +0700, Vladimir Voroshilov wrote:
> 2009/6/28 Vladimir Voroshilov <voroshil at gmail.com>:
> > 2009/6/28 Michael Niedermayer <michaelni at gmx.at>:
> >> On Sat, Jun 27, 2009 at 09:54:06AM +0700, Vladimir Voroshilov wrote:
> >>> 2009/6/27 Michael Niedermayer <michaelni at gmx.at>:
> >>> > On Sat, Jun 27, 2009 at 01:49:49AM +0700, Vladimir Voroshilov wrote:
> >>> >> Updated patch.
> >>> >>
> >>> >>
> >>> >> --
> >>> >> Regards,
> >>> >> Vladimir Voroshilov ? ? mailto:voroshil at gmail.com
> >>> >> JID: voroshil at gmail.com, voroshil at jabber.ru
> >>> >> ICQ: 95587719
> >>> >
> >>> >> ?g729dec.c | ? ?7 ++++++-
> >>> >> ?1 file changed, 6 insertions(+), 1 deletion(-)
> >>> >> c66e04bc9abc50854d2bd35ac5299ae0e4b28637 ?0005-Frame-erasure-support-for-pitch-delay-decoding.176.patch
> >>> >> From aa4b23873bc3c5e103a5756ade017558bda12961 Mon Sep 17 00:00:00 2001
> >>> >> From: Vladimir Voroshilov <voroshil at gmail.com>
> >>> >> Date: Thu, 11 Jun 2009 13:51:28 +0700
> >>> >> Subject: [PATCH 05/25] Frame erasure support for pitch delay decoding
> >>> >>
> >>> >>
> >>> >> diff --git ffmpeg-r19281/libavcodec/g729dec.c ffmpeg-r19281_v176/libavcodec/g729dec.c
> >>> >> index b937235..f193123 100644
> >>> >> --- ffmpeg-r19281/libavcodec/g729dec.c
> >>> >> +++ ffmpeg-r19281_v176/libavcodec/g729dec.c
> >>> >> @@ -306,7 +306,9 @@ static int decode_frame(AVCodecContext *avctx, void *data, int *data_size,
> >>> >> ? ? ? ? ?gc_1st_index ?= get_bits(&gb, format.gc_1st_index_bits);
> >>> >> ? ? ? ? ?gc_2nd_index ?= get_bits(&gb, format.gc_2nd_index_bits);
> >>> >>
> >>> >> - ? ? ? ?if(!i) {
> >>> >> + ? ? ? ?if (frame_erasure)
> >>> >> + ? ? ? ? ? ?pitch_delay_3x ? = 3 * ctx->pitch_delay_int_prev;
> >>> >> + ? ? ? ?else if(!i) {
> >>> >> ? ? ? ? ? ? ?if (bad_pitch)
> >>> >> ? ? ? ? ? ? ? ? ?pitch_delay_3x ? = 3 * ctx->pitch_delay_int_prev;
> >>> >
> >>> > if(!i || frame_erasure)
> >>> >
> >>> > also, i think bad_pitch and frame_erasure are redundant that is the same
> >>> > either your frame is damaged or not
> >>> > If you disagree, id like to hear which real life system uses g729 and
> >>> > can suffer from single bit errors not passing through a FEC code
> >>> > (that stuff might exist but my gut feeling says it does not, so unless
> >>> > ?you have an example i prefer the simpler code)
> >>>
> >>> Afaik, bad_pitch is rsult of checking most significant part of input
> >>> parameters: vector of quantizer
> >>> along with switched MA predictor. Getting garbage here will cause
> >>> large quality degradaion.
> >>> On the other side remaining parts will bring far less noise.
> >>> I can guess that having something data in frame (even with bad vector
> >>> of quantizer) from the standpoint of G.279 authors is still better
> >>> than entire frame erasure.
> >>
> >> you keep implicitly assuming that single bit errors happen, so let me
> >> ask again, do you know of such a case? maybe its a common thing in some
> >> scenario but if not a parity error in any part of the frame means that
> >> the rest of the frame will likely be random as well
> >
> > Well spec says (4.1.6 Computation of the parity bit):
> > "If this bit is not identical to transmitted parity bit, it is more
> > likely that bit
> > _errorS_ occured during transmission." And then spec describes how to
> > compute pitch delay of the first subframe
> > in this case (it is also clearly says that pitch ?delay for second
> > subframe is calculated as in common case).
> > This is the _only_ place when parity bit usage is described. Thus
> > bad_pitch should affect only pitch_delay.
> >
> > On the other side spec does not say what to do with pitch delay if
> > frame erasure is detected.
> > Reference code uses the same computation (as for bad parity bit) for
> > both first and second subframes.
> > This is the only way to avoid usage of adaptive-codebook index in the
> > second subframe.
> >
> >> did you run any tests of burst error, damaged sectors/packets in terms
> >> of subjective quality for the different variants? has ITU performed
> >> such tests? and if so are they documented somewhere?
> >
> > I did only tests with random frame_erasure flag. I didn't remember
> > exact results.
> > If i'm not wrong, most sample remains understandable with 50% frames erased
> > and even more for some samples.
> >
> > I didn't ?test bad_pitch, except ?in "pitch" ITU test, which
> > definitely tests only specific parts
> > of code and nothing more.
> >
> > And i don't know any ITU test results you saying about.

summary, you didnt test it, and you dont know if ITU did nor what the
reasoning is behind doing it like that. The spec contains it but we
dont know if its normative or just a suggestion on how to handle errors

sorry thats too little, i cannot approve an implementation of this part
unless its better understood.
The understanding is needed to decide which way it should be handled.
Also i feel its a little annying to skip 2 min testing and rather try to
beat your view through basically repeating that you dont know at all
why it would be better.

> >
> >>> If you can see in case of frame erasure pitch delay is computed
> >>> differently for both subframes, while bad_pitch
> >>> keeps computing of pitch_delay for second subframe the same.
> >>>
> >>> And here we again comes to the question about bitexactness.
> >>> If it is possible i prefer creating decoder which passes all ITU
> >>> tests, and ONLY after that
> >>> break it (decoder) by introducing desired optimizations.
> >>
> >> iam not stoping you from creating such decoder
> >> but for ffmpeg i like to have an independant, optimal and conformant
> >> implementation of the spec, if that includes bitexactness depends on
> >> the spec requireing it and there being any advantage in breaking such
> >> bitexactness to one of many implementations
> >
> > Hmm. About test vectors:
> > ====
> > This directory contains testvectors to validate the correct execution
> >
> > of the G.729 ANSI-C software (version 3.3). NOTE that these vectors
> >
> > are not part of a validation procedure. It is very difficult to design
> >
> > an exhaustive set of test vectors. Hence passing these vectors should
> >
> > be viewed as a minimum requirement, and is not a guarantee that the
> >
> > implementation is correct for every possible input signal.
> >
> >
> > [snip]
> >
> > The testvectors were designed to provide as much coverage as possible
> >
> > in terms of parameters and algorithm. Below we indicate what parts of
> >
> > the algorithms are excercised. Note that none of these sequences
> >
> > provides an exhaustive coverage.
> >
> > ===
> >
> > "not part of a validation procedure", "should be viewed as minimum requirement"
> > I'm in doubt again, whether passing test vector required or not for independent
> > implementations.
> 
> OMG. Just find another thing. In Annex D (6.4k mode) D.7 paragraph also says:
> The ANSI C code
> represents the normative specification of this annex. The algorithmic
> description given by the C
> code shall take precedence over the texts contained in the main body
> of G.729 and in Annex D.
> 
> Thus, when writing 6.4k decoder i should follow C code rather than specification
> when they are saying different things, should i ?

Id by happy if you followed any algorithmic description instead of following
the implementation statement by statement while making a huge effort not to
think about the algorithm behind.

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

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- 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/15837b25/attachment.pgp>