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

[N A]

Attention is currently required from: Joshua Colp, Jaco Kroon.

N A 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 5:

(4 comments)

Patchset:

PS3: 
> I agree with your comments here, I'm not completely sure about the peerformat vs ast_format_none, I' […]
Ack


Patchset:

PS4: 
> The current issue is seen (as far as I could determine) when we're receiving early media, which is a voice frame.

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.

> Yours could drop frames without it getting logged (goto cleanup).  Please make sure this doesn't result in leaked frames

The goto cleanup happens before jb_get, so I'm not following how frames could be leaked... could you clarify?

> and I'd want to add logging about that specific case

Agreed, I've added that, thanks.

> or that it then falls back to just using 20ms as the original code did (and my patch does).

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).


PS4: 
> As best I can understand, this issue only occurs when we're dealing with early media.

It's frames before any audio, including early media.

> 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?

It's the first voice frame, I believe, definitely not related to answer supervision from what I recall.

> 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.

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.


File channels/chan_iax2.c:

https://gerrit.asterisk.org/c/asterisk/+/19712/comment/a51751dc_3337a961 
PS2, Line 4169: 	if (voicefmt && ms >= (next = jb_next(pvt->jb))) {
> I've moved the assignments around. […]
Ack



-- 
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: 5
Gerrit-Owner: N A <asterisk at phreaknet.org>
Gerrit-Reviewer: Friendly Automation
Gerrit-CC: Jaco Kroon <jaco at uls.co.za>
Gerrit-CC: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Jaco Kroon <jaco at uls.co.za>
Gerrit-Comment-Date: Mon, 30 Jan 2023 22:57:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: N A <asterisk at phreaknet.org>
Comment-In-Reply-To: Jaco Kroon <jaco at uls.co.za>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20230130/6e7ab2b8/attachment.html>