-
-
Notifications
You must be signed in to change notification settings - Fork 366
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
Conversation
There was a problem hiding this 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.
src/rembhandler.cpp
Outdated
uint8_t payload_type = header->payloadType(); | ||
|
||
if (payload_type == 206 && header->reportCount() == 15) { | ||
auto remb = reinterpret_cast<RtcpRemb *>(message->data() + offset); |
There was a problem hiding this comment.
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".
src/rembhandler.cpp
Outdated
|
||
if (payload_type == 206 && header->reportCount() == 15) { | ||
auto remb = reinterpret_cast<RtcpRemb *>(message->data() + offset); | ||
mOnRemb(remb->getNumSSRC(), remb->getBitrate()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
off topic. Line 548 in 1787735
compiler will be optimize this code ? while (in_bitrate > pow(2, 18) - 1) {
maybe change to |
Indeed, it should be changed. Could you do it please? |
There was a problem hiding this 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" ?
I meant not merging master but actually rebase and then force push to clean up the branch history. |
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. |
Looking good, thank you! |
Added a media handler for receive the goog-remb bitrate from the client.