[BUG][Patch] PCM shift breaks during G722 decode

[Martin Navne <martin.navne@xxxxxxxxxx>]

Hi!

We've been using PJMEDIA as part of a media server for voip calls. When
support for G722 was added between endpoints we noticed that the sound
level was much lower compared to G711a.

After some investigation we found that the clip detector in the G722
decoder kicked in immediately when the decoding started, and that it
seemed to be due to negative signal levels being fed into the clip
detector. The current implementation of the g722_dec_decoder return 
values in the range [-16384, 16383]. The way the current clip detector

is constructed it will always detect clipping on negative values because of

the bit representation of negative values.

With a small modification in how the signal is compared with the PCM 
clip mask we avoid clip detection by negative values. The  result is that 

the sound level of the decoded G722 signal is on par with G711a.

The attached patch (git format-patch) show the fix we use.

P.S.

A PCM shift of 2 has sometimes in our environment resulted in distortion, probably due to clipping, and because of this we use a PCM shift of 1.

BR,

Martin Navne

Telavox 

From 9e0ede95620fdc452b5deecdd564d5045a2d6988 Mon Sep 17 00:00:00 2001
From: Martin Navne <martin.navne@xxxxxxxxxx>
Date: Tue, 3 Sep 2019 15:05:25 +0200
Subject: [PATCH] Avoid false overflow detection when decoding G722

Since the PCM signal from the G722 decoder can have values in the range
[-32768, 32767] the current clip detection does not work as expected.

A PCM shift of 2 leads to a PCM clip mask that looks like this:

0b 1100 0000 0000 0000
Since the MSB of a negative signal value always is set this leads to
false overflow detection. The solution here is to use the absolute value
of the signal to test for clipping.

This behaviour was detected when trying to understand why the PJMEDIA
G722 decoder gave about 6 dB lower SPL compared to the G711a decoder.
---
 pjlib/include/pj/limits.h        |  3 +++
 pjmedia/src/pjmedia-codec/g722.c | 31 +++++++++++++++++++++++++------
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/pjlib/include/pj/limits.h b/pjlib/include/pj/limits.h
index 8b00ae52..748a898c 100644
--- a/pjlib/include/pj/limits.h
+++ b/pjlib/include/pj/limits.h
@@ -36,6 +36,9 @@
 /** Maximum value for unsigned 16-bit integer. */
 #define PJ_MAXUINT16	0xffff
 
+/** Minimum value for signed 16-bit integer. */
+#define PJ_MININT16	0x8000
+
 /** Maximum value for unsigned char. */
 #define PJ_MAXUINT8	0xff
 
diff --git a/pjmedia/src/pjmedia-codec/g722.c b/pjmedia/src/pjmedia-codec/g722.c
index acddc142..f815d3dd 100644
--- a/pjmedia/src/pjmedia-codec/g722.c
+++ b/pjmedia/src/pjmedia-codec/g722.c
@@ -658,17 +658,36 @@ static pj_status_t g722_codec_decode(pjmedia_codec *codec,
     if (g722_data->pcm_shift) {
 	pj_int16_t *p, *end;
 
+	/*
+	 * If e.g. pcm_shift = 2, then pcm_clip_mask = 1100 0000 0000 0000
+	 *
+	 * From this we get that the maximum absolute value to be shifted
+	 * without overflow is 0011 1111 1111 1111, i.e. ~pcm_clip_mask.
+	 */
+	const pj_int16_t max_shift_level = ~(g722_data->pcm_clip_mask);
+
 	p = (pj_int16_t*)output->buf;
 	end = p + output->size;
 	while (p < end) {
+            pj_int16_t abs_level = (*p < 0) ? -(*p) : *p;
+            /*
+             * Since -INT16_MIN == INT16_MIN (2's complement) we add an extra
+             * level of security by an explicit check.
+             */
+            if (abs_level == PJ_MININT16 || abs_level > max_shift_level) {
 #if PJMEDIA_G722_STOP_PCM_SHIFT_ON_CLIPPING
-	    /* If there is clipping, stop the PCM shifting */
-	    if (*p & g722_data->pcm_clip_mask) {
-		g722_data->pcm_shift = 0;
-		break;
-	    }
+                /* If there is clipping, stop the PCM shifting */
+                g722_data->pcm_shift = 0;
+                break;
+#else
+                /* Avoid PCM shift overflow if we don't stop on clipping */
+                abs_level = max_shift_level;
 #endif
-	    *p++ <<= g722_data->pcm_shift;
+            }
+
+	    abs_level <<= g722_data->pcm_shift;
+	    *p = (*p < 0) ? -abs_level : abs_level;
+	    p++;
 	}
     }
 
-- 
2.20.1

_______________________________________________
Visit our blog: http://blog.pjsip.org

pjsip mailing list
pjsip@xxxxxxxxxxxxxxx
http://lists.pjsip.org/mailman/listinfo/pjsip_lists.pjsip.org