retina icon indicating copy to clipboard operation
retina copied to clipboard

Add Onvif backchannel support

Open cody-the-casual-dev opened this issue 1 year ago • 3 comments

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.

cody-the-casual-dev avatar Jan 02 '24 16:01 cody-the-casual-dev

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 #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.

scottlamb avatar Jan 02 '24 21:01 scottlamb

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.

cody-the-casual-dev avatar Jan 03 '24 00:01 cody-the-casual-dev

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 avatar Jan 03 '24 02:01 scottlamb