fix learning address for asymmetric rtp

[emvondo]

fixes for #1679 and #1680

[rfuchs]

Still not quite sure about the logic here 😅

[rfuchs]

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

[emvondo]

yes i agree rtpe_config.endpoint_learning check is not necessary

[rfuchs]

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?

[emvondo]

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 ?

[rfuchs]

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)

[emvondo]

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;