| Summary: | ASTERISK-19562: [patch] ConfBridge - Inconsistent hold-music behaviour | ||||||
| Reporter: | Neil Tallim (flan) | Labels: | |||||
| Date Opened: | 2012-03-19 19:03:46 | Date Closed: | 2012-10-08 08:48:27 | ||||
| Priority: | Major | Regression? | |||||
| Status: | Closed/Complete | Components: | Applications/app_confbridge | ||||
| Versions: | SVN 10.0.1 10.1.0 10.1.1 10.1.2 10.1.3 10.2.0 10.2.1 | Frequency of Occurrence | Constant | ||||
| Related Issues: |
| ||||||
| Environment: | Attachments: | ( 0) bugASTERISK-19562_ASTERISK-19726_ASTERISK-20181.patch ( 1) jrose_confbridge_suspension_and_startup_inconsistencies.diff ( 2) moh_everything_spacing_fixes.diff ( 3) moh_everything.patch ( 4) moh_leave.patch ( 5) moh_marked.patch ( 6) moh_unmarked.patch | |||||
| Description: | [moh_marked.patch] Primary symptom, with reproduction: 1) Marked user joins, with hold-music enabled (no music plays) 2) Unmarked user joins 3) Unmarked user leaves (marked user hears music) 4) Unmarked user joins (marked user still hears hold music) This patch primarily resolves step 4, disabling hold music when other users (marked or unmarked) join. This is effected by the first, second, and fourth hunks of the patch. It also changes the behaviour in step 1 for consistency, causing marked users to hear music when they're alone, if MoH is enabled in their profile. This is accomplished by the third hunk. ----- [moh_unmarked.patch] This is a behavioural adjustment. Before, the arrival of a second user, regardless of whether it had WAITMARKED set or not, would turn off MoH for the first non-WAITMARKED participant. This patch basically just introduces a loop over all pre-existing participants to look for a conversation partner: any user who either does not have WAITMARKED set or has MARKEDUSER; if those conditions are met, the new user is not put on hold and the eligible pre-existing user(s -- though there should only ever be one) is taken off hold. ----- [moh_leave.patch] The MoH-enabling loop now applies in all cases, not just when a marked user leaves. Marked-user-related kicking still happens as before, with the MoH loop checking the kicked flags of participants to avoid redundant or illogical work. If there are conversation partners for any non-WAITMARKED users, they are not put on hold. Anyone who is WAITMARKED is put on hold if there is no marked user. If there's only one user, they're put on hold without any consistency checks, if MoH is enabled. Users without WAITMARKED are no longer muted before the leader-has-left announcement is played, since not every non-WAITMARKED re-entry path performs unmuting, so it could eaisly have unintended side-effects that likely aren't worth the cost. ----- [moh_everything.patch] Contains everything described above. The first and second patches have a conflict if applied independently, since they solve issues individually and both solutions affect the same lines in post_join_unmarked. ----- First-party testing involved calling in with multiple marked/unmarked combinations (including with multiple marked users) and evaluating correctness of behaviour. Every conceivable scenario worked as intended. | ||||||
| Comments: | By: Neil Tallim (flan) 2012-03-20 15:27:34.256-0500 Replaced patch with an update that addressed a false assumption about how ast_test_flag() works: it uses & to compare values, so boolean | was not the same as two separate tests; this caused two participants with WAITMARKED to disable hold music when it should have kept playing. By: Neil Tallim (flan) 2012-03-22 14:28:19.199-0500 Strangely, if you upload multiple files, they don't all get code-contrib flags set. By: Matt Jordan (mjordan) 2012-03-26 07:55:19.060-0500 Thank you for contributing this patch. The next step is to post the patch for review on ReviewBoard (https://reviewboard.asterisk.org/) for developer review and comment. Instructions for using ReviewBoard can be found at https://wiki.asterisk.org/wiki/display/AST/Reviewboard+Usage. If you need an account, please contact me at mjordan@digium.com with a desired username and I'll set up an account for you. Thanks Matt By: Neil Tallim (flan) 2012-03-26 17:54:40.714-0500 _leave didn't have the WAITMARKED check that was present in _everything. _marked and _everything had an inconsistent line of logic that just exhibited itself today: (ast_test_flag(&conference_bridge_user->u_profile, USER_OPT_WAITMARKED) && (!conference_bridge->markedusers || conference_bridge->users == 1)) should have been, and now is ((ast_test_flag(&conference_bridge_user->u_profile, USER_OPT_WAITMARKED) && !conference_bridge->markedusers) || conference_bridge->users == 1) By: Jonathan Rose (jrose) 2012-05-10 09:12:29.553-0500 Adding a revision to the moh_everything patch to fix the redspace. I'm not calling it a code contribution since it's just spacing changes to moh_everything.patch. By: Jonathan Rose (jrose) 2012-06-15 16:09:00.540-0500 Hey Neil, I was looking at this patch you wrote, and as I started reproducing the issues here, I became a little baffled about just how off our behavior was when it came to mixed marked and unmarked users (that don't wait for marked). I've been talking a little with other devs about how it should work, and we came up with the following: 1. conferences should either be running or not running for the most part... 2. if two non-waiter non-marked users enter, the conference should start. Waiters who are in the conference at that point should enter with them. Really, people probably shouldn't be mixing waiters, non waiters, and marked users... but we should still have behavior for it, so that's what it is. 3. If the last marked user leaves, we'll still go ahead and put the waiters back in music on hold even if there are other callers in the conference that constitute a legitimate conference. I personally disagreed with this behavior since it conflicts a little with the idea that a conference should either be active or inactive, but it's not that big of a deal to me. I've attached a patch that I wrote based loosely on yours that I'd appreciate your opinion on since you are obviously looking at this sort of stuff. It changes the behavior of joins and leaves from what they currently are somewhat significantly. Please let me know if you feel this is working better for you or worse and if you have anything you'd like to add to the discussion on how things should work in relation to this patch. It's in the attachments list above and is called jrose_confbridge_suspension_and_startup_inconsistencies.diff Thanks for your involvement in Asterisk. By: Jonathan Rose (jrose) 2012-06-18 15:38:17.603-0500 After some more internal discussion, it looks like we are actually going to go ahead and keep wait_marked users out of the conference even when there are enough normal users (nonmarked, nonwait_marked) in, so that's changed from the above comment. By: Neil Tallim (flan) 2012-06-18 17:12:24.667-0500 Hi, Jonathan. I meant to get to this earlier today (hectic weekend prevented anything sooner), but got caught up with other tasks. My inclination is the same as what you just described, that "wait_marked" users should have to depend on a marked user at all times. If there were a priority structure to conferences (like admin > marked > non-wait-marked > wait_marked), they would undoubtedly be at the bottom. I don't like the idea of mixing them much, either, because it introduces a more complex state structure that's likely of use to only a small portion of potential users, but there's no technical reason not to support it, and the foundation is already there, so it should probably just be defined to remove surprises and make it consistent, whatever "consistent" ends up being. Should I take a look at your patch now or do you need/want to make changes to it in light of your most recent comment? Also, I should be thanking you for helping to make Asterisk accessible. Anything that helps the proliferation of open source technologies in legacy-heavy domains is welcome and I'm glad to both be able to contribute in any way and to learn from your feedback. By: Jonathan Rose (jrose) 2012-06-25 13:06:57.615-0500 At the moment, things are a little hairy when it comes to defining the behavior we want. There were some ideas being tossed around about how to handle this stuff the other day. I don't know if you use are on the Asterisk IRC channels at all, but if you wouldn't be opposed to jumping on a while some time, Terry Wilson and I would like to chat with you in realtime for a while about how to solve this dilemma. If that's something you would care to do, please tell me if you need any help with that and/or what time you would like to have that discussion. We are available more or less for certain on weekdays between 10AM and 3PM on (U.S. Central Time) and can probably make arrangements to talk at evening as well. At the moment, there is a notion to split conferences into two separate containers of users, those being active users and inactive users. Inactive users become active when conditions are met that vary upon the user type. Active users become inactive when those conditions are no longer true. Marked Activation Conditions: Two or more users (of any kind) are in the conference total_users >= 2 Wait for Marked Activation Conditions: One or more marked users are in the conference marked_users >= 1 Normal Activation Conditions: Two or more users who don't wait for marked are in the conference. (total_users - wait_marked_users) >= 2 When a user joins, if any of these conditions is true, users in the inactive container should be checked and switched into the active pool (and also suspend mute <unless the mute at start flag is raised> and music on hold <if applicable>) if their condition for entry is met. When a user joins, if any of those conditions are false, users in the active container should be checked and switched to the inactive pool (enabling music on hold and mute) if their specific conditions for entry are no longer met. Terry was talking with me earlier and he has more or less come around on the idea that being inactive should mute the user. There are ways to record the user regardless of whether music on hold is playing on the channel involving monitor. I'm eager to hear from you. By: Neil Tallim (flan) 2012-06-25 13:27:57.811-0500 Having a concrete notion of containers would certainly make the code easier to understand than just checking a bunch of conditions all the time, so I'd be very willing to help with setting that up and verifying logic flows. (Maybe it might make sense to extend the idea in other places, if it seems like it would help to clarify intent, too) I'm guessing the monitor-to-handle-recording-while-muted thing came from my discussion with Terry, but I'd like to hear his thoughts on the issue, too, in case he's thought of a condition or scenario in which my proposed revisioned solution doesn't apply. You can usually find me on Freenode as "flan", so I can drop by #asterisk easily enough, although probably not this afternoon because of workplace meetings. I'm in Mountain, if knowing my timezone helps. By: Jonathan Rose (jrose) 2012-06-25 15:35:57.297-0500 Ah, excellent. The channel you'd be wanting to pop into is actually #asterisk-dev, and we tend to be there for pretty much the whole work day if not then some. Just preface a message to me with whatever username I'm using (always jrose or jrose_atDigium or something similar) and I'll see if we can't get the conversation going. By: Rusty Newton (rnewton) 2012-07-26 16:25:47.772-0500 ASTERISK-20148 is a report by a different reporter, duplicate or very similar issue. By: Leif Madsen (lmadsen) 2012-08-29 10:48:37.589-0500 Any additional information to report on this issue from Neil? Thanks! By: Rusty Newton (rnewton) 2012-09-28 18:43:52.980-0500 Neil, is the issue completely resolved? By: Matt Jordan (mjordan) 2012-10-08 08:19:56.792-0500 Final patch by Terry Wilson (bugASTERISK-19562_ASTERISK-19726_ASTERISK-20181.patch) from review board https://reviewboard.asterisk.org/r/2072/ attached. | ||||||