-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
VP8: do not forward RTP packets which payload contains a higher temporal layer than current one. #1009
base: v3
Are you sure you want to change the base?
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.
Will take a deeper look on Monday. Question: doesn't this affect H264 or VP9?
I presume so, yes. I'm looking for side effects for this changes before applying them elsewhere. So far it works as expected. |
This is the only delicate scenario that I can think of. Ie: We are forwarding temporal layer 1 and the following payloads arrive:
Observations:
|
At this point if the app decides to set the temporal layer to 1 again:
|
@@ -325,7 +325,7 @@ namespace RTC | |||
// clang-format off | |||
if ( | |||
this->payloadDescriptor->hasTlIndex && | |||
this->payloadDescriptor->tlIndex > context->GetCurrentTemporalLayer() | |||
this->payloadDescriptor->tlIndex == context->GetTargetTemporalLayer() |
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 we need to keep the packets with temporal layer <= target ?
this->payloadDescriptor->tlIndex == context->GetTargetTemporalLayer() | |
this->payloadDescriptor->tlIndex <= context->GetTargetTemporalLayer() |
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.
We must only upgrade the temporal layer with a packet whose temporal layer is the target one.
What about if we allow packets with higher layers be forwarded only if their PID value is between S_PID - 10 and S_PID being S_PID the PID of the packet we used to switch current layers? |
Packets with lower TID than the current are always forwarded, since current temporal layer depends on them.
If this is packet can be used to change to temporal layer 1 then the current temporal layer is updated and packet is forwarded. |
I'd avoid creating this kind of logic before knowing that the current proposal does not work as expected. |
You mean current temporal layer is changed in the Consumer or in the packet context? Or in both? |
Wouldn't this solution create additional PLIs and keyframes if the network between the Sender and the SFU is lossy? Example: Given a steady state: no layer switching, current TL is 0, we filter out packets with TID > 0. WIth this PR this could happen:
In this case, the receiver will never receive the packet with SN: 1, and it will send NACKs with no retransmission (as we dropped the packet), and it will end up sending a PLI. And this would happen to all receivers. Something like what @ibc mentions above would reduce its impact I think. That said, I think that forwarding old packets with higher temporal layers could also break the video stream at the receiver side, but I haven't been able to demonstrate it so far :). |
Before going forward with this draft I want to make sure what's causing the problem for #989. There, we can see that some Consumers are continuously receiving the T1 layer when they should receive the T0 only, and that extra T1 traffic cannot happen due to old packets (which is what this branch aims to avoid). I have created a branch which logs info about each outgoing VP8 payloads. We'll try to repro #989 in the next days and see what is really happening. |
Idea: let old packets with temporal layer greater than current temporal layer pass to the consuming endpoint UNTIL a keyframe (for the target spatial layer) is sent to it. Once a keyframe is received by the consuming endpoint there is no reason for the endpoint to request any keyframe via PLI. |
Yep, that would make it. EDIT: Indeed I can't see at the moment how it would do it. Can you expose it within a flow in order to understand it. Ie: #1009 (comment)? |
It was a conceptual idea. I cannot correlate it with real changes in current code. Also, now I think it doesn't make sense. Imagine we are in spatial layer 1 all the time and suddenly we make temporal layer 0 the preferred one. At this point the issue described in this ticket may happen depending on circumstances (packet loss etc) but there is no keyframe involved at all since we have never switched the spatial layer. So ignore it please. |
Sorry I'm late to the party. I don't think this change will help with #989 but any improvement in the layers forwarding that could help mitigate the (infrequent) decoding video artefacts we have seen it is welcomed. This change looks good to me. At east the idea described, I havent' reviewed the implementation. I think in the example shared by @jcague the browser will look at the |
I think the answer is no, but... in this scenario can the SimulcastConsumer know in advance (by looking at PID or something else) them at the lost packet with SN 1 belongs to TL:1 so it must be discarded and hence packet with SN:2 is sent to the consulting endpoint with SN:1 instead? Pretty sure this is not possible so here another crazy proposal: In this scenario, when delayed SN:1 is finally received by SimulcastConsumer and the device has sent NACK for SN:1, could SimulcastConsumer send an empty packet with SN:1? I mean, a packet with no payload so it would be silently discarded by the consuming device. |
@ggarber not sure if I follow, there is no nice approach here but just 2 proposals with their own drawbacks:
|
0.1% higher bitrate imo.
I was suggesting to go with Option 2. The PLIs in the receiver side are not triggered based on packet loss but on not having decodeable frames for 2secs. In this case there will be always decodeable frames from layer 0 so there shouldn't be any extra PLIs/keyframes. |
Oh, it makes sense. Then... this PR is good (assuming code does what it's supposed to do), right? |
I didn't check the implementation but the concept of the PR is good imo, yes. |
Requires further testing. Not sure yet whether it addresses the intended issue. |
Self note: Retest locally and prepare for merge. |
@namello-gather, was this a concerning issue for your environment? If so, did it have a positive impact without drawbacks? |
Merge mediasoup issue versatica#1009, don't forward VP8 layers if not requested.
Motivation: #989, we have seen that we are sometimes sending more BW than provided by BWE.
We verified that
SimulcastConsumer
is properly selecting thetarget
spatial and temporal layers based on the available bitrate. We switch to thetarget
spatial layer as soon as we receive a keyframe from the corresponding stream. Such spatial layer becomes thecurrent
layer. From this point onwards, for the upcoming packets for the given stream we basically let the codec implementation decide whether they should be forwarded (based on thecurrent
andtarget
temporal layer) by callingRtpPacket::ProcessPayload()
which ends up calling the codec implementationProcess()
method.v3
is forwarding every old packet (which picureId is lower than the last forwarded), even those with a temporal layer greater than the current one.Example:
This means that, as shown in #989, we are sending more BW than the current available indicated by BWE (or app) and thus:
Since we respect the codec keyframes in order to update layers, I'm pretty confident we are not incurring in any issue by dropping packets which temporal layer is higher than the current one.
Can you see any drawbacks here @ibc @vpalmisano @ggarber @jcague ?