> On June 10, 2014, 11:22 a.m., Joshua Colp wrote: > > /branches/11/main/channel.c, line 7113 > > <https://reviewboard.asterisk.org/r/3603/diff/1/?file=59449#file59449line7113> > > > > This change is hazardous. It makes it so that some file descriptors on > > the "original" channel which were from the past channel that inhabited the > > structure will survive if the new channel taking its place does not have > > those file descriptors set. Effectively the file descriptors on the channel > > become a combination of the both of them. > > > > In the case of a generator this is correct because the channel taking > > its place should have the same generator as before (if you get masqueraded > > in you should hear ringing or music on hold). > > > > The question becomes for this - what is the proper behavior? That of > > generators or that of everything else? If I masquerade into another channel > > should I then be using the jitterbuffer dialplan function that they > > invoked? If so should it be reset since there is a new source of media? > > > > Right now this doesn't work because that behavior is undefined. The > > jitterbuffer code itself assumes yes, but as you've said the file > > descriptor is not preserved so it can't. > > Matt Jordan wrote: > There is another way to handle this that may be safer: the fixup callback > on the jitterbuffer channel datastore. > > Inside that fixup, the timer on the old channel can be forgotten (or > properly disposed of) and a new one created on the channel we are > masquerading into (although some checks should probably be done to make sure > that if a timer is already on it, we clean it up). > > Corey Farrell wrote: > I feel that jitterbuffer should at minimum follow the same rules as a > generator. If I enable jitterbuffer on a Local channel then > Dial(SIP/anything), I expect to have a functioning jitterbuffer after > SIP/anything is answered, the same as if the local channel was > non-optimizing. On the other hand if I have jitterbuffer enabled by a > variable set in sip.conf for the peer "anything", then I expect it to not be > disabled by lack of jitterbuffer on the Local channel. In situations where > both sides of the masquerade have jitterbuffer enabled I think we should use > the JB from 'original' instead of from 'clonechan'. My thought is that JB > was explicitly set by dialplan for 'original', where clonechan is set by peer > configuration and is therefore just a default. > > > 1) We can fix situations where JB is set on 'original' but not > 'clonechan' by not overwriting AST_JITTERBUFFER_FD. I feel like this should > be dealt with the same way as AST_GENERATOR_FD. > > > 2) For situations where both channels have JB set, we have to prevent the > JB datastore of 'clonechan' from being copied to 'original'. It looks like > the datastore chan_fixup would need to free the 'clonechan' datastore when > this happens. > > > 3) I think any situation where 'clonechan' has JB set will currently > cause corruption. ast_do_masquerade transfers datastores but not framehooks. > This would cause the framehook of 'clonechan' to be freed, but the datastore > will be transferred to 'original' with datastore->data pointing to an invalid > 'int *id'. > > I think this could be fixed by the datastore chan_fixup. It could > retrieve the framehook data from 'clonechan', copy all fields to a new > structure and zero the framehook data from clonechan to prevent jb/timer from > being destroyed when that framehook is freed. Finally we would need to set > AST_JITTERBUFFER_FD on 'original'. > > For this to work we will need a new framehook.c function: > void *ast_framehook_get_data(struct ast_channel *chan, int framehook_id);
In reality stuff is not as clear cut as this, and Local channels complicate things. The semantics you've applied to original and clonechan may or may not be true, depending on things. The operation is at its very heart take the channel clonechan and "swap" it with original, unfortunately this occurs by way of changing the structure contents. Once completed make original go away. The case that you've marked as half fixed is actually the one I would say most people use, they set a jitterbuffer on their channel (not Local) and they expect it to persist. I think that placing special logic within the operation to try to figure out what people want based on things is just fraught with peril, even with some sort of priority basis as you've used. I'd rather we document a specific simple behavior for this and try not to let Local channels influence it. If someone wants to keep a jitterbuffer they can disable optimization. - Joshua ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3603/#review12107 ----------------------------------------------------------- On June 13, 2014, 6:48 p.m., Corey Farrell wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3603/ > ----------------------------------------------------------- > > (Updated June 13, 2014, 6:48 p.m.) > > > Review request for Asterisk Developers, Joshua Colp and Matt Jordan. > > > Bugs: ASTERISK-22409 > https://issues.asterisk.org/jira/browse/ASTERISK-22409 > > > Repository: Asterisk > > > Description > ------- > > During masquerade it is possible for the AST_JITTERBUFFER_FD to be cleared > (set to -1). This change adds a check when copying channel fd's to prevent > clearing an FD with -1. This seems to resolve the bad audio quality > experienced after the masquerade. When AST_JITTERBUFFER_FD was set to -1, > this prevented the channel from polling that timer. This caused RTP packets > to be received late, and discarded. > > The changes to func_jitterbuffer.c were created first. ast_free(jbframe); is > needed to prevent jbframe from leaking if it is rejected by jb_impl. This > ensures we don't start leaking packets if they are received too late or > rejected by jb_impl for any other reason. > > The second change to func_jitterbuffer prevents a leak of ast_null_frame's > that were duplicated (ie with ast_frdup or ast_frisolate). I believe this > leak might actually be unrelated to the masquerade issue, and likely occurs > for every single ast_null_frame. > > > Diffs > ----- > > /branches/11/main/channel.c 416102 > /branches/11/funcs/func_jitterbuffer.c 416102 > > Diff: https://reviewboard.asterisk.org/r/3603/diff/ > > > Testing > ------- > > Verified the scenario outlined in ASTERISK-22409 no longer experiences audio > quality loss, and no longer causes leaks (tested under valgrind). I patched > asterisk to ensure that ast_frfree performed an immediate free to ensure > valgrind would report any attempted use after free. > > In early testing, I used debug messages instead of the added ast_frfree's - I > verified the leaked frames reported by valgrind matched exactly to the number > of debug messages. > > For the masquerade fix I tested with some debug code that showed the old and > new FD, this is how I found the valid FD being replaced by -1. See JIRA > ticket for example output. > > I have not tested this issue or fix against 12+, but the relevant code is the > same as 11 - func_jitterbuffer code was moved to core but still the same code. > > > Thanks, > > Corey Farrell > >
-- _____________________________________________________________________ -- 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
