Mitigate corruption, ask for an IDR every X frames #140
Conversation
To mitigate TheElixZammuto#117 (Compression artifacts forming when left on the same scene for a while), request an IDR frame from the host every 1000 packets. This seems to reset the decoder well enough to completely avoid slow corruption problem, but shouldn't add too much extra bandwidth. Unless I'm missing something, Sunshine never sends I or IDR frames after initially establishing the stream unless it's specifically asked to by the client.
WalkthroughThe recent modifications to the FFmpeg decoder enhance video frame processing by implementing a mechanism to request IDR frames when a specified threshold of frames is exceeded. By introducing a new constant and a counter for frames since the last IDR, the decoder improves its ability to handle frame corruption, ensuring more stable and higher-quality video playback. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- Streaming/FFmpegDecoder.cpp (3 hunks)
- Streaming/FFmpegDecoder.h (1 hunks)
Additional comments not posted (4)
Streaming/FFmpegDecoder.h (1)
47-47: Addition offramesSinceLastIDRaligns with PR objectives.The new member variable
framesSinceLastIDRin theDecoderstruct is a crucial addition for tracking the number of frames since the last IDR frame. This aligns well with the PR's goal to mitigate corruption by periodically requesting IDR frames.Streaming/FFmpegDecoder.cpp (3)
19-19: MacroMAX_DELAY_BETWEEN_IDR_FRAMESsets a clear threshold.The definition of
MAX_DELAY_BETWEEN_IDR_FRAMESwith a value of 1000 frames provides a clear threshold for requesting IDR frames, aligning with the PR's objective to manage frame corruption.
169-169: Initialization offramesSinceLastIDRis necessary.Setting
framesSinceLastIDRto zero in theInitfunction is essential for correctly tracking the number of frames since the last IDR frame from the start of the decoding process.
222-226: Implementation of periodic IDR frame requests is well-aligned with objectives.The logic in
SubmitDUto incrementframesSinceLastIDRand request an IDR frame when the threshold is exceeded is a well-executed implementation of the PR's goal to mitigate video corruption.
|
I would have liked to have only sent this when the decoder starts to get weird, but I couldn't find any way to detect that. Tried logging anything I could find and nothing really stands out. |
|
I haven't checked:
|
|
Looks like missed frames can't be used, at least not for the issue as it happens to me. 0 missed frames and still the corruption starts. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- Streaming/FFmpegDecoder.cpp (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- Streaming/FFmpegDecoder.cpp
|
|
Hi @BradyBrenot, Just to confirm, have you checked the Sunshine build here #117 (comment) ? In practice the commit LizardByte/Sunshine@4840aaa of the test build of Sunshine should already do this but on NVEnc only (the issue only occurs there IIRC), on the server side. |
| #define DECODER_BUFFER_SIZE 1048576 | ||
| #define MAX_DELAY_BETWEEN_IDR_FRAMES 1000 |
There was a problem hiding this comment.
Is there a reason for the 1000 value or it's just due to empircal tests?
To mitigate #117 (Compression artifacts forming when left on the same scene for a while), request an IDR frame from the host every 1000 packets. This seems to reset the decoder well enough to completely avoid slow corruption problem, but shouldn't add too much extra bandwidth.
Unless I'm missing something, Sunshine never sends I or IDR frames after initially establishing the stream unless it's specifically asked to by the client.
(This is a hacky mitigation, not a proper fix. It works but it's not the ideal fix. Since this only seems to happen on this platform, I'm guessing either we're touching memory we shouldn't / in a way we shouldn't, or ffmpeg has a hardware acceleration bug on this platform. Tracking down the root cause is a pain though, especially with UWP's limitations)
Summary by CodeRabbit
New Features
Bug Fixes