-
Notifications
You must be signed in to change notification settings - Fork 782
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.
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
Update IP version choosing logic in media transport creation for generating SDP offer. #4067
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great improvement!
Is this true? |
IMO the "preference" is originally for good scenario (i.e: both usable). But when there is a problem such as "both are not usable" (we expect media won't work), I assume that IPv4 may have a little bit better chance to work than IPv6 (e.g: because we used to deal with IPv4 more than IPv6)? I am open to this, let's hear what others may say. Btw, as described in the definitions, "none is available" is different to "none is usable". But yes, even when both is not available, it will prioritize IPv4 for the same reason above. |
Since the user has explicitly specified its IPv6 preference, IMO following the user would be more reasonable. |
Right, that was my expectation when I read I'd like to hear what others think about this. |
I've updated the behavior based on your comments in 76496f0. Thanks for the feedback :) |
Maybe it may sense to split the enum/flags up in two or three. Could be more expressive. |
For example? |
enum {
PJSUA_IPV4 = 1,
PJSUA_IPV6 = 1 << 1
};
enum {
PJSUA_NO_PREFERENCE = 0,
PJSUA_PREFER_IPV4 = 1,
PJSUA_PREFER_ IPV6 = 2
}; For me, having 2 enums make it more expressive and it easier to reason about the cases. |
Having two enums adds a further conditional and in fact makes it somewhat unclear (at least to me); I prefer just one enum and am fine with the suggestion made by @nanangizz above. What if neither PJSUA_IPV4 or PJSUA_IPV6 are set? The suggested enum also reminded me of what you can set as socket option on dual-use sockets (IPV6_V6ONLY); Furthermore, how would you make it configurable in the app? Would you have two separate parameters? This seems unergonomic and not very user friendly (also remember, we're talking about the PJSUA layer here, which is pretty high level); and if you were to merge them together so that there's only one parameter? You'd probably end up with the suggestion above anyway. |
Looks like what you want is to separate settings of IP versions & fallback methods. Actually that is out of this PR scope. Note that changing the enum names (and as mentioned by @andreas-wehrmann, perhaps also changing the setting into two separate params) may cause a backward compatibility issue, thing we try to avoid. Also note that the enum is shared by media & SIP signaling. |
I am convinced :-) |
This will update the behavior of account config's
ipv6_media_use
:PJSUA_IPV6_DISABLED
, media will always use IPv4 regardless (may use loopback/local-link IP address).PJSUA_IPV6_ENABLED_USE_IPV6_ONLY
, media will always use IPv6 regardless (may use loopback/local-link IP address).PJSUA_IPV6_ENABLED_PREFER_IPV4
, media will use IPv4 if usable, otherwise it may fallback to IPv6 (if it is usable). If none is usable, it will prioritize IPv4 (if available).PJSUA_IPV6_ENABLED_PREFER_IPV6,
media will use IPv6 if usable, otherwise it may fallback to IPv4 (if it is usable). If none is usable, it will prioritize IPv6 (if available).PJSUA_IPV6_ENABLED
orPJSUA_IPV6_ENABLED_NO_PREFERENCE
, media will find any usable IP version, and prioritizing IPv4 (e.g: if both are usable/available).By useable, it means that the host has an IP address that is not loopback, local-link, nor disabled/invalid.
By available, it means that the host has an IP address for the specified IP version, including loopback or local-link address.