[Asterisk-code-review] feat: AudioSocket channel and application (...asterisk[master])
Seán C. McCord
asteriskteam at digium.com
Sun Sep 22 13:48:32 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 13:
(18 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.
Would it be sufficient to use something like UUIDv5, which creates a UUID from the SHA-1 hash of a namespace and name? That could be deterministic so long as there is an agreed namespace UUID (arbitrary generated UUID for "Asterisk" perhaps?).
This would supply a means by which we could use the unique channel ID but still conform to the existing AudioSocket protocol specification.
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>
> Done
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);
> Done
Done
https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@85
PS10, Line 85: );
> Done
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");
> Done
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;
: }
> Done
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;
: }
> Done
Done
https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@137
PS10, Line 137: ast_log(LOG_WARNING, "No frame received\n");
> Done
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;
> Done
Done
https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@156
PS10, Line 156: if (!(f = ast_audiosocket_receive_frame(svc))) {
> Within this function you are not freeing the frame returned by ast_audiosocket_receive_frame. […]
Done
https://gerrit.asterisk.org/#/c/11579/10/apps/app_audiosocket.c@183
PS10, Line 183: .support_level = AST_MODULE_SUPPORT_CORE,
> Done
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@136
PS10, Line 136: struct audiosocket_instance *instance;
> At line 172 you allocate this structure. […]
Done
https://gerrit.asterisk.org/#/c/11579/10/channels/chan_audiosocket.c@260
PS10, Line 260: .support_level = AST_MODULE_SUPPORT_CORE,
> Done
Done
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@55
PS10, Line 55:
> Nice; I didn't know about autoservice.
Done
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.
> Done
Done
https://gerrit.asterisk.org/#/c/11579/10/res/res_audiosocket.c@174
PS10, Line 174: usleep(100);
> Done
Leftover from debugging
https://gerrit.asterisk.org/#/c/11579/10/res/res_audiosocket.c@196
PS10, Line 196: if (!(buf = ast_malloc(3 + f->datalen))) {
> Done
Whatever solution I had to this before was lost, but it seems like variable length stack-allocated arrays are a contentious subject. Is there another way which I am missing?
https://gerrit.asterisk.org/#/c/11579/10/res/res_audiosocket.c@307
PS10, Line 307: return &f;
> Done
Done
https://gerrit.asterisk.org/#/c/11579/12/res/res_audiosocket.c
File res/res_audiosocket.c:
https://gerrit.asterisk.org/#/c/11579/12/res/res_audiosocket.c@62
PS12, Line 62: ast_sockaddr_resolve(&addrs, server, PARSE_PORT_REQUIRE, AST_AF_UNSPEC))) {
> You may want to use the new ast_dns_resolve_ipv6_and_ipv4 API I just created. […]
The only problem with ast_dns_resolve_ipv6_and_ipv4 is that it only returns the first AAAA or A record. I need a way to walk through all the options so that if one host is unavailable, I can failover to the next in the DNS set. This is necessary if I have no loadbalancer in place.
--
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: 13
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: Sun, 22 Sep 2019 18:48:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: George Joseph <gjoseph at digium.com>
Comment-In-Reply-To: Seán C. McCord <ulexus at gmail.com>
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/20190922/41d16c98/attachment.html>
More information about the asterisk-code-review
mailing list