Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix learning address for asymmetric rtp #1682

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix learning address for asymmetric rtp #1682

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
076e127
fix learning address for asymmetric rtp
emvondo Jun 21, 2023
b3b30fe
disable endpoint learning based on stream flags too
emvondo Jun 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 13 additions & 18 deletions daemon/media_socket.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1457,7 +1457,7 @@ static const char *kernelize_one(struct rtpengine_target_info *reti, GQueue *out

if (PS_ISSET2(stream, STRICT_SOURCE, MEDIA_HANDOVER)) {
mutex_lock(&stream->out_lock);
__re_address_translate_ep(&reti->expected_src, MEDIA_ISSET(media, ASYMMETRIC) ? &stream->learned_endpoint : &stream->endpoint);
__re_address_translate_ep(&reti->expected_src, MEDIA_ISSET(media, ASYMMETRIC) && stream->learned_endpoint.address.family ? &stream->learned_endpoint : &stream->endpoint);
mutex_unlock(&stream->out_lock);
if (PS_ISSET(stream, STRICT_SOURCE))
reti->src_mismatch = MSM_DROP;
Expand Down Expand Up @@ -1880,13 +1880,13 @@ void __reset_sink_handlers(struct packet_stream *ps) {
}
void __stream_unconfirm(struct packet_stream *ps, const char *reason) {
__unkernelize(ps, reason);
if (!MEDIA_ISSET(ps->media, ASYMMETRIC)) {
if (ps->selected_sfd)
ilog(LOG_DEBUG | LOG_FLAG_LIMIT, "Unconfirming peer address for local %s (%s)",
endpoint_print_buf(&ps->selected_sfd->socket.local),
reason);

if (ps->selected_sfd)
ilog(LOG_DEBUG | LOG_FLAG_LIMIT, "Unconfirming peer address for local %s (%s)",
endpoint_print_buf(&ps->selected_sfd->socket.local),
reason);
PS_CLEAR(ps, CONFIRMED);
}

__reset_sink_handlers(ps);
}
static void stream_unconfirm(struct packet_stream *ps, const char *reason) {
Expand Down Expand Up @@ -2396,15 +2396,8 @@ static bool media_packet_address_check(struct packet_handler_ctx *phc)

PS_SET(phc->mp.stream, RECEIVED);

/* do not pay attention to source addresses of incoming packets for asymmetric streams */
if (MEDIA_ISSET(phc->mp.media, ASYMMETRIC) || phc->mp.stream->el_flags == EL_OFF) {
if (phc->mp.stream->el_flags == EL_OFF || rtpe_config.endpoint_learning == EL_OFF)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking rtpe_config.endpoint_learning shouldn't be necessary, as stream->el_flags should be set to the appropriate value already

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes i agree rtpe_config.endpoint_learning check is not necessary

PS_SET(phc->mp.stream, CONFIRMED);
if (MEDIA_ISSET(phc->mp.media, ASYMMETRIC) && !phc->mp.stream->learned_endpoint.address.family) {
mutex_lock(&phc->mp.stream->out_lock);
phc->mp.stream->learned_endpoint = phc->mp.fsin;
mutex_unlock(&phc->mp.stream->out_lock);
}
}

/* confirm sinks for unidirectional streams in order to kernelize */
if (MEDIA_ISSET(phc->mp.media, UNIDIRECTIONAL)) {
Expand Down Expand Up @@ -2515,10 +2508,12 @@ static bool media_packet_address_check(struct packet_handler_ctx *phc)
if (!wait_time || !phc->mp.stream->learned_endpoint.address.family ||
memcmp(use_endpoint_confirm, &phc->mp.stream->learned_endpoint, sizeof(endpoint)))
{
endpoint = phc->mp.stream->endpoint;
phc->mp.stream->endpoint = *use_endpoint_confirm;
endpoint = !MEDIA_ISSET(phc->mp.media, ASYMMETRIC) ? phc->mp.stream->endpoint : phc->mp.stream->learned_endpoint;
if (!MEDIA_ISSET(phc->mp.media, ASYMMETRIC))
phc->mp.stream->endpoint = *use_endpoint_confirm;

phc->mp.stream->learned_endpoint = *use_endpoint_confirm;
if (memcmp(&endpoint, &phc->mp.stream->endpoint, sizeof(endpoint))) {
if (memcmp(&endpoint, !MEDIA_ISSET(phc->mp.media, ASYMMETRIC) ? &phc->mp.stream->endpoint : &phc->mp.stream->learned_endpoint , sizeof(endpoint))) {
Comment on lines +2511 to +2516
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these 3 case distinctions are based on the same condition, so you could reduce this to just one if/else.

In particular, if ASYMMETRIC is set, then the final memcmp() condition will never be true, because it ends up comparing learned_endpoint against endpoint, and endpoint has just been set to learned_endpoint 3 lines earlier, with no chance of it ending up something different, no?

Copy link
Contributor Author

@emvondo emvondo Jul 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean something like

if (MEDIA_ISSET(phc->mp.media, ASYMMETRIC)){
    endpoint = phc->mp.stream->learned_endpoint;

}else{
    endpoint = phc->mp.stream->endpoint;
    phc->mp.stream->endpoint = *use_endpoint_confirm;
}

phc->mp.stream->learned_endpoint = *use_endpoint_confirm;

In particular, if ASYMMETRIC is set, then the final memcmp() condition will never be true,

yes but isn't that a specific case ? the same logic applies if ASYMMETRIC is not set we will end up comparing stream->endpoint to endpoint (set to stream->endpoint earlier). we are comparing old value with current value.

with no chance of it ending up something different, no?

yes if the value does not change but it could change (especially for asymmetric) but the same logic applies for non ASYMMETRIC stream i think ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean something like

if (MEDIA_ISSET(phc->mp.media, ASYMMETRIC)){
    endpoint = phc->mp.stream->learned_endpoint;

}else{
    endpoint = phc->mp.stream->endpoint;
    phc->mp.stream->endpoint = *use_endpoint_confirm;
}

phc->mp.stream->learned_endpoint = *use_endpoint_confirm;

Yes exactly.

In particular, if ASYMMETRIC is set, then the final memcmp() condition will never be true,

yes but isn't that a specific case ? the same logic applies if ASYMMETRIC is not set we will end up comparing stream->endpoint to endpoint (set to stream->endpoint earlier). we are comparing old value with current value.

with no chance of it ending up something different, no?

yes if the value does not change but it could change (especially for asymmetric) but the same logic applies for non ASYMMETRIC stream i think ?

I don't see how it could change. Consider your code above, with MEDIA_ISSET(phc->mp.media, ASYMMETRIC) being true. You set endpoint to learned_endpoint. And then immediately after you compare endpoint against learned_endpoint. How could these two ever be different?

For the non-asymmetric case you compare the previously learned endpoint (saved in endpoint) with the newly learned endpoint (use_endpoint_confirm stored into phc->mp.stream->endpoint) and use that to trigger a log message. (Not that the log message is crucial, but it makes me think that the logic isn't sound)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how it could change. Consider your code above, with MEDIA_ISSET(phc->mp.media, ASYMMETRIC) being true. You set endpoint to learned_endpoint. And then immediately after you compare endpoint against learned_endpoint. How could these two ever be different?

because before memcmp we update learned_endpoint

phc->mp.stream->learned_endpoint = *use_endpoint_confirm;

ilog(LOG_DEBUG | LOG_FLAG_LIMIT, "Peer address changed from %s%s%s to %s%s%s",
FMT_M(endpoint_print_buf(&endpoint)),
FMT_M(endpoint_print_buf(use_endpoint_confirm)));
Expand Down