-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
PLI dilution #3422
base: 0.x
Are you sure you want to change the base?
PLI dilution #3422
Conversation
@natikaltura you should change the target branch to 0.x |
@natikaltura thanks for the PRs! Answering here even though some considerations will apply to #3423 as well (where there's more changes due to the multiple streams). I think the patches can be simplified, since you're adding a new volatile that's probably not needed. The main problem is that the fence we currently have doesn't seem to be working well with the multistream nature of Janus (the same function is invoked by multiple threads), and mostly because we only update I don't like the change of the threshold from 100ms to 1 second. It will make responsiveness for new users (and simulcast changes) awful in some cases. While we can agree that 100ms may be too short (and probably the cause of your ~10 PLIs per second to the publisher), I don't see 1s as the answer, especially with the fence fix from the paragraph above. Coming to multistream, I don't see the point of adding a new function. I'd just add a new property to the existing function to dictate a potential different behaviour (e.g., -1 does what it did before, a positive integer only pokes the specific stream). Not even sure why we need two? If we're starting to do drill down PLIs, why not only do that?
As you noticed, that's because the simulcast code defaults to 250ms, but that's a (reasonable) default. You can override that with the |
@lminiero Thanks a lot for the comments! I’ll fix both PRs after getting your inputs about the following:
Thanks! I'll remove the sending_pli volatile from the 0.x stream since it seems unnecessary with the other two PR changes:
I originally copied the sending_pli volatile from the multistream implementation.
Just to emphasize the need for this fix, I have logs showing 200 and more PLIs per second (for each layer), especially with screen sharing on challenging networks :)
Sure. I will unite it to one function.
Thanks! I realize I missed the full picture... |
Fix: overflow of PLI requests to Publishers (especially with simulcast)
This is a PR for v0.14.3 branch (see PR# for the multistream)
We observed a significant issue while using the Janus Media Server on both the v0.14.3 branch and the v1.2.3 multistream branch. When VP8 simulcast is enabled with a group of viewers for a Publisher, the Publisher sometimes receives dozens of Picture Loss Indication (PLI) requests per second. Although these PLI requests don't directly lead to full frame retransmissions or additional outgoing networking overhead from the Publisher, they do trigger a lot of processing, resulting in CPU spikes on the Publisher.
Additional Context
While this PR addresses the overflow of PLI requests, a related issue remains unresolved. Specifically, when Chrome is used for desktop sharing, the generated video streams often operate at a very low frame rate (0.2–0.5 FPS). This low frame rate causes Janus to generate excessive PLIs. The issue arises primarily from the janus_rtp_simulcasting_context_process_rtp() function in rtp.c. It suggests that the Publisher requires a PLI (->need_pli) if no stream packets have been received for more than 250,000 microseconds.
As a temporary fix, we applied a patch that identifies desktop sharing by checking for the “_desktop” string constant we add to the Publisher's display property. However, for a correect solution it might be beneficial to add a lowFps property for Publishers. This would allow for more precise logic to handle such cases effectively.
Thank you in advance for your input.
Nati Baratz