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

feature: upgrade to use udp unicast #73

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

gilbert-sci
Copy link
Collaborator

@gilbert-sci gilbert-sci commented Mar 5, 2025

Summary

In preparation for switching to UDP unicast, we have updated the StreamOut configuration to support udp unicast

Changes

  • Updates synapse api
  • Modularizes packet loss/bitrate calculation in streaming
  • Updates stream out to use Udp unicast

Migration

StreamOut configurations can just be defined like this (with an empty configuration for stream_out)

    {
      "type": "kStreamOut",
      "id": 1,
      "stream_out": {
      }
    },

The client will automatically fill with the clients IP and a default streaming port. You can also set a different port yourself, like this:

    {
      "type": "kStreamOut",
      "id": 1,
      "stream_out": {
        "udp_unicast": {
          "destination_port": 12345
        }
      }
    },

Specific IP addresses will not work, unless it is your own. In most cases, just let the client figure out the correct IP/Port to use.

Testing

  • Tested streaming from a device with the updated server
  • Tested running the example on a device with an updated server
  • Tested with starting/reading from synapse-sim
    • e.g. synapse-sim --rpc-port 50098

@gilbert-sci gilbert-sci self-assigned this Mar 5, 2025
@polymerizedsage
Copy link
Contributor

I have been doing some testing with this branch and the SciFi UDP unicast branch. A pain point I noticed is the inclusion of a destination IP in the config means that configs are not host-agnostic. If I change wireless networks or want to share the config to someone else, the destination IP needs to be updated. Perhaps the CLI can handle setting this field to the local IP address?

Ill wait till you are ready for review for any other comments.

@gilbert-sci gilbert-sci force-pushed the gilbert/udp-unicast branch from 35caa8b to 449d7ac Compare March 10, 2025 17:55
@gilbert-sci gilbert-sci changed the title draft: feature: upgrade to use udp unicast feature: upgrade to use udp unicast Mar 10, 2025
self.logger.info(
f"created multicast socket on {self.socket}, group {config.multicast_group}"
)
self.__socket.bind((dest_address, dest_port))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we don't need to bind to send, and we don't want to bind to the dest port if we want to receive on the same machine.

else:
self.__destination_port = destination_port

def read(self) -> Tuple[Optional[SynapseData], int]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

While you're here, technically this should be

Suggested change
def read(self) -> Tuple[Optional[SynapseData], int]:
def read(self) -> Tuple[Optional[Tuple[NDTPHeader, SynapseData]], int]:

(_unpack's signature is wrong as well)

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

Successfully merging this pull request may close these issues.

3 participants