[Asterisk-code-review] translate: Enables native Packet-Loss Concealment (PLC) for ... (asterisk[master])

[Jaco Kroon]

Jaco Kroon has posted comments on this change.

Change subject: translate: Enables native Packet-Loss Concealment (PLC) for supporting codecs.
......................................................................


Patch Set 9:

(3 comments)

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

PS9, Line 548: /* Out-of-order packet - more precise: late packet */
> Good catch. However, I am not aware of anything which calls ast_translate w
I don't have such examples.  Even though we don't have such examples, since it's (almost) trivial to deal with it, shouldn't we just handle them?

Without looking at the changes I actually suspect it's your patch to deal with multiple out frames that also allowed for submitting multiple in the first place, and I'm not 100% convinced that the code dealing with the timing submissions will actually do everything but on a quick glance it looks like it should.  I may be wrong here, but is there any harm in allowing multiple in frames?  Perhaps add a warning that multiple in frames has not been tested properly ... ?


PS9, Line 597: while (0 < frames_missing) { /* lost packet(s) */
> As Matthew pointed out, this suggestion would require to rewrite all codec 
Because it would be simpler.  No need to loop, or push back frames onto the front of the list, or anything odd, simply pass a single frame to path right here and be done with it.  That said, looping here is simpler than modifying all the codecs :).


PS9, Line 617: temp = ast_frdup(&missed); /* does not duplicate LIST_NEXT */
> I did that on purpose. Therefore, I am not sure about that one. I do not kn
consume says to "free" the passed in frames. So if consume is false we must not free them, if it's true we should.  Thus also where your memory leak comes from, you dynamically allocated the frames, and if consume is false, then those dynamically allocated frames will leak.


-- 
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-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes