[Asterisk-code-review] format/g729: Improved CNG and ptime!=20 handling. (asterisk[master])

[Jaco Kroon]

Jaco Kroon has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/15629 )


Change subject: format/g729:  Improved CNG and ptime!=20 handling.
......................................................................

format/g729:  Improved CNG and ptime!=20 handling.

In spite of asterisk passing annex=b we're still seeing a significant
amount of CNG being transmitted towards us.  Additionally, we're seeing
at least one local provider that insists on ptime=60 and another using
ptime=30.

Changes:

1.  Don't assume that a received frame % 10 isn't valid, CNG frames are
    two bytes in size, so accomodate those by discarding them.
2.  Only warn once on detecting CNG frames (per stream).
3.  Return partial valid reads, so reading 10 bytes from an input file
    is valid, since we could have an audio file of say 50ms which will
    be 50 bytes.
4.  Log invalid seeks.

This does not deal with the "all zero" frames ending up in the output
file ... resulting in beeps when transcoding these "gaps".  This is
existing behaviour.  The correct way of dealing with this is noise
generation based on CNG parameters.  Alternatively a set of "silent"
frames which we inject randomly (from say sounds/en/silence/10.g729).

Change-Id: I3c64eeeef5005c55b677d7a1fa46797c831f3429
Signed-off-by: Jaco Kroon <jaco at uls.co.za>
---
M formats/format_g729.c
1 file changed, 40 insertions(+), 11 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/29/15629/1

diff --git a/formats/format_g729.c b/formats/format_g729.c
index a6539a3..2d692ff 100644
--- a/formats/format_g729.c
+++ b/formats/format_g729.c
@@ -41,24 +41,38 @@
 
 /* Portions of the conversion code are by guido at sienanet.it */
 
-#define	BUF_SIZE	20	/* two G729 frames */
-#define	G729A_SAMPLES	160
+#define G729A_FRAME_SIZE      10
+#define G729A_FRAME_SAMPLES   80
+#define G729A_CNG_SIZE        2
+
+/* original code assumed ptime=20 (which is the norm), keep these for the
+ * moment, or two frames per packet */
+#define G729A_BUF_SIZE        (G729A_FRAME_SIZE * 2) /* two G729 frames */
+#define G729A_BUF_SAMPLES     (G729A_FRAME_SAMPLES * 2)
+
+#define G729A_SIZE2SAMPLES(b) ((b) * G729A_FRAME_SAMPLES / G729A_FRAME_SIZE)
+#define G729A_SAMPLES2SIZE(s) ((s) * G729A_FRAME_SIZE / G729A_FRAME_SAMPLES)
+
+struct ast_g729_pvt {
+	int detected_cng:1; /* simple flag indicating whether or not we detected
+						   and reported on VAD/CNG for this write stream yet */
+};
 
 static struct ast_frame *g729_read(struct ast_filestream *s, int *whennext)
 {
 	size_t res;
 
 	/* Send a frame from the file to the appropriate channel */
-	s->fr.samples = G729A_SAMPLES;
-	AST_FRAME_SET_BUFFER(&s->fr, s->buf, AST_FRIENDLY_OFFSET, BUF_SIZE);
+	AST_FRAME_SET_BUFFER(&s->fr, s->buf, AST_FRIENDLY_OFFSET, G729A_BUF_SIZE);
 	if ((res = fread(s->fr.data.ptr, 1, s->fr.datalen, s->f)) != s->fr.datalen) {
-		if (res && res != 10) /* XXX what for ? */ {
+		if (res % G729A_FRAME_SIZE) /* G729 must be multiple of G729A_FRAME_SIZE */ {
 			ast_log(LOG_WARNING, "Short read of %s data (expected %d bytes, read %zu): %s\n",
 					ast_format_get_name(s->fr.subclass.format), s->fr.datalen, res,
 					strerror(errno));
+			return NULL;
 		}
-		return NULL;
 	}
+	s->fr.samples = G729A_SIZE2SAMPLES(res);
 	*whennext = s->fr.samples;
 	return &s->fr;
 }
@@ -66,9 +80,18 @@
 static int g729_write(struct ast_filestream *fs, struct ast_frame *f)
 {
 	int res;
+	struct ast_g729_pvt* pvt = fs->_private;
 
-	if (f->datalen % 10) {
-		ast_log(LOG_WARNING, "Invalid data length, %d, should be multiple of 10\n", f->datalen);
+	if (f->datalen % G729A_FRAME_SAMPLES) {
+		if (f->datalen % G729A_FRAME_SIZE == G729A_CNG_SIZE) {
+			if (!pvt->detected_cng) {
+				ast_log(LOG_WARNING, "Probable CNG parameter update detected, this is a bug in the sender who should not be sending CNG updates - try and switch off Voice Activity Detected (VAD) or Silence Suppression on the sender.\n");
+				pvt->detected_cng = 1;
+			}
+		} else {
+			ast_log(LOG_WARNING, "Invalid data length, %d, should be multiple of 10\n", f->datalen);
+		}
+		/* can we avoid this for the CNG case?  assuming it's not a bare CNG frame */
 		return -1;
 	}
 	if ((res = fwrite(f->data.ptr, 1, f->datalen, fs->f)) != f->datalen) {
@@ -87,7 +110,12 @@
 	fseeko(fs->f, 0, SEEK_END);
 	max = ftello(fs->f);
 
-	bytes = BUF_SIZE * (sample_offset / G729A_SAMPLES);
+	if (sample_offset % G729A_FRAME_SAMPLES) {
+		ast_log(AST_LOG_ERROR, "Invalid seek, sample_offset=%ld not a multiple of frame_samples=%d.\n",
+				sample_offset, G729A_FRAME_SAMPLES);
+		// return -1; /* we should flag error, but old code did not */
+	}
+	bytes = G729A_SAMPLES2SIZE(sample_offset);
 	if (whence == SEEK_SET)
 		offset = bytes;
 	else if (whence == SEEK_CUR || whence == SEEK_FORCECUR)
@@ -124,7 +152,7 @@
 static off_t g729_tell(struct ast_filestream *fs)
 {
 	off_t offset = ftello(fs->f);
-	return (offset/BUF_SIZE)*G729A_SAMPLES;
+	return G729A_SIZE2SAMPLES(offset);
 }
 
 static struct ast_format_def g729_f = {
@@ -135,7 +163,8 @@
 	.trunc = g729_trunc,
 	.tell = g729_tell,
 	.read = g729_read,
-	.buf_size = BUF_SIZE + AST_FRIENDLY_OFFSET,
+	.buf_size = G729A_BUF_SIZE + AST_FRIENDLY_OFFSET,
+	.desc_size = sizeof(struct ast_g729_pvt),
 };
 
 static int load_module(void)

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/15629
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I3c64eeeef5005c55b677d7a1fa46797c831f3429
Gerrit-Change-Number: 15629
Gerrit-PatchSet: 1
Gerrit-Owner: Jaco Kroon <jaco at uls.co.za>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210315/a611ad42/attachment.html>