[Asterisk-code-review] feat: AudioSocket channel and application (...asterisk[master])
Joshua Colp
asteriskteam at digium.com
Tue Aug 20 07:40:32 CDT 2019
Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/11579 )
Change subject: feat: AudioSocket channel and application
......................................................................
Patch Set 10: Code-Review-1
(21 comments)
https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c
File apps/app_audiosocket.c:
https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@30
PS10, Line 30: <support_level>core</support_level>
This should be extended
https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@75
PS10, Line 75: static int audiosocket_exec(struct ast_channel *chan, const char *data);
This doesn't need to be forward declared.
https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@85
PS10, Line 85: );
Indentation
https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@102
PS10, Line 102: ast_log(LOG_ERROR, "failed to connect to AudioSocket\n");
Should this cause the application to exit?
https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@113
PS10, Line 113:
: if (ast_set_write_format(chan, ast_format_slin)) {
: ast_log(LOG_ERROR, "Failed to set write format to SLINEAR\n");
: return 1;
: }
: if (ast_set_read_format(chan, ast_format_slin)) {
: ast_log(LOG_ERROR, "Failed to set read format to SLINEAR\n");
: return 1;
: }
The original write and read formats should be stored and then restored when this is completed.
https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@131
PS10, Line 131: if (ast_channel_state(chan) != AST_STATE_UP) {
: return 0;
: }
Should this be done when this function is entered?
https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@137
PS10, Line 137: ast_log(LOG_WARNING, "No frame received\n");
This will occur under normal conditions if the channel is hung up
https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@141
PS10, Line 141: f->delivery.tv_sec = 0;
: f->delivery.tv_usec = 0;
Why is this done?
https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@156
PS10, Line 156: if (!(f = ast_audiosocket_receive_frame(svc))) {
Should this frame be freed?
https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@183
PS10, Line 183: .support_level = AST_MODULE_SUPPORT_CORE,
EXTENDED
https://gerrit.asterisk.org/#/c/11579/10/channels/chan_audiosocket.c
File channels/chan_audiosocket.c:
https://gerrit.asterisk.org/#/c/11579/10/channels/chan_audiosocket.c@30
PS10, Line 30: <support_level>core</support_level>
extended
https://gerrit.asterisk.org/#/c/11579/10/channels/chan_audiosocket.c@136
PS10, Line 136: struct audiosocket_instance *instance;
In certain off-nominal error cases this can end up getting leaked
https://gerrit.asterisk.org/#/c/11579/10/channels/chan_audiosocket.c@260
PS10, Line 260: .support_level = AST_MODULE_SUPPORT_CORE,
EXTENDED
https://gerrit.asterisk.org/#/c/11579/10/include/asterisk/res_audiosocket.h
File include/asterisk/res_audiosocket.h:
https://gerrit.asterisk.org/#/c/11579/10/include/asterisk/res_audiosocket.h@74
PS10, Line 74: * This returned object is an ao2 reference counted object.
: *
: * Any attribute in the returned \ref hepv3_capture_info that is a
: * pointer should point to something that is allocated on the heap,
: * as it will be free'd when the \ref hepv3_capture_info object is
: * reclaimed.
: *
: * \param payload The payload to send to the HEP capture node
: * \param len Length of \ref payload
This isn't hep
https://gerrit.asterisk.org/#/c/11579/10/include/asterisk/res_audiosocket.h@84
PS10, Line 84: * \retval A \ref ast_frame on success
What should be done with the returned frame? Does it need ast_frfree called on it?
https://gerrit.asterisk.org/#/c/11579/10/res/res_audiosocket.c
File res/res_audiosocket.c:
https://gerrit.asterisk.org/#/c/11579/10/res/res_audiosocket.c@28
PS10, Line 28: <support_level>core</support_level>
extended
https://gerrit.asterisk.org/#/c/11579/10/res/res_audiosocket.c@55
PS10, Line 55:
If this is called from a channel thread and either resolution or connection takes awhile to succeed/fail, data can then pile up on the underlying source that is feeding the channel (such as the RTP socket). It's probably wise to put the channel into autoservice before calling this so that doesn't occur.
https://gerrit.asterisk.org/#/c/11579/10/res/res_audiosocket.c@106
PS10, Line 106: * \brief Handle the connection that was started by launch_netscript.
Doesn't match how this is used
https://gerrit.asterisk.org/#/c/11579/10/res/res_audiosocket.c@174
PS10, Line 174: usleep(100);
Why this usleep?
https://gerrit.asterisk.org/#/c/11579/10/res/res_audiosocket.c@196
PS10, Line 196: if (!(buf = ast_malloc(3 + f->datalen))) {
I don't believe you need to malloc this. I'd expect the stack would be fine.
https://gerrit.asterisk.org/#/c/11579/10/res/res_audiosocket.c@307
PS10, Line 307: return &f;
Can't this be called from multiple threads at the same time, thus resulting in this same frame being used by each since it is static?
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/11579
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Ie866e6c4fa13178ec76f2a6971ad3590a3a588b5
Gerrit-Change-Number: 11579
Gerrit-PatchSet: 10
Gerrit-Owner: Seán C. McCord <ulexus at gmail.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
Gerrit-Reviewer: Seán C. McCord <ulexus at gmail.com>
Gerrit-Comment-Date: Tue, 20 Aug 2019 12:40:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190820/7f1ccaad/attachment.html>
More information about the asterisk-code-review
mailing list