neels 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//COMMIT_MSG Commit Message: https://gerrit.osmocom.org/c/osmo-msc/+/15953/6//COMMIT_MSG@39 PS6, Line 39: mnccc_release_sent > ccc? ;) heh we should invent that, a hacker's call control interface for congresses 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? i should check that indeed. I think the preference passed from the BSS still matters most, but I should take another look 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. right, doesn't https://gerrit.osmocom.org/c/osmo-msc/+/15953/6/src/libmsc/cc_sdp.c@27 PS6, Line 27: static const enum gsm48_bcap_speech_ver utran_codecs[] = { neither 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. how? 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. will split, missed this one 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. […] will check -- 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: Sat, 11 Jul 2020 02:13:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <[email protected]> Comment-In-Reply-To: laforge <[email protected]> Gerrit-MessageType: comment
