Re: [asterisk-dev] Call unhold/topology change indication order
On Thu, May 12, 2022 at 12:06 PM Fridrich Maximilian wrote: > Hi, > > I think I have found out why the indication order on hold/unhold matters: > > AST_CONTROL_HOLD/UNHOLD only cares about the audio stream and does not > touch > the topology of any other streams. So when Asterisk receives an SDP with > audio > and video and both are sendonly, chan_pjsip indicates AST_CONTROL_HOLD for > the audio stream and AST_CONTROL_STREAM_TOPOLOGY_REQUEST_CHANGE for the > video > stream. > > If AST_CONTROL_HOLD is indicated after > AST_CONTROL_STREAM_TOPOLOGY_REQUEST_CHANGE, it still has "outdated" > topology > information as it only cares about the default audio stream. So after the > stream topology is changed, it is "overwritten" by the outdated topology > from > the hold/unhold indication. > > To resolve the issue, the topology request change needs to check if this is > also a hold/unhold. If it is, there are two option: > > 1. Ensure that it executes after the hold/unhold indication > 2. Ensure that the hold/unhold indication receives the updated topology > > I'm stuck on implementing either of those solutions. I think the place we > need > to work on is in bridge_channel.c:bridge_handle_trip() before the call to > stream_topology_changed(). In bridge_handle_trip() we might still have a > chance > to interact with the other channel(s) in the bridge. > > Does anyone have any idea on how to proceed? > Not off the top of my head. I will try to give this some thought but I don't know if/when I'll really have any thought, it's not something that can really be answered without digging in deeply and refreshing memory on the entire view of everything. -- 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 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
