Attention is currently required from: neels.
7 comments:
Commit Message:
Patch Set #8, Line 12: againt
agent?
Done
Patchset:
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:
codecs_assigned = rtp_end->codecs_assigned;
OSMO_ASS
(lets put these two lines inside the 'if { ... […]
Done
codecs_assigned = rtp_end->codecs_assigned;
OSMO_ASS
(lets put these two lines inside the 'if { ... […]
Done
(add a comma "in case of foo, barize")
Done
Patch Set #8, 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:
Patch Set #8, 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 change 32218. To unsubscribe, or for help writing mail filters, visit settings.