Attention is currently required from: neels. dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/32218 )
Change subject: mgcp_codec: fix codec decision ...................................................................... Patch Set 9: (7 comments) Commit Message: https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/86779cf9_6563eada PS8, Line 12: againt > agent? Done Patchset: PS8: > I'm having difficulties reviewing: I need to manually figure out whether > mgcp_codec_decide() has cha […] Done File src/libosmo-mgcp/mgcp_codec.c: https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/3d48ff3e_6e75854a PS8, Line 386: codecs_assigned = rtp_end->codecs_assigned; : OSMO_ASS > (lets put these two lines inside the 'if { ... […] Done https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/d77ee675_e934961c PS8, Line 386: codecs_assigned = rtp_end->codecs_assigned; : OSMO_ASS > (lets put these two lines inside the 'if { ... […] Done https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/853f3cf1_5fef3d64 PS8, Line 400: e > (add a comma "in case of foo, barize") Done https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/11618c55_aaf10a76 PS8, Line 401: * a tentative decision is made. > i fail to understand: […] 1) The decision is made for both directions and each side keeps the result in end.codec. The main idea is to make a conscious codec decision (which still may be changed though) once and then convert (if necessary) all packets according to this decision. This also means that we can check if the packets we receive match our decision and toss the ones we do not expect. Also more importantly we also always know into what format/codec we have to convert before we emit the packets. 2) "connection" does not refer so much for RTP, refers to the "conn" we create and manage here. Destination endpoint is also wrong since both "connections" live on one endpoint. RFC3435 also uses the term "connection". 3) When the connections are created, they are created one after another, so when the first connection is created it will have no partner or destination connection. But still we must make an initial codec choice. This is in particular important when the first connection is created in loopback mode. When the second connection is created, then the mgcp_codec_decide is called from the other side again and the full informed codec choice can be made. This may also mean that the codec changes, but it will always be a change that is valid within what both sides have signalled via SDP. So the tentative codec choice it is kind of a corner case but has a very practical reason. File src/libosmo-mgcp/mgcp_network.c: https://gerrit.osmocom.org/c/osmo-mgw/+/32218/comment/910c8bad_f2815f08 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. […] I think there must be a consciously chosen payload type for each connection (endpoint is the entity that contains the two connections). I think that this is necessary so that we are able to ignore unexpected packets. Lets imagine that on side A a call agent assigns amr-bwe/PT1, and amr-oa/PT2 for compatibility reasons. On side B only amr-bew/PT3 is assigned. Now side A sends the same audio stream with amr-bew/PT1 and amr-oa/PT2. If we do not know what to expect we would "mix" both audio stream to amr-bew/PT3. This means we have to make a decision and we should make it based on the codecs that both sides have assigned and then follow it. We also cannot make the decision for every packet that arrives since we would end up in the "mix"-effect described below. It is also better to make the decision not for each packets to safe some CPU cycles. Since the decision is already made when the connection is created we can just look up the payload type in end.codec->payload_type. (payload conversion is done in other functions) What might be necessary in the future though are separate codec decisions in case we want to transfer video as well. Then we might need an end.codec_video->payload_type as well. But thats out of scope for the moment as there are no plans to support video telephony yet. -- 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: 9 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: neels <[email protected]> Gerrit-Comment-Date: Wed, 26 Apr 2023 15:20:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <[email protected]> Gerrit-MessageType: comment
