Fix SimulcastConsumer fails to switch spatial layers

[satoren]

Fix #990

#990 (comment)

It appears #825 was not fully fixed.

[ibc]

Is it still a draft PR? or can we review it already?

[satoren]

@ibc
I suspect that the CI error that is occurring #992 will also occur here, so I will make ready after that is resolved.

[satoren]

It's already late in my time zone, so tomorrow

[jmillan]

I believe the one below is a more concise and less error prone fix:

This way we are avoiding the buggy scenario by only setting the syncRequired and spatialLayerToSync variables for the packet that will actually sync the stream.

Before this change we would set those flags for a non keyframe packet (even if it belonged to a target spatial layer) so the flags would remain set and the packet would be dropped.

This solution is more consistent and robust.

Nice catch @satoren!

diff --git a/worker/src/RTC/SimulcastConsumer.cpp b/worker/src/RTC/SimulcastConsumer.cpp
index 100f86c8d..70bd13012 100644
--- a/worker/src/RTC/SimulcastConsumer.cpp
+++ b/worker/src/RTC/SimulcastConsumer.cpp
@@ -660,12 +660,14 @@ namespace RTC

                // Check whether this is the packet we are waiting for in order to update
                // the current spatial layer.
-               if (this->currentSpatialLayer != this->targetSpatialLayer && spatialLayer == this->targetSpatialLayer)
+               // clang-format off
+               if (
+                       this->currentSpatialLayer != this->targetSpatialLayer &&
+                       spatialLayer == this->targetSpatialLayer &&
+                       packet->IsKeyFrame()
+               )
+               // clang-format on
                {
-                       // Ignore if not a key frame.
-                       if (!packet->IsKeyFrame())
-                               return;
-
                        shouldSwitchCurrentSpatialLayer = true;

                        // Need to resync the stream.

[jmillan]

@satoren, could you please try it whenever suits you?

[satoren]

@jmillan Thank you. Seems to be working fine.

[ibc]

Guys I'm looking at the changes and rationale and I don't understand which behavior changes here or how it fixes the problem. I'm not saying it's wrong. May you please explain the scenario those changes fix?

[jmillan]

May you please explain the scenario those changes fix?

It's explained here

[jmillan]

May you please explain the scenario those changes fix?

Definitely my patch is totally a placebo. It changes nothing.., it just move a condition up to the parent conditional statement. What was I thinking about...

[ibc]

May you please explain the scenario those changes fix?

Definitely my patch is totally a placebo. It changes nothing.., it just move a condition up to the parent conditional statement. What was I thinking about...

Let' see:

Scenario 1 (non keyframe received)

• currentSpatialLayer is 1
• targetSpatialLayer is 2
• spatial layer of received packet is 2
• received packet is not keyframe

In v3:

1. We enter condition 1 block.
2. Since it's not a key frame (immediate condition within the block), we return early.
3. So no flag was changed and packet was discarded.

In this PR:

1. We don't enter condition 1 block (since it's not a keyframe).
2. We enter condition 2 block (since spatial layer of the packet is not the same as current spatial layer), so we return early.
3. So no flag was changed and packet was discarded.

So no changes in Scenario 1 between v3 and this PR.

Scenario 2 (keyframe received)

• currentSpatialLayer is 1
• targetSpatialLayer is 2
• spatial layer of received packet is 2
• received packet is keyframe

In v3:

1. We enter condition 1 block.
2. It's a key frame (immediate condition within the block).
3. So we change flags and keep processing the packet.

In this PR:

1. We enter condition 1 block.
2. So we change flags and keep processing the packet.

So no changes in Scenario 2 between v3 and this PR.

Scenario 3 (non keyframe received II)

• currentSpatialLayer is 1
• targetSpatialLayer is 2
• spatial layer of received packet is 1
• received packet is not keyframe

In v3:

1. We don't enter condition 1 block.
2. We don't enter condition 2 block (since spatial layer of the packet matches currentSpatialLayer).
3. So we don't change flags and keep processing the packet.

In this PR:

1. We don't enter condition 1 block (since it's not a keyframe).
2. We don't enter condition 2 block (since spatial layer of the packet matches currentSpatialLayer).
3. So we don't change flags and keep processing the packet.

So no changes in Scenario 3 between v3 and this PR.

Conclusion

Unless I miss some scenario, I don't see how changes in this PR can change anything.

[ibc]

Let's close this PR since, as proven above, it cannot fix anything. Alternative PR here #999