pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/15953 )
Change subject: add full SDP codec information to the MNCC socket ...................................................................... Patch Set 6: (7 comments) https://gerrit.osmocom.org/c/osmo-msc/+/15953/6/src/libmsc/cc_sdp.c File src/libmsc/cc_sdp.c: https://gerrit.osmocom.org/c/osmo-msc/+/15953/6/src/libmsc/cc_sdp.c@10 PS6, Line 10: /* In order of preference. TODO: make configurable */ Are we adding regression here? Wasn't it already configurable before? https://gerrit.osmocom.org/c/osmo-msc/+/15953/6/src/libmsc/cc_sdp.c@11 PS6, Line 11: static const enum gsm48_bcap_speech_ver mobile_codecs[] = { Not sure if it really makes sense having this static here. https://gerrit.osmocom.org/c/osmo-msc/+/15953/6/src/libmsc/cc_sdp.c@107 PS6, Line 107: for (a->payload_type = 96; a->payload_type <= 127; a->payload_type++) { would be great having defines for this numbers (96,127). https://gerrit.osmocom.org/c/osmo-msc/+/15953/6/src/libmsc/codec_sdp_cc_t9n.c File src/libmsc/codec_sdp_cc_t9n.c: https://gerrit.osmocom.org/c/osmo-msc/+/15953/6/src/libmsc/codec_sdp_cc_t9n.c@170 PS6, Line 170: const struct codec_mapping *codec_mapping_by_gsm0808_speech_codec_type(enum gsm0808_speech_codec_type sct, uint16_t cfg) either fix the TODO or mark cfg as unused for the compiler. https://gerrit.osmocom.org/c/osmo-msc/+/15953/6/src/libmsc/gsm_04_08_cc.c File src/libmsc/gsm_04_08_cc.c: https://gerrit.osmocom.org/c/osmo-msc/+/15953/6/src/libmsc/gsm_04_08_cc.c@1261 PS6, Line 1261: GSM48_PDISC_CC | (trans->transaction_id << 4), wrong indentation. https://gerrit.osmocom.org/c/osmo-msc/+/15953/6/src/libmsc/sdp_msg.c File src/libmsc/sdp_msg.c: https://gerrit.osmocom.org/c/osmo-msc/+/15953/6/src/libmsc/sdp_msg.c@558 PS6, Line 558: if (codec->rate != 8000) Next time, please split this kind of stuff into separate commits for easier review. https://gerrit.osmocom.org/c/osmo-msc/+/15953/6/tests/msc_vlr/msc_vlr_test_call.err File tests/msc_vlr/msc_vlr_test_call.err: https://gerrit.osmocom.org/c/osmo-msc/+/15953/6/tests/msc_vlr/msc_vlr_test_call.err@429 PS6, Line 429: This empty line in stderr (libosmocore log) looks suspìcious.As if you are adding an extra \n somewhere. -- To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/15953 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Change-Id: I8c3b2de53ffae4ec3a66b9dabf308c290a2c999f Gerrit-Change-Number: 15953 Gerrit-PatchSet: 6 Gerrit-Owner: neels <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <[email protected]> Gerrit-Reviewer: neels <[email protected]> Gerrit-Reviewer: pespin <[email protected]> Gerrit-Comment-Date: Tue, 02 Jun 2020 10:06:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
