[Asterisk-code-review] translate: Fix transcoding while different in frame size. (asterisk[13])

[Alexander Traud]

Alexander Traud has posted comments on this change.

Change subject: translate: Fix transcoding while different in frame size.
......................................................................


Patch Set 6:

(1 comment)

> when the affected gsm, ilbc, lpc10, and speex codecs are involved.

… and Opus, AMR, AMR-WB, and GSM-EFR. I do no know the source code of the G.729 and SILK modules, but they might create wrong frames as well.

As stated in the original bug-report, especially with AMR(-WB) this bug is not major but even a critical bug because two-third of the overall samples get discarded. I think a dropped frame is better than a wrong frame.

I am not an integrator and do not have a bird's eyes view on all callers of ast_translate. Again, I think a dropped frame is better than a wrong frame. Anyway, if you found misuses of the API in callers of ast_translate, please, create an issue report for them – with test cases to reproduce those scenarios. Then somebody might pick those and fixes the callers one by one. Perhaps I find the time and resources (for things I do not use yet). However, without test-cases I do not know how to use/call/test those control flows at all.

If you think those issues do block this patch here, please, create those issues as child issues of ASTERISK-25353. In that case, please, do understand my surprise that this is brought up to my attention four days after the first code-review, in the fourth code-review cycle.

https://gerrit.asterisk.org/#/c/1155/6/main/translate.c
File main/translate.c:

Line 576: 				} else {
        : 					path->nextout = ast_tvadd(path->nextout, ast_samp2tv(
        : 						current->samples, ast_format_get_sample_rate(current->subclass.format)));
        : 				}
        : 
        : 				if (f->samples != current->samples && ast_test_flag(current, AST_FRFLAG_HAS_TIMING_INFO)) {
        : 					ast_debug(4, "Sample size different %d vs %d\n", f->samples, current->samples);
        : 					ast_clear_flag(current, AST_FRFLAG_HAS_TIMING_INFO);
        : 				}
> The if (f->samples... code needs to be part of the else clause since that i
This would change the previous control flow (CNG frames *did* pass this if-condition previously). Furthermore, this change is out of scope of this patch. If you think you found a bug in the previous code, please, create an issue report and propose a change. I cannot judge on your proposed change because I am not an expert in this area. Making this change would be a +1 from me, which I cannot provide. Sorry.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2e229569d73191d66a4e43fef35432db24000212
Gerrit-PatchSet: 6
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Alexander Traud <pabstraud at compuserve.com>
Gerrit-Reviewer: Alexander Traud <pabstraud at compuserve.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes