Skip to content
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

Fix lack of "layerschange" event when all streams in the producer die #1122

Conversation

ibc
Copy link
Member

@ibc ibc commented Jul 19, 2023

The initial purpose of this PR was to JUST enable RTP inactivity check in simulcast streams (this was already done in PR #1061) and just if the video has more than 1 single stream (remember that if the producer video is VP8/H264 with a single stream/encoding with N temporal layers, it generates a SimulcastConsumer). And this has been done already. However I've found another issue (regression that happened no idea when).

So here the problem:

  • Simulcast with > 1 streams.
  • So RTP inactivity checks are enabled in all streams.
  • All Producer streams die so RTP inactivity happens in all them.
  • SimulcastConsumer::ProducerRtpStreamScore() is called in all them with score: 0.
  • Such a method should trigger 'consumerlayerschange' with spatialLayer: null and temporalLayer: null, but it doesn't (pretty sure it did in the past).
  • Why? Because SimulcastConsumer::ProducerRtpStreamScore() ends calling this->MayChangeLayers() which ends calling this->listener->OnConsumerNeedBitrateChange().
  • Transport::OnConsumerNeedBitrateChange() ends calling this->DistributeAvailableOutgoingBitrate() which calls consumer->GetBitratePriority().
  • SimulcastConsumer::GetBitratePriority() checks this->IsActive() and it returns false so Transport forgets about this consumer and won't call IncreaseLayer() etc, so the consumer won't have a chance to notify null layers.
  • SimulcastConsumer::IsActive() returns false because it's check the score of all the streams in the producer and, if all 0, it returns false.
  • So here the problem. Not sure how to resolve it. But I did: https://github.com/versatica/mediasoup/pull/1122/files#diff-24e17490eee048f39100d99aee1f3b0f842a73ebca6128162e3e838443c07b4bR1336

…ms in the producer die

The initial purpose of this PR was to JUST enable RTP inactivity check in simulcast streams (this was already done in PR #1061) and **just** if the video has more than 1 single stream (remember that if the producer video is VP8/H264 with a single stream/encoding with N temporal layers, it generates a `SimulcastConsumer`). And this has been done already. However I've found another issue (regression that happened no idea when).

So here the problem:

- Simulcast with > 1 streams.
- So RTP inactivity checks are enabled in all streams.
- All Producer streams die so RTP inactivity happens in all them.
- `SimulcastConsumer::ProducerRtpStreamScore()` is called in all them with `score: 0`.
- Such a method should trigger 'consumerlayerschange' with `spatialLayer: null` and `temporalLayer: null`, but it doesn't (pretty sure it did in the past).
- Why? Because `SimulcastConsumer::ProducerRtpStreamScore()` ends calling `this->MayChangeLayers()` which ends calling `this->listener->OnConsumerNeedBitrateChange()`.
- `Transport::OnConsumerNeedBitrateChange()` ends calling `this->DistributeAvailableOutgoingBitrate()` which calls `consumer->GetBitratePriority()`.
- `SimulcastConsumer::GetBitratePriority()` checks `this->IsActive()` and it returns `false` so `Transport` forgets about this consumer and won't call `IncreaseLayer()` etc, so the consumer won't have a chance to notify null layers.
- `SimulcastConsumer::IsActive()` returns false because it's check the score of all the streams in the producer and, if all 0, it returns `false`.
- So here the problem.
@ibc ibc requested a review from jmillan July 19, 2023 12:38
@ibc ibc marked this pull request as ready for review July 19, 2023 13:11
@ibc ibc changed the title Attempt to fix lack of "consumerlayerschange" event when it all streams in the producer die Fix lack of "consumerlayerschange" event when all streams in the producer die Jul 19, 2023
@jmillan
Copy link
Member

jmillan commented Jul 19, 2023

SimulcastConsumer::GetBitratePriority() checks this->IsActive() and it returns false so Transport forgets about this consumer and won't call IncreaseLayer() etc, so the consumer won't have a chance to notify null layers.

How can it not be active at this point? MayChangeLayers() is only called if the Consumer is active.

@ibc
Copy link
Member Author

ibc commented Jul 19, 2023

Because all the streams of the producer now have score 0 so SimulcastConsumer::IsActive() returns false. And because I think we call MayChangeLayers() by just checking if parent Consumer::IsActive() which is true.

@jmillan
Copy link
Member

jmillan commented Jul 19, 2023

I wonder if the following change in SimulcastConsumer::ProducerRtpStreamScore would be more clear and explicit:

diff --git a/worker/src/RTC/SimulcastConsumer.cpp b/worker/src/RTC/SimulcastConsumer.cpp
index 3527f3c0f..a5d4a44aa 100644
--- a/worker/src/RTC/SimulcastConsumer.cpp
+++ b/worker/src/RTC/SimulcastConsumer.cpp
@@ -324,9 +324,14 @@ namespace RTC

                if (RTC::Consumer::IsActive())
                {
+                       // All Producer streams are dead.
+                       if (!IsActive())
+                       {
+                               UpdateTargetLayers(-1, -1);
+                       }
+
                        // Just check target layers if the stream has died or reborned.
                        // clang-format off
-                       if (
+                       else if (
                                !this->externallyManagedBitrate ||
                                (score == 0u || previousScore == 0u)

EDIT: diff lines may not match with 'v3' since I was in a different branch.

@ibc
Copy link
Member Author

ibc commented Jul 19, 2023

@jmillan pushed here. Tested. It works. 331d42b

@ibc ibc merged commit 2f6bfca into v3 Jul 19, 2023
@ibc ibc deleted the do-not-run-rtp-inactivity-check-on-single-streams-with-n-temporal-layers branch July 19, 2023 14:25
@ibc ibc changed the title Fix lack of "consumerlayerschange" event when all streams in the producer die Fix lack of "layerschange" event when all streams in the producer die Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants