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

Add Onvif backchannel support #91

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cody-the-casual-dev
Copy link

This PR is an attempt to start addressing #35. I'm learning Rust and ONVIF/RTSP, so this is a very rough starting draft. However, I was hoping to get some early feedback and direction.

Is this close to the approach you had in mind? Instead of creating a SendingPacket type, should I just be leveraging rtsp-types::Message?

I'm also having trouble figuring out the best way to actually get payload into a message to send back.

I would greatly appreciate any and all critical feedback.

@scottlamb
Copy link
Owner

Awesome!

Is this close to the approach you had in mind? Instead of creating a SendingPacket type, should I just be leveraging rtsp-types::Message?

Yes, this looks on-track. A new type like SendingPacket sounds good. (And to be able to send whole frames, a matching SendingCodecItem or something at the Demuxed layer, but you might want to start with the packet-level stuff and add the higher-level thing later.) I don't think leveraging rtsp_types::Message would make sense for several reasons:

  • The caller doesn't know a lot of the details that get stuffed in there, such as the RTP payload type, RTSP channel number, and RTSP headers (cseq, user-agent, authorization). It'd be confusing for them to have to put in dummy values and then have retina clear them out and add in its own stuff.
  • If using Transport::Udp, these messages don't go over the RTSP stream at all, so even using an RTSP type at all would be confusing.
  • The rtsp_types crate isn't exposed in Retina's public API. I'd prefer to keep it that way. I wouldn't rule out switching to our own RTSP code some day for consider more efficient buffering model #6 or some other reason.
  • The caller shouldn't have to serialize a RTP packet header either, which would be required to make the body of a rtsp_message::Data. They should be able to just specify the payload and timestamp and have retina take care of the protocol details.

I'm also having trouble figuring out the best way to actually get payload into a message to send back.

Have you seen retina::rtp::RawPacketBuilder? You'll want to make a RTP packet with that, and then stuff that into a RTSP data message (for Transport::Tcp) or directly into a UDP packet (for Transport::Udp). Ideally we'd support both, but I think it'd be fine to start with just one, returning a not supported error if the caller tries to set things up for the other.

@cody-the-casual-dev
Copy link
Author

Thanks a ton! I had started on this work a few months ago, but had somewhat stalled. I won't promise a quick turnaround, but your feedback is helpful and encouraging.

Yes, this looks on-track. A new type like SendingPacket sounds good. (And to be able to send whole frames, a matching SendingCodecItem or something at the Demuxed layer, but you might want to start with the packet-level stuff and add the higher-level thing later.) I don't think leveraging rtsp_types::Message would make sense for several reasons:

  • The caller doesn't know a lot of the details that get stuffed in there, such as the RTP payload type, RTSP channel number, and RTSP headers (cseq, user-agent, authorization). It'd be confusing for them to have to put in dummy values and then have retina clear them out and add in its own stuff.
  • If using Transport::Udp, these messages don't go over the RTSP stream at all, so even using an RTSP type at all would be confusing.
  • The rtsp_types crate isn't exposed in Retina's public API. I'd prefer to keep it that way. I wouldn't rule out switching to our own RTSP code some day for consider more efficient buffering model #6 or some other reason.
  • The caller shouldn't have to serialize a RTP packet header either, which would be required to make the body of a rtsp_message::Data. They should be able to just specify the payload and timestamp and have retina take care of the protocol details.

Those all make sense to me!

Have you seen retina::rtp::RawPacketBuilder? You'll want to make a RTP packet with that, and then stuff that into a RTSP data message (for Transport::Tcp) or directly into a UDP packet (for Transport::Udp). Ideally we'd support both, but I think it'd be fine to start with just one, returning a not supported error if the caller tries to set things up for the other.

I had not seen that, I'll take a look.

@scottlamb
Copy link
Owner

Thanks a ton! I had started on this work a few months ago, but had somewhat stalled.

I know that story all too well. No pressure from me to complete it quickly, and I'm happy to review and give guidance.

@scottlamb scottlamb marked this pull request as draft January 9, 2024 05:13
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