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

rtcp: add hooking for sending APP packets #187

Merged
merged 2 commits into from
Mar 13, 2023

Conversation

wowaser
Copy link
Contributor

@wowaser wowaser commented Dec 25, 2022

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

@wowaser
Copy link
Contributor Author

wowaser commented Dec 25, 2022

I took into account your considerations from this comment:
#168 (comment)

@jrsnen
Copy link
Member

jrsnen commented Jan 18, 2023

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
Copy link
Member

jrsnen commented Mar 6, 2023

@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

@tampsa tampsa merged commit 09094e2 into ultravideo:master Mar 13, 2023
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