[Asterisk-code-review] app_voicemail.c: Support multiple file formats for forwarded messages. (...asterisk[13])

[cmaj]

cmaj has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/12981 )

Change subject: app_voicemail.c: Support multiple file formats for forwarded messages.
......................................................................


Patch Set 1:

(4 comments)

> Patch Set 1:
> 
> (1 comment)

Trying to add new patch set...

https://gerrit.asterisk.org/#/c/12981/1/apps/app_voicemail.c 
File apps/app_voicemail.c:

https://gerrit.asterisk.org/#/c/12981/1/apps/app_voicemail.c@5511 
PS1, Line 5511: fopen
> There is a utility function called ast_file_is_readable which might be better
Using ast_file_is_readable in next patch, thanks!


https://gerrit.asterisk.org/#/c/12981/1/apps/app_voicemail.c@5514 
PS1, Line 5514: 		if (c)
              : 			*c = '\0';
> Always use braces with an "if".
Done


https://gerrit.asterisk.org/#/c/12981/1/apps/app_voicemail.c@5518 
PS1, Line 5518: fopen
> Same. […]
Done


https://gerrit.asterisk.org/#/c/12981/1/apps/app_voicemail.c@5558 
PS1, Line 5558: 			} else {
              : 				if (!strcasecmp(format, "wav")) {
              : 					if (vmu->volgain < -.001 || vmu->volgain > .001) {
              : 						res = snprintf(sox_gain_cmd, sizeof(sox_gain_cmd), "sox -v %.4f %s.%s -e signed-integer -b 16 %s",
              : 									   vmu->volgain, attach, altformat, fname);
              : 					} else {
              : 						res = snprintf(sox_gain_cmd, sizeof(sox_gain_cmd), "sox %s.%s -e signed-integer -b 16 %s",
              : 									   attach, altformat, fname);
              : 					}
              : 				} else {
              : 					if (vmu->volgain < -.001 || vmu->volgain > .001) {
              : 						res = snprintf(sox_gain_cmd, sizeof(sox_gain_cmd), "sox -v %.4f %s.%s %s",
              : 									   vmu->volgain, attach, altformat, fname);
              : 					} else {
              : 						res = snprintf(sox_gain_cmd, sizeof(sox_gain_cmd), "sox %s.%s %s",
              : 									   attach, altformat, fname);
              : 	
> Can you explain why we need this logic?  What was it about the original sox command that no longer w […]
I found in testing that newer sox is not able to convert between these particular formats without additional parameters (from gsm to wav.) In the future, additional email attachment formats may need to be added, at which time a switch/case may be more desirable vs increased if/then complexity.



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

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Change-Id: I7321e7f7e7c58adbf41dd4fd7191c887b9b2eafd
Gerrit-Change-Number: 12981
Gerrit-PatchSet: 1
Gerrit-Owner: cmaj <chris at penguinpbx.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: cmaj <chris at penguinpbx.com>
Gerrit-CC: Sean Bright <sean.bright at gmail.com>
Gerrit-Comment-Date: Mon, 14 Oct 2019 21:31:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: George Joseph <gjoseph at digium.com>
Comment-In-Reply-To: Sean Bright <sean.bright at gmail.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20191014/a10e4388/attachment-0001.html>