Re: [asterisk-dev] Call unhold/topology change indication order
On Wed, May 11, 2022 at 9:54 AM Joshua C. Colp wrote: > On Wed, May 11, 2022 at 11:50 AM Fridrich Maximilian < > m.fridr...@commend.com> wrote: > >> > You're in off-nominal untested un thought of territory, so the code >> behavior >> > probably reflects that. Specifically having both audio and video, and >> doing >> > hold/unhold. Audio hold is special and separate from stream negotiation, >> > while video isn't, so things probably don't handle that. >> >> Considering that, I would like to have a discussion about considering >> reverting >> ASTERISK-28783 [1]. This change showed that the handling of non-default >> streams >> is not mature enough yet to be used in production environments. >> Specifically, >> one-way streams and hold/unhold are not handled correctly. In order to >> keep >> Asterisk reliably usable for many different use-cases (Swiss Army Knife), >> I >> would suggest reverting this change for now until the handling of >> non-default >> streams is mature enough to handle more than just the most basic >> use-cases. >> >> Please let me know what you think about this suggestion. >> > > Oh, that's my change from long ago. That change has existed in the code > base for quite a long time. It's not something that can likely just be > reverted, and I'd be hesitant in doing so. It would also require disabling > the test coverage for it too I think. It could also cause ripples > elsewhere. I'd be against it. > > Thoughts from others? > > -- > Joshua C. Colp > Asterisk Technical Lead > Sangoma Technologies > Check us out at www.sangoma.com and www.asterisk.org > I too would be hesitant in doing a straight revert of the patch. I think at this point a new patch would be needed that fixes the newly reported bug ( ASTERISK-30051), but also maintains the current behavior of ASTERISK-28783 in some way. -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] Call unhold/topology change indication order
On Wed, May 11, 2022 at 11:50 AM Fridrich Maximilian wrote: > > You're in off-nominal untested un thought of territory, so the code > behavior > > probably reflects that. Specifically having both audio and video, and > doing > > hold/unhold. Audio hold is special and separate from stream negotiation, > > while video isn't, so things probably don't handle that. > > Considering that, I would like to have a discussion about considering > reverting > ASTERISK-28783 [1]. This change showed that the handling of non-default > streams > is not mature enough yet to be used in production environments. > Specifically, > one-way streams and hold/unhold are not handled correctly. In order to keep > Asterisk reliably usable for many different use-cases (Swiss Army Knife), I > would suggest reverting this change for now until the handling of > non-default > streams is mature enough to handle more than just the most basic use-cases. > > Please let me know what you think about this suggestion. > Oh, that's my change from long ago. That change has existed in the code base for quite a long time. It's not something that can likely just be reverted, and I'd be hesitant in doing so. It would also require disabling the test coverage for it too I think. It could also cause ripples elsewhere. I'd be against it. Thoughts from others? -- Joshua C. Colp Asterisk Technical Lead Sangoma Technologies Check us out at www.sangoma.com and www.asterisk.org -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] Call unhold/topology change indication order
On Wed, May 11, 2022 at 11:50 AM Fridrich Maximilian wrote: > > You're in off-nominal untested un thought of territory, so the code > behavior > > probably reflects that. Specifically having both audio and video, and > doing > > hold/unhold. Audio hold is special and separate from stream negotiation, > > while video isn't, so things probably don't handle that. > > Considering that, I would like to have a discussion about considering > reverting > ASTERISK-28783 [1]. This change showed that the handling of non-default > streams > is not mature enough yet to be used in production environments. > Specifically, > one-way streams and hold/unhold are not handled correctly. In order to keep > Asterisk reliably usable for many different use-cases (Swiss Army Knife), I > would suggest reverting this change for now until the handling of > non-default > streams is mature enough to handle more than just the most basic use-cases. > > Please let me know what you think about this suggestion. > What was the behavior before and after in the various cases, that we should consider reverting it? -- Joshua C. Colp Asterisk Technical Lead Sangoma Technologies Check us out at www.sangoma.com and www.asterisk.org -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] Call unhold/topology change indication order
On Wed, May 11, 2022 at 8:54 AM Fridrich Maximilian wrote: > Hi, > > I am currently working on fixing [ASTERISK-30051] res_pjsip: No video > after un-hold with moh_passthrough=yes. [1] > > I have debugged the code with DEBUG_THREADS enabled (which resolves the > issue) > and without and compared the two. The issue clearly seems to be that > chan_pjsip:chan_pjsip_indicate() is called in the wrong order: > > 1. It is called to indicate AST_CONTROL_STREAM_TOPOLOGY_REQUEST_CHANGE > 2. It is called to indicate AST_CONTROL_UNHOLD > > With DEBUG_THREADS enabled, AST_CONTROL_UNHOLD is always called first and > video > works as expected. > > Before I try to fix the underlying issue (which I think I have identified > - see > below) I want to understand why AST_CONTROL_UNHOLD needs to be indicated > before > AST_CONTROL_STREAM_TOPOLOGY_REQUEST_CHANGE. > > Does anyone have any idea why it has to be called in that order? > You're in off-nominal untested un thought of territory, so the code behavior probably reflects that. Specifically having both audio and video, and doing hold/unhold. Audio hold is special and separate from stream negotiation, while video isn't, so things probably don't handle that. -- Joshua C. Colp Asterisk Technical Lead Sangoma Technologies Check us out at www.sangoma.com and www.asterisk.org -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] Tons of warnings compiling Asterisk 16.24.1
On Wed, May 11, 2022 at 6:12 AM Dennis Buteyn wrote: > On 5/10/22 15:21, Joshua C. Colp wrote: > > On Tue, May 10, 2022 at 9:10 AM Dennis Buteyn > wrote: > >> Noticed a ton of warnings (-Wformat-truncation, -Wunused-result, >> -Wstringop-overflow, -Wstringop-truncation, -Wlto-type-mismatch, >> -Wfree-nonheap-object, -Warray-bounds) compiling Asterisk 16.24.1 on the >> few compilers I tried: >> >> gcc (Debian 8.3.0-6) 8.3.0 >> gcc (Debian 10.2.1-6) 10.2.1 20210110 >> gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0 >> gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0 >> gcc (Ubuntu 11.2.0-7ubuntu2) 11.2.0 >> >> Just curious if anyone is aware of these warnings and whether they have >> been resolved in later revisions. >> > > Are these the default warning options when built in developer mode? If so, > then we take care of them as soon as we become aware - and we just did a > batch of GCC12 fixes to take care of the latest ones. Otherwise you'd need > to be more specific about what exactly is showing them. For example, if > it's PJSIP ones we generally don't touch it and such fixes would need to go > upstream. If these are options you've set, then we wouldn't have seen them. > > > Rebased on master (a24979a2d7) and ignoring anything PJSIP-related, still > getting a bunch of warnings with gcc (Ubuntu 11.2.0-7ubuntu2) 11.2.0 > (attached). > > Compiler flags used: > > -Wformat > -Werror=format-security > -Wdate-time > -Wall > -Wstrict-prototypes > -Wmissing-prototypes > -Wmissing-declarations > -Wl,--warn-common > > As for the recent fixes, I guess we just need to wait for 16.27. > This didn't really answer my question. Are you using additional options beyond what is normally used? If so, then it is unlikely anyone would have seen these before or investigated and would explain why Jenkins isn't seeing them either. You'd need to file an issue[1] attaching the log, and then it will go into the queue to be looked at. [1] https://issues.asterisk.org/jira -- Joshua C. Colp Asterisk Technical Lead Sangoma Technologies Check us out at www.sangoma.com and www.asterisk.org -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] Tons of warnings compiling Asterisk 16.24.1
On 5/10/22 15:21, Joshua C. Colp wrote: On Tue, May 10, 2022 at 9:10 AM Dennis Buteyn wrote: Noticed a ton of warnings (-Wformat-truncation, -Wunused-result, -Wstringop-overflow, -Wstringop-truncation, -Wlto-type-mismatch, -Wfree-nonheap-object, -Warray-bounds) compiling Asterisk 16.24.1 on the few compilers I tried: gcc (Debian 8.3.0-6) 8.3.0 gcc (Debian 10.2.1-6) 10.2.1 20210110 gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0 gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0 gcc (Ubuntu 11.2.0-7ubuntu2) 11.2.0 Just curious if anyone is aware of these warnings and whether they have been resolved in later revisions. Are these the default warning options when built in developer mode? If so, then we take care of them as soon as we become aware - and we just did a batch of GCC12 fixes to take care of the latest ones. Otherwise you'd need to be more specific about what exactly is showing them. For example, if it's PJSIP ones we generally don't touch it and such fixes would need to go upstream. If these are options you've set, then we wouldn't have seen them. Rebased on master (a24979a2d7) and ignoring anything PJSIP-related, still getting a bunch of warnings with gcc (Ubuntu 11.2.0-7ubuntu2) 11.2.0 (attached). Compiler flags used: -Wformat -Werror=format-security -Wdate-time -Wall -Wstrict-prototypes -Wmissing-prototypes -Wmissing-declarations -Wl,--warn-common As for the recent fixes, I guess we just need to wait for 16.27. -- Dennis Buteyn Xorcom Ltd menuselect.c: In function 'generate_makeopts_file': menuselect.c:1689:41: warning: ignoring return value of 'system' declared with attribute 'warn_unused_result' [-Wunused-result] 1689 | system(touchcommand); | ^~~~ menuselect.c:1698:41: warning: ignoring return value of 'system' declared with attribute 'warn_unused_result' [-Wunused-result] 1698 | system(rmcommand); | ^ menuselect.c:1708:33: warning: ignoring return value of 'system' declared with attribute 'warn_unused_result' [-Wunused-result] 1708 | system(touchcommand); | ^~~~ menuselect.c:1717:33: warning: ignoring return value of 'system' declared with attribute 'warn_unused_result' [-Wunused-result] 1717 | system(rmcommand); | ^ ./include/asterisk/pbx.h:523:5: warning: type of 'ast_add_extension2' does not match original declaration [-Wlto-type-mismatch] ael_main.c:221:5: note: type mismatch in parameter 11 221 | int ast_add_extension2(struct ast_context *con, | ^ ael_main.c:221:5: note: 'ast_add_extension2' was previously declared here ./include/asterisk/cli.h:268:5: warning: type of '__ast_cli_register_multiple' does not match original declaration [-Wlto-type-mismatch] ael_main.c:204:6: note: return value type mismatch 204 | void __ast_cli_register_multiple(void) | ^ ael_main.c:204:6: note: type 'void' should match type 'int' ael_main.c:204:6: note: '__ast_cli_register_multiple' was previously declared here ./include/asterisk/pbx.h:335:6: warning: type of 'ast_context_destroy' does not match original declaration [-Wlto-type-mismatch] ael_main.c:414:6: note: type mismatch in parameter 1 414 | void ast_context_destroy(void) | ^ ael_main.c:414:6: note: 'ast_context_destroy' was previously declared here ./include/asterisk/cli.h:283:5: warning: type of 'ast_cli_unregister_multiple' does not match original declaration [-Wlto-type-mismatch] ael_main.c:408:6: note: return value type mismatch 408 | void ast_cli_unregister_multiple(void) | ^ ael_main.c:408:6: note: type 'void' should match type 'int' ael_main.c:408:6: note: 'ast_cli_unregister_multiple' was previously declared here ./include/asterisk/pbx.h:310:6: warning: type of 'ast_merge_contexts_and_delete' does not match original declaration [-Wlto-type-mismatch] ael_main.c:389:6: note: type mismatch in parameter 1 389 | void ast_merge_contexts_and_delete(void) | ^ ael_main.c:389:6: note: 'ast_merge_contexts_and_delete' was previously declared here ./include/asterisk/pbx.h:936:5: warning: type of 'ast_context_verify_includes' does not match original declaration [-Wlto-type-mismatch] ael_main.c:395:6: note: return value type mismatch 395 | void ast_context_verify_includes(void) | ^ ael_main.c:395:6: note: type 'void' should match type 'int' ael_main.c:395:6: note: 'ast_context_verify_includes' was previously declared here ./include/asterisk/pbx.h:1321:21: warning: type of 'ast_walk_contexts' does not match original declaration [-Wlto-type-mismatch] ael_main.c:401:22: note: type mismatch in parameter 1 401 | struct ast_context * ast_walk_contexts(v
