Skip to content

Conversation

timmarkhuff
Copy link
Collaborator

@timmarkhuff timmarkhuff commented Oct 8, 2025

A refactor of RTSPServer to allow for one server will multiple streams.

Usage looks like this:

server = RTSPServer(port=port)
server.create_stream(get_frame_callback1, width, height, fps, mount_point='/stream0')
server.create_stream(get_frame_callback2, width, height, fps, mount_point='/stream1')
server.start()
time.sleep(n) # keep the server up 
server.stop()

This also fixes a bug where the server only allowed one client per stream. Most of the complexity of this PR is due to special handling to allow multiple clients.

I also added a test for RTSP server/client functionality. This required a little refactor of our CICD setup.

@timmarkhuff timmarkhuff requested a review from honeytung October 9, 2025 23:58
@@ -0,0 +1,34 @@
FROM ubuntu:22.04
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Factoring out our system dependencies for CICD into this dockerfile seemed like a good idea, but I'm curious what others think.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to me since we have additional dependencies to install now. Good call!

[tool.poetry]
name = "framegrab"
version = "0.13.2"
version = "0.14.0"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The changes to RTSPServer are breaking

Copy link
Member

@honeytung honeytung left a comment

Choose a reason for hiding this comment

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

LGTM! It would be great to update the README to include the new features but the implementation makes sense!

libgstreamer1.0-dev \
libgirepository1.0-dev \
gir1.2-gst-rtsp-server-1.0 \
gir1.2-gstreamer-1.0 \
Copy link
Member

Choose a reason for hiding this comment

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

Should probably include these in the README.md

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the readme

@@ -0,0 +1,34 @@
FROM ubuntu:22.04
Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to me since we have additional dependencies to install now. Good call!

@timmarkhuff timmarkhuff merged commit bc8b405 into main Oct 13, 2025
6 checks passed
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.

2 participants