ultravideo / uvgRTP

An open-source library for RTP/SRTP media delivery
BSD 2-Clause "Simplified" License
296 stars 84 forks source link

rtcp: add hooking for sending APP packets #187

Closed wowaser closed 1 year ago

wowaser commented 1 year ago

I started from scratch on a new branch. This is my new proposition

wowaser commented 1 year ago

I took into account your considerations from this comment: https://github.com/ultravideo/uvgRTP/pull/168#issuecomment-1270144281

jrsnen commented 1 year ago

Hi!

This already looks very good, but I still have a couple of points: 1) This public API in this PR is referencing a data structure which is not part of the public API (rtcp_app_packet), which does compile because it is in the header, but it should then be made part of the public API for frames, which resides in frame.hh. There however already exists and APP packet which is used at the receiver, so that does not seem like a good idea. 2) install_outgoing_app_hook requires app name, but this is only used as an identifier and not as the actual name field in the APP packet sent. This should be at least clarified. 3) would install_send_app_hook be a better name?

As for solutions to 1) and 2), I would probably modify the install function so that it takes the name and subtype as parameters, and have the installed function somehow return both the len and payload fields (one or two reference parameters are probably needed). After calling the function, len should be checked that it follow specification (divisible by 4). For 2), the name and subtype should probably be used as the identifier for deletion and overwriting, although the specification is not very clear on subtype usage.

I would also not define an additional helper function in rtcp_packets, since they hide the actual structure of the packet, but I can also change this afterward since it is not part of the public API.

Extra std::cout is still left in code.

BR, Joni

jrsnen commented 1 year ago

@wowaser if you are wondering about the extra commit, we decided to finalize this PR with an additional commit, before merging. I was instructing the changes. We plan to do an RTCP related release soon and this would be a nice addition to it. Hopefully this is ok.

BR, Joni