Fix VP9 out of order packets forwarding

[vpalmisano]

Hi 👋
When I run different tests with 1 VP9 video publisher with network packet loss (10-15%) and delay (100-500ms) at sender side, I always measure a low video framerate (~3-4fps at average) at receiver side.

Looking at the VP9 Process method, I see that when we receive an out-of-order packet, we forward it without checking if it satisfies the current spatial and temporal layer values. As opposite, dropping those out of order packets in some cases improves the measured fps at receiver side.

This PR tries uses an intermediate approach, saving the spatial/temporal and pictureID relation in a list when the current layers change. When an old packet arrives, we search for the spatial/temporal layer for the packets pictureID (getting the layer that was active for the given pictureID) and drop the packet if its layer is greater than that. This way we are sure that we forward the packet only if it would have been forwarded did it arrive in order.
This prevents forwarding packets that would later be discarded by the decoder, and that would potentially generate over-processing, overflowing the jitter buffer and hence discarding legitimate packets, etc.

Tests results in the same conditions show an increase of the measured fps from ~3-4 to 7-8fps.

Ref:
similar PR for VP8: #1009

[ibc]

Thanks. Please give us some days before we review (terribly busy these days). And will also review the VP8 related PR.

[jmillan]

@vpalmisano, in #1009 we simply discard packets which temporal layer is higher than the current one. Why don't we follow the same approach here? I seems way more simple.

If the current spatial layer is N, then we know that the receiver is already ready to receive it, and receiving an N+1 old packet won't have any benefit as it will be dropped by the decoder.

[vpalmisano]

@vpalmisano, in #1009 we simply discard packets which temporal layer is higher than the current one. Why don't we follow the same approach here? I seems way more simple.

If the current spatial layer is N, then we know that the receiver is already ready to receive it, and receiving an N+1 old packet won't have any benefit as it will be dropped by the decoder.

Ideally yes, but the receiver could use the jitterbuffer the keep the latest N packets and in this case it would be better to keep receiving the correct layers instead of immediately switch the to the new layer.

[jmillan]

Ideally yes, but the receiver could use the jitterbuffer the keep the latest N packets and in this case it would be better to keep receiving the correct layers instead of immediately switch the to the new layer.

But since we are only changing layers with key frames, loss of previous packets won't generate issues because corresponding keyframe for the new layer is already in the jitterbuffer too. Ie:

The jitterBuffer in the client is as follows:

| layer-1 (seq-4) "keyframe (ksvc) or end of frame" |
| layer-2 (seq-2) |
| layer-2 (seq-1) |

The packet related to "layer-2 (seq-3)" is missing BUT "layer-1 (seq-4)" is a keyframe or an end of a frame, hence not receiving (seq-3) should not have a negative effect.

Can you please @vpalmisano try with this much simple approach and share the insigths?

[vpalmisano]

But since we are only changing layers with key frames, loss of previous packets won't generate issues because corresponding keyframe for the new layer is already in the jitterbuffer too. Ie:

Consider this scenario:

-> currentLayer=S1
pictureID_1: S1 -> forward
pictureID_1: S2 -> drop
pictureID_3: S1 -> forward
-> currentLayer=S2
pictureID_4: S2 -> forward
pictureID_2: S1 -> ?
pictureID_2: S2 -> ?

Here we can avoid dropping pictureID_2 and we can forward S1 layer, because if was the current selected layer since pictureID_1. If we drop pictureID-2 we will create an hole in the jitterbuffer, right?

[jmillan]

Here we can avoid dropping pictureID_2 and we can forward S1 layer, because if was the current selected layer since pictureID_1. If we drop pictureID-2 we will create an hole in the jitterbuffer, right?

We are talking about filtering layers greater than the current one, not the lower ones, hence both packets would be delivered in this case, right?

[vpalmisano]

We are talking about filtering layers greater than the current one, not the lower ones, hence both packets would be delivered in this case, right?

With the current code, old packets will be delivered at both S1 and S2 layers.
After the changes, only S1 will be delivered (I'm assuming that pictureID_2 is not a keyframe).

[jmillan]

Looking at the VP9 Process method, I see that when we receive an out-of-order packet, we forward it without checking if it satisfies the current spatial and temporal layer values. As opposite, dropping those out of order packets in some cases improves the measured fps at receiver side.

I fail to see why sending old packets would make the reception fps drop. Do you know a reason (in the codec spec or in the browser implementation) for this to happen?

[vpalmisano]

I fail to see why sending old packets would make the reception fps drop. Do you know a reason (in the codec spec or in the browser implementation) for this to happen?

I have only some hypothesis:

1. There is an high probability that the packet at S0 layer is both a start and an end of picture packet, so the decoder will prefer to decode it discarding the higher layer packets instead of waiting for them to form a complete picture.
2. Putting uncompleted pictures in the decoder buffer probably causes more RTX and PLIs requests.

[jmillan]

Some comments made. The idea looks fine @vpalmisano 👌

I'll re-review after the requested changes are done

[ibc]

Some cosmetic changes requested.

[jmillan]

@vpalmisano, for future reading of this PR, could you please update the PR summary block:

This PR tries to use a intermediate approach, saving the spatial/temporal layer changes with the corresponding pictureID in a list. When an old packet arrives, we decide to drop it or not searching the corresponding spatial/temporal layer value, this way we are sure that we forward the right layer as requested since that pictureID.

by:

This PR tries uses an intermediate approach, saving the spatial/temporal and pictureID relation in a list when the current layers change. When an old packet arrives, we search for the spatial/temporal layer for the packets pictureID (getting the layer that was active for the given pictureID) and drop the packet if its layer is greater than that. This way we are sure that we forward the packet only if it would have been forwarded did it arrive in order.

This prevents forwarding packets that would later be discarded by the decoder, and that would potentially generate over-processing, overflowing the jitter buffer and hence discarding legitimate packets, etc.