Skip to content
This repository has been archived by the owner on Mar 22, 2022. It is now read-only.

Redundant conversion to ARGB consumes a lot of CPU (HL2 cannot stream > 720p with hardware H.264) #151

Closed
fibann opened this issue Dec 18, 2019 · 7 comments · Fixed by #486
Assignees
Labels
bug Something isn't working
Milestone

Comments

@fibann
Copy link
Member

fibann commented Dec 18, 2019

This is a regression starting from ab67d06 (so impacts releases starting from v1.0.1). It's not incredibly clear what's happening but the best guess is that adding the callbacks triggers the conversion of every frame to RGB, which starves the encoding thread.

This is going to be fixed as part of #152. If the RGB frames are not needed, a simple workaround is to revert the commit above.

@fibann fibann added the bug Something isn't working label Dec 18, 2019
@djee-ms djee-ms added this to the 2.0.0 milestone Dec 18, 2019
@fibann
Copy link
Member Author

fibann commented Dec 19, 2019

Further experiments show that just reverting that commit won't fix the issue on latest master. So some other cause was introduced between that commit and now. Will investigate more.

@fibann
Copy link
Member Author

fibann commented Dec 19, 2019

Commit 1050e49 moved the offending callback to another file, so blindly reverting ab67d06 won't disable it. Cherry-pick fibann@2ae2408 instead.

@fibann
Copy link
Member Author

fibann commented Jun 18, 2020

This has been fixed by #412 - video callbacks are now added only if there is at least one managed handler, so we don't waste cycles by default anymore.

@fibann fibann closed this as completed Jun 18, 2020
fibann added a commit to fibann/MixedReality-WebRTC that referenced this issue Jul 16, 2020
fibann added a commit that referenced this issue Jul 16, 2020
@fibann fibann changed the title HL2 cannot stream > 720p with hardware H.264 Redundant conversion to ARGB consumes a lot of CPU (HL2 cannot stream > 720p with hardware H.264) Jul 24, 2020
@fibann
Copy link
Member Author

fibann commented Jul 24, 2020

#412 implemented the correct pattern in video sources but did not backport it to video tracks, so this is still open.

@fibann fibann reopened this Jul 24, 2020
@fibann fibann assigned fibann and unassigned djee-ms Jul 24, 2020
fibann added a commit to fibann/MixedReality-WebRTC that referenced this issue Jul 28, 2020
Registering a callback with the interop library triggers the conversion of
every frame from the native format into the one the callback wants, which is
very expensive and should be done only when needed.

This also fixes a deadlock caused by subscription and frame-ready event trying
to acquire the callback lock and the WebRTC worker thread at the same time.

Fixes microsoft#151.
fibann added a commit to fibann/MixedReality-WebRTC that referenced this issue Jul 28, 2020
Registering a callback with the interop library triggers the conversion of
every frame from the native format into the one the callback wants, which is
very expensive and should be done only when needed.

This also fixes a deadlock caused by subscription and frame-ready event trying
to acquire the callback lock and the WebRTC worker thread at the same time.

Fixes microsoft#151.
fibann added a commit that referenced this issue Jul 29, 2020
Registering a callback with the interop library triggers the conversion of
every frame from the native format into the one the callback wants, which is
very expensive and should be done only when needed.

This also fixes a deadlock caused by subscription and frame-ready event trying
to acquire the callback lock and the WebRTC worker thread at the same time.

Fixes #151.
@kspark-scott
Copy link

@fibann Can you clarify whether this issue affects encoding, decoding, or both? Your initial description here refers to starving the encoding thread, but this also references issue #160 which looks like a problem triggered by inbound video.

I ask because we have an upcoming release of our product and are trying to decide whether to move to 2.0.0 for this release or wait until the next. The performance benefit of this fix would likely mean we would move to 2.0.0 immediately, but would prefer not to incur the risk (however minimal) if there is no immediate benefit.

Thanks

@fibann
Copy link
Member Author

fibann commented Aug 4, 2020

@kspark-scott this affects both local and remote video (note that it's not directly related to encoding, as long as you have a local track - even if you are not streaming it - you'll get the performance hit).

While we recommend that you upgrade to 2.0 in general, you can simply cherry-pick fibann@2ae2408 (as said above) to work around this issue on 1.0.3 (note anyway that with that change the ARGB32 frame events will not be fired at all).

@kspark-scott
Copy link

Perfect, thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants