-
Notifications
You must be signed in to change notification settings - Fork 1k
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 padding generator handler #794
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.
👍 Nice! Just minor comments
} | ||
|
||
void RtpPaddingGeneratorHandler::read(Context *ctx, std::shared_ptr<dataPacket> packet) { | ||
RtpUtils::forEachRRBlock(packet, [this](RtcpHeader *chead) { |
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.
I'm thinking we should move as much as we can to the stats. Here it makes sense to react as soon as we get the info but I keep thinking we should avoid parsing the same packets n times.
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.
👍
} | ||
first_packet_received_ = true; | ||
if (fast_start_ && clock_->now() - started_at_ > kFastStartMaxDuration) { | ||
ELOG_WARN("Fast start disabled"); |
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.
See log level comment above
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.
good catch! I used those while testing it manually
} | ||
} else if (fast_start_ && chead->packettype == RTCP_Receiver_PT && chead->getSourceSSRC() == audio_source_ssrc_) { | ||
if (chead->getFractionLost() > kMaxFractionLostAllowed) { | ||
ELOG_WARN("Fast start disabled"); |
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.
Probably DEBUG or INFO would be better here
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.
good catch!
@@ -59,3 +59,4 @@ log4j.logger.rtp.SenderBandwidthEstimationHandler=WARN | |||
log4j.logger.rtp.StatsCalculator=WARN | |||
log4j.logger.rtp.LayerDetectorHandler=WARN | |||
log4j.logger.rtp.PliPacerHandler=WARN | |||
log4j.logger.rtp.RtpPaddingGeneratorHandler=WARN |
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.
Can you please also add this to the tests log4cxx.properties?
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.
sure!
Description
As part of Simulcast/SVC it adds a new handler to generate padding. It will be useful when we need to increase bitrates and sender bandwidth estimations.
Changes in Client or Server public APIs
Not needed.
[] It includes documentation for these changes in
/doc
.