-
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
Handler to filter packets from higher quality layers #775
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.
LGTM! Nice job. Some questions inside
|
||
void QualityFilterHandler::handleFeedbackPackets(std::shared_ptr<dataPacket> packet) { | ||
RtpUtils::forEachRRBlock(packet, [this](RtcpHeader *chead) { | ||
RtpUtils::updateREMB(chead, 400000); |
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 400000? Even if we have proper feedback soon, it probably would make more sense to use maxVideoBW
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.
you got it!
namespace erizo { | ||
|
||
class RtpUtils { | ||
public: | ||
static bool sequenceNumberLessThan(uint16_t first, uint16_t second); | ||
|
||
static void forEachRRBlock(std::shared_ptr<dataPacket> packet, std::function<void(RtcpHeader*)> f); |
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.
👍 to this... very helpful
checkSSRCChange(ssrc); | ||
rtp_header->setSSRC(video_sink_ssrc_); | ||
|
||
if (packet->compatible_spatial_layers.back() == target_spatial_layer_ && packet->ending_of_layer_frame) { |
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.
If I understand this right, if we put this after current line 95, we won't be setting the marker for packets that we're going to discard anyway
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.
👍
Description
This PR adds a new handler to drop packets that belong to spatial/temporal layers that we don't want to forward to the subscribers.
Changes in the Client API
It also adds a mechanism to choose the spatial and temporal layer from the client (stream._setQualityLayer(...)). The method is prefixed with underscore because the feature is not finished yet and we don't want people to start using it in production yet.
I'll update client's documentation once we make the API public.