[Asterisk-code-review] translate: Re-enables native Packet-Loss Concealment (PLC) f... (asterisk[master])

[Jaco Kroon]

Jaco Kroon has posted comments on this change.

Change subject: translate: Re-enables native Packet-Loss Concealment (PLC) for iLBC and Speex.
......................................................................


Patch Set 9: Code-Review-1

(4 comments)

whilst most of the logic and reasoning seems fine and I'm definitely for this, I think it's a little more complex than needed on the last hunk, and there is a few assumptions that may or may not hold.  For example, I'm not sure if any of the current callers passes multiple frames (jitter buffers?) but the existing code allowed for that, the new code only checks the first frame for out-of-order as well as PLC, if it's possible to get multiple input frames that assumption doesn't hold.  Is it possible that frames in the incoming list might simple be out-of-order?

Base idea I think is excellent.

https://gerrit.asterisk.org/#/c/1820/9/main/translate.c
File main/translate.c:

PS9, Line 566: 
             : 
Isn't here to check if a frame was missing and to add a framein() for a frame with the number of missing samples directly here?  Also, it's possible to just skip over out-of-order samples here completely in case incoming f is a list.

A bit inefficient since frameout() should be PLC'ed already and thus the logic should only really kick in on the first iteration through this loop.


PS9, Line 548: /* Out-of-order packet - more precise: late packet */
are we guaranteed (original code seems to indicate that f can be a list of frames - lines 563 through 569 - are we guaranteed that the incoming frames is in-order and that these frames are "packed" (meaning, no time gaps)?


PS9, Line 597: while (0 < frames_missing) { /* lost packet(s) */
Can't we insert a single frame with samples = total number of missing samples?


PS9, Line 617: temp = ast_frdup(&missed); /* does not duplicate LIST_NEXT */
if consume == false these frames will be leaked.


-- 
To view, visit https://gerrit.asterisk.org/1820
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfcf0670e094e9718d82fd9920f1fb2dae122006
Gerrit-PatchSet: 9
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Alexander Traud <pabstraud at compuserve.com>
Gerrit-Reviewer: Alexander Traud <pabstraud at compuserve.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Jaco Kroon <jaco at uls.co.za>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matthew Fredrickson <creslin at digium.com>
Gerrit-HasComments: Yes