Skip to content

libobs: Fix texture encoder duping frames when queue full #10096

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

derrod
Copy link
Member

@derrod derrod commented Jan 11, 2024

Description

Fixes an oversight with queue_frame() where the first frame in the queue would be duplicated if there are no available free queue slots, thus keeping the encoder busy with duplicate frames rather than allowing it to work through the existing queue.

By changing the condition to only duplicate frames if there are free slots and the current frames needs duplication this snowballing issue no longer occurs.

Additionally, to avoid calling circlebuf_pop_front() on an empty buffer frames are now skipped if no items are available in the queue.

Motivation and Context

Want to be able to push GPU encoders to the limit without it falling apart when tabbing in and out of a game.

How Has This Been Tested?

Twitch "enhanced broadcasting" dev build with 4 NVENC encoders running slow presets/tunes.

Note that this issue is rather hard to reproduce with a single encode session, in my case I noticed it with 4 running: 1440p120 AV1 (p7), 1080p60 AV1 (p7), 720p60 HEVC (p5), and 480p30 H.264 (p3).

However, I have been able to trigger it with a single encoder in some cases, it just required tabbing in and out of a GPU intensive game running in exclusive fullscreen.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@derrod derrod added Bug Fix Non-breaking change which fixes an issue Seeking Testers Build artifacts on CI labels Jan 11, 2024
@derrod derrod requested a review from Lain-B January 11, 2024 21:35
@derrod
Copy link
Member Author

derrod commented Jan 12, 2024

Okay this has an issue where the PTS isn't incremented when frames are skipped, need to find a way to deal with that :|

@RytoEX RytoEX marked this pull request as draft January 12, 2024 19:58
@derrod
Copy link
Member Author

derrod commented Jan 12, 2024

Pushed a fix, not sure if there's a better way, but we need to keep PTS synced up so just inserting fake frames into the queue seems to do the trick.

@derrod derrod marked this pull request as ready for review January 12, 2024 23:19
@derrod derrod changed the title libobs: Fix queue_frame duplicating GPU encoder queue items when full libobs: Fix texture encoder duplicating frames when it redlines Jan 13, 2024
@derrod derrod force-pushed the fix-gpu-queue branch 2 times, most recently from c5fec9f to 5933332 Compare January 13, 2024 02:10
@derrod derrod changed the title libobs: Fix texture encoder duplicating frames when it redlines libobs: Fix texture encoder duping frames when queue full Jan 13, 2024
@ma3uk
Copy link

ma3uk commented Jan 13, 2024

It works fine, without this commit, when recording League of Legends, each collapse and unfolding of the game window gave several missed frames, with this commit there are no frame skips.

@derrod derrod force-pushed the fix-gpu-queue branch 3 times, most recently from 5933332 to df6fd88 Compare January 13, 2024 17:52
@derrod derrod mentioned this pull request Jan 25, 2024
6 tasks
@palana
Copy link
Contributor

palana commented Mar 27, 2024

this is kind of interesting in that it brings the behavior closer to what obs classic did; have you checked how this interacts with keyframe intervals? one of the issues in obs classic was that configuring a keyframe interval did not exactly work because encoders were fed a potentially variable number of frames, so there was no stability guarantee around having a keyframe interval of 2 seconds (e.g. 120 frames at 60 fps) actually be 2 seconds; I'd assume this suffers from the same issue, unless you bring back manual synchronization (which we also experimented with in obs classic)?

also, thoughts on extending this to cpu based encoders?

@derrod
Copy link
Member Author

derrod commented Mar 27, 2024

this is kind of interesting in that it brings the behavior closer to what obs classic did; have you checked how this interacts with keyframe intervals? one of the issues in obs classic was that configuring a keyframe interval did not exactly work because encoders were fed a potentially variable number of frames, so there was no stability guarantee around having a keyframe interval of 2 seconds (e.g. 120 frames at 60 fps) actually be 2 seconds; I'd assume this suffers from the same issue, unless you bring back manual synchronization (which we also experimented with in obs classic)?

I'm not entirely familiar with the implementation in OBS classic but this still keeps a steady frame rate going into the encoder, with skips only happening if the encoder queue is full. I don't really think this is an issue per-se, as you already don't have guarantees for keyframe intervals being strict unless scenecut=0 or equivalents are set.

FWIW in my testing the times this actually kicked in were fairly rare, as it requires the encoder to run close to its maximum throughput for a temporary stall to fill up the queue.

also, thoughts on extending this to cpu based encoders?

Haven't looked into that yet, but I suppose it would be possible.

@palana palana mentioned this pull request Jun 7, 2024
6 tasks
When a texture encoder is working close to its throughput limit and a
stall causes the encoder frame queue to fill up the old implementation
would duplicate frames already in the queue to maintain sync.

This would result in a snowball effect if the encoder wasn't able to
work through the queue fast enough, continually adding more duplicates
keeping the encoder busy while dropping actually new frames that could
be encoded instead.

To fix this we add a flag that allows us to insert "fake" frames into
the queue, those do not go back into the available queue and simply
signal the GPU encoder thread to incremene the PTS without actually
encoding anything. This is required to maintain audio/video sync while
still allowing us to skip frames.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Non-breaking change which fixes an issue Seeking Testers Build artifacts on CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants