Attention is currently required from: dexter. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/32218 )
Change subject: mgcp_codec: fix codec decision ...................................................................... Patch Set 8: (6 comments) Commit Message: https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/1201c686_7e648bb6 PS8, Line 12: againt agent? Patchset: PS8: I'm having difficulties reviewing: I need to manually figure out whether mgcp_codec_decide() has changed. Is it possible / easy to add a separate patch that only moves the function, and then I can easily see here what parts of it changed? I wrote some review below but I haven't completely read and understood this patch yet. Would be great if you could clarify a bit, thanks! File src/libosmo-mgcp/mgcp_codec.c: https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/6aec06cc_12a92977 PS8, Line 386: codecs_assigned = rtp_end->codecs_assigned; : OSMO_ASS (lets put these two lines inside the 'if { ...' below) https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/17e03822_c386cb44 PS8, Line 400: e (add a comma "in case of foo, barize") https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/5a2c0fd7_f9f5d8ec PS8, Line 401: * a tentative decision is made. i fail to understand: 1) Decide for both sides? The MGW has two lists of pt-nr to codec. It gets a specific pt-nr in an RTP packet on one side, and needs to decide what the pt-nr for this codec is on the other side. So how can there be a decision on both sides? 2) destination connection? RTP is UDP, there is no connection? do you mean to say "in case no destination endpoint is set up"? 3) how do codec choices matter when there is no destination side? this only makes sense to run when both sides of RTP are set up and ready? File src/libosmo-mgcp/mgcp_network.c: https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/762637cd_27599f00 PS8, Line 511: rtp_hdr->payload_type = (uint8_t) conn_dst->end.codec->payload_type; this looks wrong. There should not be one specific payload type on an endpoint. There should always be a list of possible payload types. This function should get as input the payload type number of one specific RTP packet that arrived, plus the list of pt-to-codec from the src side, and also the destination endpoint. 1. This should look up the codec that src-pt-nr means, in the source endpoint cfg. 2. Then should find this codec in the destination endpoint list and patch the matching pt-nr into this particular RTP packet. What confuses me is that it seems this is what the function was doing before this patch, and now it is changed to sort of pick one specific codec from conn_dst->end that has been decided on beforehand? (which would be wrong AFAIK) -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/32218 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: I6c3291f825488e5d8ce136aeb18450156794aeb5 Gerrit-Change-Number: 32218 Gerrit-PatchSet: 8 Gerrit-Owner: dexter <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels <[email protected]> Gerrit-Reviewer: pespin <[email protected]> Gerrit-CC: laforge <[email protected]> Gerrit-Attention: dexter <[email protected]> Gerrit-Comment-Date: Tue, 25 Apr 2023 19:32:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
