[Asterisk-code-review] feat: AudioSocket channel and application (...asterisk[master])
Seán C. McCord
asteriskteam at digium.com
Fri Aug 23 20:49:10 CDT 2019
Seán C. McCord has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/11579 )
Change subject: feat: AudioSocket channel and application
......................................................................
Patch Set 12:
(21 comments)
> Patch Set 12: Code-Review-1
>
> (1 comment)
>
> Is UUID absolutely required? Could you just use the unique channel id, whether auto-generated or provided in an ARI call? If so, then chan_audiosocket could easily plug into the new externalMedia thing with encapsulation "audiosocket" and transport "tcp". with no other changes to your code.
By the current protocol specification, it is a hard requirement, but I an not averse modifying that spec. UUID is nice because it is a fixed-length binary ID, but beyond that, it is completely arbitrary.
I'll see what will need to be changed for this and how disruptive it might be.
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
Done
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.
Done
https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@85
PS10, Line 85: );
> Indentation
Done
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?
Done
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.
Done
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?
Done
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
Done
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?
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?
I don't understand. I'm freeing it in both the error and non-error conditions. Are you suggesting that the frame should _not_ be freed? Is that because I subsequently reuse the variable for the receive-from-audiosocket and thus, it is redundant? I was assuming there may be inner structs which ast_frfree() would properly dispose of which may not be, should I just reuse the pointer.
https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@183
PS10, Line 183: .support_level = AST_MODULE_SUPPORT_CORE,
> EXTENDED
Done
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
Done
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
I _think_ this addresses your concern (added NULL set to failure label), but I don't see any condition in which that would have happened. If there some other error-related callback I should be setting on the module to handle this?
https://gerrit.asterisk.org/#/c/11579/10/channels/chan_audiosocket.c@260
PS10, Line 260: .support_level = AST_MODULE_SUPPORT_CORE,
> EXTENDED
Done
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
Done
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?
Pardon my ignorance, but I would assume that would depend on the caller, no? If it is processed internally, it would be freed. If it is passed on down through a channel, it would be handled by whatever handler is down the line as per usual.
I have added a note in the parameter suggesting that freeing the frame is the responsibility of the caller. Is this your intention?
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
Done
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/ […]
Nice; I didn't know about autoservice.
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
Done
https://gerrit.asterisk.org/#/c/11579/10/res/res_audiosocket.c@174
PS10, Line 174: usleep(100);
> Why this usleep?
Done
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.
Done
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 […]
Done
--
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: 12
Gerrit-Owner: Seán C. McCord <ulexus at gmail.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
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: Sat, 24 Aug 2019 01:49:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Joshua Colp <jcolp at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190823/a954563f/attachment.html>
More information about the asterisk-code-review
mailing list