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

Implement bitrate controller in vpx and h264 codecs and use REMB RTCP messages to set bitrate naively #467

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KW-M
Copy link
Contributor

@KW-M KW-M commented Jan 20, 2023

Description

  • Implemented the BitRateController interface for the vpx and h264 codecs
  • Added very simple REMB based bit rate handling.
    This sets every track to use roughly all (hardcoded to 93%) the estimated available bandwidth.
    Currently it doesn't compensate for when other tracks or data channels are transmitting over the same bandwidth.
    I couldn't find an easy way to detect what other tracks & datachannels were in use over the same peer connection, so doing bit rate control in a smarter way is still an open problem (see rtcpReadLoop in track.go).

@codecov
Copy link

codecov bot commented Jan 20, 2023

Codecov Report

Base: 58.21% // Head: 58.41% // Increases project coverage by +0.19% 🎉

Coverage data is based on head (44a76f5) compared to base (f8f8511).
Patch coverage: 64.58% of modified lines in pull request are covered.

❗ Current head 44a76f5 differs from pull request most recent head 5478710. Consider uploading reports for the commit 5478710 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #467      +/-   ##
==========================================
+ Coverage   58.21%   58.41%   +0.19%     
==========================================
  Files          62       62              
  Lines        3683     3715      +32     
==========================================
+ Hits         2144     2170      +26     
- Misses       1412     1415       +3     
- Partials      127      130       +3     
Impacted Files Coverage Δ
track.go 26.38% <44.44%> (+1.06%) ⬆️
pkg/codec/x264/x264.go 68.75% <50.00%> (+3.63%) ⬆️
pkg/codec/vpx/vpx.go 83.50% <90.00%> (-0.02%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@EmrysMyrddin
Copy link
Contributor

Hi, thank you for your PR, it looks awesome !

While automatic bitrate adjustment is a great feature, it should probably be configurable. We should be able to at least disable it by default and chose the percentage of the detected maximum bandwidth.

Perhaps you could split this PR, letting us merge your bitrate controllers and discuss about how to implement the automatic bitrate adjustment in a dedicated PR ?

@KW-M
Copy link
Contributor Author

KW-M commented Jan 23, 2023

Thanks for the suggestions @EmrysMyrddin. I rolled back this PR to just include the bit-rate encoder controllers.

I'm thinking the place to implement the option for auto bit-rate control is in the RTPCodec struct at the beginning of codec.go, and then maybe allow setting the percentage bandwidth used per stream by passing the percentage as a mediaStreamConstraint in getUserMedia(). Does that seem good, or might there be a better way to do it?

Copy link
Contributor

@EmrysMyrddin EmrysMyrddin left a comment

Choose a reason for hiding this comment

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

LGTM, just a little question and we should be able to merge this :-)

@@ -78,6 +79,22 @@ Encoder *enc_new(x264_param_t param, char *preset, int *rc) {
return NULL;
}

#define RC_MARGIN 10000 /* 1kilobits / second*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this 10 kb/s ?

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.

None yet

2 participants