[Asterisk-code-review] chan_iax2: Fix jitterbuffer regression prior to receiving audio. (asterisk[master])

[Jaco Kroon]

Attention is currently required from: N A.

Jaco Kroon has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/19712 )

Change subject: chan_iax2: Fix jitterbuffer regression prior to receiving audio.
......................................................................


Patch Set 3:

(2 comments)

Patchset:

PS3: 
This looks good to me, two minor comments in the code, and a more general overview here.

Kevin left some comments on my PR at https://gerrit.asterisk.org/c/asterisk/+/18689 which relates.  Specifically other pieces of code dereferences subclass.format and may crash.

The counter to this is that this change only affects frames going through the jitter buffer, and without the jitter buffer those frames would pass through the rest of the code as is anyway, so if they don't have subclass.format set, they would currently crash the server regardless.

The original commit 1b62831f2cfe5dcaa519885dd96b645fc05728e7 most likely added the voicefmt check here to avoid a crash on the first line inside the if() statement inside ast_format_get_default_ms().  Guessing we can also use ast_format_none here if voicefmt is null, so simply move voicefmt = inside the if(), and tac a ?: ast_format_none onto there (if we've not yet negotiated a format, we really have none rather than what we've got set for the peer?).

Whether to use the peerformat or ast_format_none here would be best answered by someone more familiar with the internal workings involved here.

Overall, the contemplated change here only affects the call to ast_format_get_default_ms() just inside the if, and JB_INTERP frames - which I'm fairly sure would not get generated prior to voice commencing.  I'm not quite following the code in _jb_get in main/jitterbuf.c in this regard, however, what I can say is that we will only get JB_INTERP frames if and only if silence_begin_ts is zero, with a default value of -1, and silence_begin_ts gets set to zero whenever we see a VOICE frame going through the JB, so I *think* we're safe here.


File channels/chan_iax2.c:

https://gerrit.asterisk.org/c/asterisk/+/19712/comment/be6889a2_16d77c13 
PS2, Line 4169: 	if (voicefmt && ms >= (next = jb_next(pvt->jb))) {
At this point voicefmt should always be properly set, so no need for the voicefmt check in the if statement.

Additionally, for performance reasons, it's probably best to move the voicefmt assignments to just inside the if statement now (why obtain voicefmt if we're not going to use it?).



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/19712
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Ie4fd1e8e088a145ad89e0427c2100a530e964fe9
Gerrit-Change-Number: 19712
Gerrit-PatchSet: 3
Gerrit-Owner: N A <asterisk at phreaknet.org>
Gerrit-Reviewer: Friendly Automation
Gerrit-CC: Jaco Kroon <jaco at uls.co.za>
Gerrit-Attention: N A <asterisk at phreaknet.org>
Gerrit-Comment-Date: Wed, 14 Dec 2022 20:25:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20221214/cb6b1de8/attachment.html>