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.

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

Extending the uv_udp_send_t to hold a "before_send" callback #2536

Closed
ibc opened this issue Nov 6, 2019 · 18 comments
Closed

Extending the uv_udp_send_t to hold a "before_send" callback #2536

ibc opened this issue Nov 6, 2019 · 18 comments
Labels

Comments

@ibc
Copy link
Contributor

ibc commented Nov 6, 2019

I need to set UDP or TCP socket options (via setsockopt()) per sent data, this is, I may need to set different socket options for each new UDP datagram or TCP data to be sent.

For instance, this is my use case:

I need to set different DSCP values to UDP packets (actually IP packets) sent on
the same uv_udp_t handle. For instance, I send RTP audio and video packets and need to set a specific DSCP/ToS value just for audio packets:

pseudo-code:

int tos = 0x60; // CS3 just for audio packets.
uv_os_fd_t fd;

// Get the real file descriptor from our libuv UDP handle:
uv_fileno((const uv_handle_t*)udpHandle, &fd);

// Set ToS/DSCP:
if (setsockopt(sock, IPPROTO_IP, IP_TOS,  &tos, sizeof(tos)) < 0) {
    fprintf(stderr,"Error setting TOS: %s\n", strerror(errno));
}

// Try to send the packet synchronously:
uv_udp_try_send(....);

// It fails, so send it asynchronously:
uv_udp_send(....);

This is: before sending each packet I need to call setsockopt() with the corresponding DSCP/ToS value for that packet. However, since I may need to use uv_udp_send() it may happen that, when libuv finally sends the RTP audio packet, the socket DSCP setting may has been modified again due to a new call to setsockopt() for a different packet to be sent.

As suggested by Santiago in the mailing list, the uv_udp_send_t struct (and so the uv_write_t struct) may hold a before_send callback so that would give the app a chance to run setsockopt().

Does it seem feasible?

@saghul
Copy link
Member

saghul commented Nov 7, 2019

I think the right way to do this is to use sendmsg in the ancilliary data msg_control you can set the TOS per packet. See an example here: https://stackoverflow.com/questions/42629200/sendmsg-fails-with-invalid-argument-while-sending-udp-packet-with-ip-tos-ancil

@ibc
Copy link
Contributor Author

ibc commented Nov 7, 2019

Ok, so then uv_udp_send, uv_udp_try_send, uv_write and uv_try_write should be extended with such a msg_control data, right?

@saghul
Copy link
Member

saghul commented Nov 7, 2019

Heh, well, that's easier said than done, of course. For one, windows doesn't have sendmsg, and WSASendMsg is not it.

@santigimeno
Copy link
Member

WSASendMsg is not it.

Just curious, why is that? Looking at the documentation it seems it could be used to send control data using the WSAMSG structure.

@saghul
Copy link
Member

saghul commented Nov 8, 2019

Oops, you're totally right. It doesn't appear to support IP_TOS, but I guess that's anoher story.

@santigimeno
Copy link
Member

santigimeno commented Nov 8, 2019

It doesn't appear to support IP_TOS, but I guess that's anoher story.

I would say it does: https://docs.microsoft.com/en-us/windows/win32/winsock/ipproto-ip-socket-options

@santigimeno
Copy link
Member

Sorry, you're right. It says: 'Do not use.'

@ibc
Copy link
Contributor Author

ibc commented Nov 12, 2019

So, what would be the way to go?

  1. Extend uv_udp_send, uv_udp_try_send, uv_write and uv_try_write with msg_control info.
  2. Extend uv_udp_send_t and uv_write_t structs with a before_send and before_write optional callback.

?

@saghul
Copy link
Member

saghul commented Nov 12, 2019

Option 1) would have higher chances of success because you could create _ex variants that are backwards compatible. Option 2) would break the ABI.

@ibc
Copy link
Contributor Author

ibc commented Nov 12, 2019

Makes sense. I'll work in a PR in the next weeks.

@ibc
Copy link
Contributor Author

ibc commented Nov 12, 2019

Excuse me, does not 1) break the current API (not ABI)?

@saghul
Copy link
Member

saghul commented Nov 13, 2019

No, if you create new functions.

@santigimeno
Copy link
Member

santigimeno commented Nov 13, 2019

@saghul, a couple of questions:

  • wrt option 1, when creating the new _ex variants won't you be hitting the same issue in the case of sending the data asynchronously, meaning you'd have to store the msg_control info in the uv_udp_send_t request so it's available when actually calling sendmsg? or are you implying creating a new request type?
  • Regarding the ABI compatibility, hypothetically if someone would want to extend uv_udp_send_t, or any other request for the matter, would it be acceptable using this reserved field?

Thanks!

@saghul
Copy link
Member

saghul commented Nov 13, 2019

Yeah, we could indeed use the reserved fields for that.

@ibc
Copy link
Contributor Author

ibc commented Nov 13, 2019

No, we can't. For this feature, libuv should call a before_send cb placed into that ex by the app, so it should be a dedicated slot in the req struct.

@stale
Copy link

stale bot commented Jan 13, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 13, 2020
@stale stale bot closed this as completed May 13, 2020
@ibc
Copy link
Contributor Author

ibc commented May 13, 2020

Thanks, bot.

@misi
Copy link

misi commented May 14, 2021

I am wondering about this issue. Is it closed because of lost of interest or just lack of time? I guess the second.
I am wondering if there is still interest to implement DSCP marking in mediasoup?

AFAIK home WIFI routers implemented WMM and it is by default on, and it adds priority to media traffic if it is marked
e.g. EF(audio) or AF41(video).
Of course we can do as workaround, to mark all audio/video mediasoup media ports traffic with one DSCP from linux firewall nftables/iptables but it is somehow not right.
This way I still think it would be better to mark audio and video from mediasoup with different DSCP, and so I still hope it is still planed to be implement it in mediasoup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants