fix(macos): fix broken streaming on MacOS

[Hazer]

Description

I have a MacOS 14.4 running with a AMD RX 570, and noticed the mentioned issues, so after discovering version 0.19.1 was working (thanks to @VasylBaran), I decided to try narrowing down the issue.

Solution

Partially reverts e535706a09f1849d2799fd72e889e4d7182f7fe5, this commit changes av_img_t and how it retains/release the frame buffers, which breaks something with how the frame is available to the encoder when needed and creates the mentioned issues.

If someone could help me out fixing av_img_t without reverting the structure, I would gladly improve this solution. Or if you have even another solution, I can pull and run here on my hardware.

Fixes av_img_t usage and related structs, while retaining it until the next av_img_t is correctly configured. This slightly changes how it retains/release the frame buffers, allowing the frame to be available to the encoder when needed and avoids the mentioned issues.

Quoting my discovery journey from #2272:

First it was in commit 036aa2e470cf72e63f036b6b4ec23446a4fdf140, it freed the frame too soon in video.cpp:1735, so the encoder failed, then eed27d3 broken macOS for a while. It then was fixed in a29d2e11ea4b46590dab0ea97586bbc5152a8c7a, which worked if I reverted 036aa2e changes after line 1735, but got broken again in the next commit... After poking around a bit, found out that now the culprit was mostly e535706a09f1849d2799fd72e889e4d7182f7fe5, this commit changes av_img_t and how it retains/release the frame buffers, which again, breaks something with how the frame is available to the encoder when needed.

So after trying to fix the retain/release cycle of this change without success, I decided to just revert the necessary changes, taking care to not remove newer unrelated changes, and it's now working for me even with the nightly branch.

Screenshot

Issues Fixed

• Fixes Hardware encoders do not work on macOS Ventura #2272
• Fixes Broken streaming (freeze/looping) on Mac (14.4) and Sunshine (v0.22.2) #2347

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Dependency update (updates to dependencies)
• Documentation update (changes to documentation)
• Repository update (changes to repository files, e.g. .github/...)

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

Branch Updates

LizardByte requires that branches be up-to-date before merging. This means that after any PR is merged, this branch
must be updated before it can be merged. You must also
Allow edits from maintainers.

• I want maintainers to keep my branch updated

[CLAassistant]

All committers have signed the CLA.

[ReenigneArcher]

@cgutman could you review this since it partially reverts some of your changes?

[codecov]

Codecov Report

Attention: Patch coverage is 45.23810% with 23 lines in your changes are missing coverage. Please review.

Project coverage is 7.04%. Comparing base (c5d8e1b) to head (ee77d2e).

Additional details and impacted files

@@            Coverage Diff             @@
##           nightly   #2485      +/-   ##
==========================================
+ Coverage     6.99%   7.04%   +0.04%     
==========================================
  Files           87      87              
  Lines        17679   17697      +18     
  Branches      8399    8405       +6     
==========================================
+ Hits          1237    1246       +9     
- Misses       13713   13874     +161     
+ Partials      2729    2577     -152     

Flag	Coverage Δ
Linux	5.35% <100.00%> (+<0.01%)	⬆️
Windows	2.56% <0.00%> (-0.01%)	⬇️
macOS-12	8.07% <40.47%> (+0.08%)	⬆️
macOS-13	7.98% <40.47%> (+0.08%)	⬆️
macOS-14	8.28% <40.47%> (+0.04%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/platform/macos/nv12_zero_device.cpp	100.00% <ø> (ø)
src/video.cpp	25.27% <66.66%> (+0.23%)	⬆️
src/platform/macos/av_img_t.h	76.47% <75.00%> (+9.80%)	⬆️
src/platform/macos/display.mm	40.17% <29.62%> (-3.13%)	⬇️

... and 22 files with indirect coverage changes

[Hazer]

@ReenigneArcher @cgutman I was using this version privately and noticed hardware encoding wasn't working yet, while now at least software encoding was working, it only fixed the frame being available to the encoder in time.

So I went into another investigation and found out that AV_CODEC_FLAG_LOW_DELAY won't work with videotoolbox, it fails during avcodec_open2, so I made a new commit, and this fixes hardware encoder for h264 and hvec.

[TimmyOVO]

With AV_CODEC_FLAG_LOW_DELAY flag set , it seems like videotoolbox hevc and h.264 encoders works as expect on M2 Max.
This change should consider enclosed by #ifndef __APPLE__

[Hazer]

With AV_CODEC_FLAG_LOW_DELAY flag set , it seems like videotoolbox hevc and h.264 encoders works as expect on M2 Max. This change should consider enclosed by #ifndef __APPLE__

@TimmyOVO By low delay set, you mean my PR it's working without my last changes and with:
ctx->flags |= AV_CODEC_FLAG_CLOSED_GOP | AV_CODEC_FLAG_LOW_DELAY; set?

If that's the case AV_CODEC_FLAG_LOW_DELAY, works on Apple Silicon, but probably not on Intel macs with AMD dGPUs, it would be nice to have someone with M1 to try it out too and confirm if it's all Apple Silicon vs only M2+...

Is currently there a way to check if we are under Apple Silicon vs Intel because #ifdef __APPLE__ only means macOS and this fix it's exactly because I'm on an Intel mac.

[Hazer]

Actually, is this related AT ALL, with platform and cpu architecture or it's just a GPU/Driver capabilities thing? Because probably we should just retry any codec that failed without AV_CODEC_FLAG_LOW_DELAY...

[Hazer]

As I'm not sure this is platform specific, nor architecture specific, I changed it to retry once for any codec that we try to open and have failed. This seems to me more a capability of certain GPUs than a platform/architecture thing, double checking if this flag is breaking any encoder seems better to me.

[Hazer]

@ReenigneArcher I've fixed the release/retains issues using the av_img_t pattern @cgutman created, we're not reverting anymore, just adding. Tested both with software encoders and hardware encoders (seems like the software encoder is slightly more sensible to this, on my end).

The most important changes are:

• Changed how av_pixel_buf_t is created and used
• Create new av_sample_buf_t and av_pixel_buf_t earlier
• Store the old av_pixel_buf_t values into a new shared_ptr temp_retain_av_img_t and release only after configuring all settings of the new av_pixel_buf_t, this way we give it enough time to avoid being released too early

I think this is the definitive version of this PR, if all testing goes well now.

[ReenigneArcher]

@Hazer thank you! Could you fix the linting errors? https://github.com/LizardByte/Sunshine/actions/runs/9024488207/job/24799565097?pr=2485#step:5:14

[Hazer]

@Hazer thank you! Could you fix the linting errors? https://github.com/LizardByte/Sunshine/actions/runs/9024488207/job/24799565097?pr=2485#step:5:14

Done, but it's a bit uglier now in my opinion haha
PS: Sorry for not noticing before @ReenigneArcher

[ReenigneArcher]

I agree, but it's difficult to change the lint rules without causing conflicts in all the open PRs.

[TimmyOVO]

As I'm not sure this is platform specific, nor architecture specific, I changed it to retry once for any codec that we try to open and have failed. This seems to me more a capability of certain GPUs than a platform/architecture thing, double checking if this flag is breaking any encoder seems better to me.

I agree, this might be related to gpu driver ,cause intel based mac are using different gpu driver than apple silicon one.Now looks good to me.

[ReenigneArcher]

This isn't necessarily related to your changes, but would like to try to figure it out nonetheless.

I can't figure out why the flatpak build is failing to checkout the latest commit.

Failed to download sources: module sunshine: Git commit for branch fix/macos-x86-dGPU is 8b981397b1e5a48d5fb18c2e01725f240c525d22, but expected 2c72ac42022516bd26cc9bb6b8b59cd0b294d801

I do see 8b981397b1e5a48d5fb18c2e01725f240c525d22 in your repo, but not in this branch which makes it extra confusing.

[Hazer]

Failed to download sources: module sunshine: Git commit for branch fix/macos-x86-dGPU is 8b981397b1e5a48d5fb18c2e01725f240c525d22, but expected 2c72ac42022516bd26cc9bb6b8b59cd0b294d801

I do see 8b981397b1e5a48d5fb18c2e01725f240c525d22 in your repo, but not in this branch which makes it extra confusing.

I think it failed in my CI too, let's try something, I deleted the tag created in my repo, if you run it now what happens?

[ReenigneArcher]

Yep, it was the tag. I'll have to keep that in mind for future cases.