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 a media handler to respond to remb bitrate #1185

Merged
merged 4 commits into from
Jun 7, 2024

Conversation

evaldemar
Copy link
Contributor

@evaldemar evaldemar commented May 15, 2024

Added a media handler for receive the goog-remb bitrate from the client.

Copy link
Owner

@paullouisageneau paullouisageneau left a comment

Choose a reason for hiding this comment

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

Thank you for adding, I have a few comments. Sorry for the version.h failure mess, please rebase on current master.

uint8_t payload_type = header->payloadType();

if (payload_type == 206 && header->reportCount() == 15) {
auto remb = reinterpret_cast<RtcpRemb *>(message->data() + offset);
Copy link
Owner

Choose a reason for hiding this comment

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

You should check the size again before casting to RtcpRemb for the sake of completeness. You could also add a method to check that the identifier is indeed "REMB".


if (payload_type == 206 && header->reportCount() == 15) {
auto remb = reinterpret_cast<RtcpRemb *>(message->data() + offset);
mOnRemb(remb->getNumSSRC(), remb->getBitrate());
Copy link
Owner

Choose a reason for hiding this comment

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

Why do you pass the number of SSRCs (instead of the SSRCs themselves) in the REMB feedback message? It seems useless for the user. I think you should remove it from the callback signature. If I understand correctly, it is not necessary to pass SSRCs as the bitrate is the total bitrate for the RTP session anyway. The SSRCs should be used for dispatching across tracks in PeerConnection::dispatchMedia(), if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe. i don't understand well goog-remb, i think maybe its need for anything, i will change.

@evaldemar
Copy link
Contributor Author

off topic.
i don't understand one nuance.

while (in_bitrate > pow(2, 18) - 1) {

compiler will be optimize this code ?
while (in_bitrate > pow(2, 18) - 1) {

maybe change to
while (in_bitrate > 0x3FFFF) { ?

@paullouisageneau
Copy link
Owner

off topic. i don't understand one nuance.

while (in_bitrate > pow(2, 18) - 1) {

compiler will be optimize this code ?
while (in_bitrate > pow(2, 18) - 1) {

maybe change to while (in_bitrate > 0x3FFFF) { ?

Indeed, it should be changed. Could you do it please?

src/rtp.cpp Outdated Show resolved Hide resolved
Copy link
Owner

@paullouisageneau paullouisageneau left a comment

Choose a reason for hiding this comment

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

It looks good, thanks! Could you please rebase your branch on current master to remove bogus commits like "Bumped version to 0.21.1" ?

@paullouisageneau
Copy link
Owner

I meant not merging master but actually rebase and then force push to clean up the branch history.

@evaldemar
Copy link
Contributor Author

i,m sorry , i have been a little experience in pool request and git. usually i do pull, commit & push. I'll deal with it. today or tomorrow. and i'm sorry for my English.

src/rtp.cpp Outdated Show resolved Hide resolved
@paullouisageneau
Copy link
Owner

Looking good, thank you!

@paullouisageneau paullouisageneau merged commit eec049f into paullouisageneau:master Jun 7, 2024
12 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