<p> Attention is currently required from: N A. </p>
<p><a href="https://gerrit.asterisk.org/c/asterisk/+/19712">View Change</a></p><p>2 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">Patchset:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/19712?tab=comments">Patch Set #3:</a> </p><p style="white-space: pre-wrap; word-wrap: break-word;">This looks good to me, two minor comments in the code, and a more general overview here.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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?).</p><p style="white-space: pre-wrap; word-wrap: break-word;">Whether to use the peerformat or ast_format_none here would be best answered by someone more familiar with the internal workings involved here.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File channels/chan_iax2.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/19712/comment/be6889a2_16d77c13">Patch Set #2, Line 4169:</a> <code style="font-family:monospace,monospace">     if (voicefmt && ms >= (next = jb_next(pvt->jb))) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">At this point voicefmt should always be properly set, so no need for the voicefmt check in the if statement.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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?).</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/19712">change 19712</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/19712"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Ie4fd1e8e088a145ad89e0427c2100a530e964fe9 </div>
<div style="display:none"> Gerrit-Change-Number: 19712 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: N A <asterisk@phreaknet.org> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-CC: Jaco Kroon <jaco@uls.co.za> </div>
<div style="display:none"> Gerrit-Attention: N A <asterisk@phreaknet.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 14 Dec 2022 20:25:07 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>