PCM shift issue in the G722 decoder

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

- if (*p & g722_data->pcm_clip_mask) {

+ const pj_int16_t max_shift_level = ~(g722_data->pcm_clip_mask);
+ if (*p < g722_data->pcm_clip_mask || *p > max_shift_level) {

Has anyone experienced a similar issue?

The following changes is what we are using to fix this issue, and you
are free to do what you want with it; discard, use, rewrite. :)

(The reason for using the absolute value of the signal is to avoid
undefined behavior by left shifting negative values.)

BR,
Martin Navne

diff --git a/pjmedia/src/pjmedia-codec/g722.c b/pjmedia/src/pjmedia-codec/g722.c
index acddc142..c7d3d6a3 100644
--- a/pjmedia/src/pjmedia-codec/g722.c
+++ b/pjmedia/src/pjmedia-codec/g722.c
@@ -656,19 +656,38 @@ static pj_status_t g722_codec_decode(pjmedia_codec *codec,
 
     /* Adjust input signal level from 14-bit to 16-bit */
     if (g722_data->pcm_shift) {
-     pj_int16_t *p, *end;
+     pj_int16_t *p, *end, abs_level;
+
+     /*
+      * If 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) {
+            abs_level = (*p < 0) ? -(*p) : *p;
+            if (abs_level > max_shift_level) {
+                /* Avoid PCM shift overflow if we don't stop on clipping */
+                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 */
+                if (*p < g722_data->pcm_clip_mask || *p > max_shift_level) {
+                    g722_data->pcm_shift = 0;
+                    break;
+                }
 #endif
-         *p++ <<= g722_data->pcm_shift;
+            }
+
+         abs_level <<= g722_data->pcm_shift;
+         *p++ = (*p < 0) ? -abs_level : abs_level;
  }
     }

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

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