<p> Attention is currently required from: Joshua Colp, Jaco Kroon. </p>
<p><a href="https://gerrit.asterisk.org/c/asterisk/+/19712">View Change</a></p><p>4 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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I agree with your comments here, I'm not completely sure about the peerformat vs ast_format_none, I' […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li></ul></li><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 #4:</a> </p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">The current issue is seen (as far as I could determine) when we're receiving early media, which is a voice frame.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Well, to be precise, it's when receiving frames prior to the first voice frame, I think... I noticed this on an IAX2 trunk that was sending text frames over the link before any voice. The text frames never make it to the other side with this bug.</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Yours could drop frames without it getting logged (goto cleanup).  Please make sure this doesn't result in leaked frames</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">The goto cleanup happens before jb_get, so I'm not following how frames could be leaked... could you clarify?</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">and I'd want to add logging about that specific case</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Agreed, I've added that, thanks.</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">or that it then falls back to just using 20ms as the original code did (and my patch does).</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">I don't believe this case should happen anyways, but if it does, then I guess the question is if it's safe to proceed without a voicefmt anyways. Maybe that was Kevin's concern about that possibly, which my patch currently sidesteps since it won't proceed if neither format exists (and in testing, this was fine).</p></li><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 #4:</a> </p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">As best I can understand, this issue only occurs when we're dealing with early media.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">It's frames before any audio, including early media.</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">I don't like the comment for the reason this is receiving a voice frame, which invalidates the comment, I'm not sure when voiceformat gets set but I believe it's when the call gets answered, which then sets the voice frame types we are permitted to send?</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">It's the first voice frame, I believe, definitely not related to answer supervision from what I recall.</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">In the case where the fallback mechanism also fails to obtain a time-value, we should either log that case, so that we can solve it, or default to the used-to-be default of 20ms.  Your call.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">I'm fine with either, but from the testing I did, I don't think both checks *should* generally fail, so if they did, it might be something to address in rechecking our assumptions rather than blindly using 20ms.</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/a51751dc_3337a961">Patch Set #2, Line 4169:</a> <code style="font-family:monospace,monospace">   if (voicefmt && ms >= (next = jb_next(pvt->jb))) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I've moved the assignments around. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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: 5 </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-CC: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Attention: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Attention: Jaco Kroon <jaco@uls.co.za> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 30 Jan 2023 22:57:14 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: N A <asterisk@phreaknet.org> </div>
<div style="display:none"> Comment-In-Reply-To: Jaco Kroon <jaco@uls.co.za> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>