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

[Alexander Traud]

Alexander Traud has posted comments on this change.

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


Patch Set 9:

(5 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 */
> are we guaranteed (original code seems to indicate that f can be a list of 
Good catch. However, I am not aware of anything which calls ast_translate with several frames (aka a frame list). With Asterisk 13.6, a patch of me was added which supports several frames as output *within* ast_translate and as result of ast_translate. Or stated differently: If you found a case with several frames as input, you found a new regression. In that case, please, provide a concrete example and create a new issue report.


PS9, Line 597: while (0 < frames_missing) { /* lost packet(s) */
> Can't we insert a single frame with samples = total number of missing sampl
As Matthew pointed out, this suggestion would require to rewrite all codec modules because they are called with one frame each. Why do you want to have just a single frame?


PS9, Line 617: temp = ast_frdup(&missed); /* does not duplicate LIST_NEXT */
> How about just pushing these frames into path right here?  why queue them f
Yes, if I go for this alternative approach = do not honor consume for the newly introduced null frames, this is a valid suggestion. Does not make the code easier to write, but that is my problem; and as you state, better than moving memory around. Therefore: Can someone comment what the purpose of 'consume' is?


PS9, Line 617: temp = ast_frdup(&missed); /* does not duplicate LIST_NEXT */
> if consume == false these frames will be leaked.
I did that on purpose. Therefore, I am not sure about that one. I do not know what the caller is doing with the frame(s). Normally, the caller should free them automatically because otherwise the first frame would leak as well. Those (new) null frames belong to the original stream (and should have been there before). Therefore, those new frames stay there if the caller request ast_translate to do so. Therefore, I did not free them.

Anyway, I do not understand what this 'consume' does at all. Perhaps somebody can comment. If it is uncertain, instead of using this patch, what about freeing everything in the frame list before leaving ast_translate?


Line 637: 			while (current != inner && AST_LIST_NEXT(current, frame_list)) {
> Why find the last frame every time?  Why not just remember what the last fr
Mhm. The last frame was not calculated before, because it was the last frame of inner (the result of the last transcoding). Therefore, I am not sure I got your question, yet.


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