Audio only master #1043

paulpv · 2024-07-21T22:10:17Z

This is a master based cherry pick of @Trouffman 's main commit in #1027 to give him credit for the fix.

This will be released as 4.14.1 in a few days.

The audio-only was resetting the frame and settings parameter only when it was set. This created other issues that the "ndi receiver" reset was not done uniformly depending if there was video or audio only. This also created issue at launch to throw some error.

Trouffman · 2024-07-25T02:22:36Z

There is a known issue in this PR that might collide with a V6 feature.

Let me test it on master before merging.

Trouffman · 2024-07-26T06:32:30Z

Test Settings / steps:

NDI Source set as "Audio Only"
Restart OBS
Start Recording

Results possibles:

Success : Audio-Only Source is audible in recording when expected.
Fail : Audio-Only Source does not behave as expected (ie.: missing on Recording)

Test results :

Behavior when hidden:	Keep source connected	Disconnect source when not visible
Keep last frame when disconnected : ticked	Success	Success
Keep last frame when disconnected : unticked	FAIL(1)	Success

Failure status #1 :

Audio level is visible in UI
Audio is NOT in recorded file / missing

Trouffman · 2024-07-26T11:51:07Z

HOLD ON for merge!
Found a better way to address this issue and keep it compatible with other features.

Adding a first Frame creation before any drawing start should help with the "frame not seen", "blank frame" and "audio only not on recording" issues.

Trouffman · 2024-07-26T13:02:14Z

This is now compatible with the latest added features.

It should also fix some other issues where the "frame" of and NDI source is not visible (because size is 0x0 for some reasons).

This also should follow the concept that :

When you start : start with a clean state then apply settings

Tested on MacOS (M2) with OBS 30.1.2 + test Pattern Generator and a 1K tone

paulpv · 2024-07-26T19:52:56Z

This is now compatible with the latest added features.

It should also fix some other issues where the "frame" of and NDI source is not visible (because size is 0x0 for some reasons).

This also should follow the concept that :

When you start : start with a clean state then apply settings

Tested on MacOS (M2) with OBS 30.1.2 + test Pattern Generator and a 1K tone

Related to the 0x0 frame, I have noticed in the latest OBS log output that it says:

info: [obs-ndi] +ndi_source_create('NDI™ Source'...)
error: bmalloc: Allocating 0 bytes is broken behavior, please fix your code! This will crash in future versions of OBS.
info: [obs-ndi] +ndi_source_update('NDI™ Source'...)

So we will need to address the 0x0 problem soon anyway! :)

paulpv · 2024-07-27T01:13:54Z

@Trouffman Are we good/safe/ok to merge this?

Trouffman · 2024-07-27T04:26:58Z

That error message is new it seems ! (The bmalloc 0 byte allocation)

This has been tested on Mac, not yet windows/Linux.

Can merge, will need testing anyway.

paulpv · 2024-07-27T04:47:40Z

Can merge, will need testing anyway.

This PR would be the place to test the changes on Linux and Windows.
This PR will generate installers because of the Seeking Testers label.

Once merged to master there won't be any installers generated until an actual release is tagged.
I supposed we could tag a release and just not create a Release entry at https://github.com/DistroAV/DistroAV/releases and test that tag and once it is working then create a Release entry.

Trouffman · 2024-07-27T06:00:30Z

Let do the test on this PR!
I can have windows tested soon.

Trouffman · 2024-07-27T15:46:30Z

I quickly tested on Windows (11) but I need more people to test it on windows.

Behavior when hidden:	Keep source connected	Disconnect source when not visible
Keep last frame when disconnected : ticked	tbc	tbc
Keep last frame when disconnected : unticked	tbc	no audio on recording after obs restart

paulpv · 2024-07-30T05:11:15Z

src/obs-ndi-source.cpp

@@ -440,6 +440,16 @@ void *ndi_source_thread(void *data)

 	NDIlib_recv_create_v3_t *reset_recv_desc = &recv_desc;

+	if (!s->running) {


I don't get this.
This line is:

if (!s->running) { ... }

The next immediate line after this if statement is:

while (s->running) { ... }

If the if statement is entered, then s->running is false, then the while loop never will be entered.
If the if statement isn't entered, then s->running is true, then the while loop will be entered.

I know this ndi_source_thread runs inside of the while loop.

So, this tells me that this if (!s->running) { ... } statement is not doing anything.

😕

paulpv · 2024-07-30T05:11:46Z

src/obs-ndi-source.cpp

@@ -440,6 +440,16 @@ void *ndi_source_thread(void *data)

 	NDIlib_recv_create_v3_t *reset_recv_desc = &recv_desc;

+	if (!s->running) {
+		// Force a clean frame at first start
+		obs_source_output_video(obs_source, blank_video_frame());


This allocates a blank video frame and never releases it.

We should probably just reuse s->config.blank_frame

src/obs-ndi-source.cpp

paulpv · 2024-07-30T05:20:12Z

Around

DistroAV/src/obs-ndi-source.cpp

Lines 202 to 208 in cb40079

    
           static obs_source_frame *blank_video_frame() 
        
           { 
        
           	obs_source_frame *frame = 
        
           		obs_source_frame_create(VIDEO_FORMAT_NONE, 0, 0); 
        
           	frame->timestamp = os_gettime_ns(); 
        
           	return frame; 
        
           }

I looked into the bmalloc: Allocating 0 bytes is broken behavior log warning and it has been around for over 2 years.
We should probably fix this soon too! :)

static obs_source_frame *blank_video_frame()
{
	obs_source_frame *frame =
		obs_source_frame_create(VIDEO_FORMAT_NONE, 0, 0);
	frame->timestamp = os_gettime_ns();
	return frame;
}

This is causing the following OBS log message(s):

info: [obs-ndi] +ndi_source_create('NDI™ Source'...)
error: bmalloc: Allocating 0 bytes is broken behavior, please fix your code! This will crash in future versions of OBS.
info: [obs-ndi] +ndi_source_update('NDI™ Source'...)

obs_source_frame_create: https://github.com/obsproject/obs-studio/blob/e215502b620ce9d97f4d0191adef24856682b9e9/libobs/obs.h#L2751-L2760
- video_frame_init: https://github.com/obsproject/obs-studio/blob/8c71b0b586a0b0a21fb6de9113c57de168d28668/libobs/media-io/video-frame.c#L227
  - bmalloc: https://github.com/obsproject/obs-studio/blob/106c7aa61f3229be628a77ef840bce414580df82/libobs/util/bmem.c#L104-L107
    This warning has been there since 2022/07/16:
    https://github.com/obsproject/obs-studio/blame/106c7aa61f3229be628a77ef840bce414580df82/libobs/util/bmem.c#L104-L107

I wonder what OBS dev's recommendation is for the best way to have an audio/video capable source that intentionally wants to use only audio and no video.

Making a 0x0 video made sense at the time and is obviously a borderline hack.
A 1x1 video might also have weird side-effects and even be visible in the video!
I wonder if just creating a more non-extreme 10x10, 50x50, or 100x100 might make more sense.
Maybe have some reasonable transparency color mask and allow the user to user to set their own.
Seems complicated. 😦

Maybe what we should really be doing is defining two separate NDI Source types:

NDI Audio/Video Source: If you don't want the audio, just mute the source!
We could maybe have an option in the Source to save a little overhead and not process audio, essentially Video Only.
Video Only sources don't care if audio isn't pumped
NDI Audio Only Source: would show up in OBS as a standard audio only source and would never need any blank_[video_]frame logic.

paulpv · 2024-07-30T05:37:47Z

@Trouffman I think we should hold off on this code change and not push it to master.
Any objections?

Maybe still keep it open to discuss?

Trouffman · 2024-07-30T07:54:24Z

Agreed. This is not ready for prime time yet.

Trouffman · 2024-07-30T08:15:09Z

So whenever I try to fix one behavior, this brings another side issue or another "combo" of settings that breaks

Could be about time to rework that part more deeply.

Trouffman · 2024-07-31T02:35:17Z

@paulpv This would be the new audio-Only fix approach.
Has been tested on : MACos (M2).

paulpv · 2024-07-31T03:28:56Z

src/obs-ndi-source.cpp

@@ -1075,7 +1098,7 @@ void *ndi_source_create(obs_data_t *settings, obs_source_t *obs_source)
 		QString("OBS-NDI '%1'").arg(name).toUtf8();

 	// Allocate blank video frame
-	s->config.blank_frame = blank_video_frame();
+	// s->config.blank_frame = blank_video_frame();


Intentionally not creating any blank_frame?
Does this work?

Correct, the creation of the blank frame moved to ndi_source_update().

This is the only line where config.blank_frame is ever assigned.
There are no other calls to blank_video_frame().
So, nothing else is ever creating a blank frame.
If this is commented out then all uses of s->config.blank_frame are just sending nullptr.

Maybe this is intentional.
Maybe this is accidental.
Maybe you stumbled on a solution accidentally? 😃

If not calling blank_video_frame() is intentional and fixes this, then maybe the whole static obs_source_frame *blank_video_frame() method should be removed?

This is a happy little accident :D that fixed the bug!

If we call "blank_video_frame()" = the bug exist. Setting config.blank_frame = nullptr (explicitly or not) fixes the problem.

I am looking though commit history to find the reason of that blank frame to see what it was used for. (fixing a bug maybe?)

Digging deeper :

This blank_frame was added to fix the Audio only that was "keeping the last frame/not cleaning the frame".
7d1cf13

This was part of v4.3.0 : "Source bugfix: video not cleared when switching to Audio Only mode"
https://github.com/DistroAV/DistroAV/releases/tag/4.3.0

src/obs-ndi-source.cpp

paulpv · 2024-07-31T19:34:42Z

@Trouffman I will test this on Windows [and maybe Linux] once you reply to my comments.

That commented out s->config.blank_frame = blank_video_frame(); makes me think you might have miss-committed/pushed this line.

Trouffman · 2024-08-01T07:00:21Z

Tested and confirmed it works & fix the bug on MACOS & Windows 11.

paulpv · 2024-08-01T23:22:22Z

src/obs-ndi-source.cpp

+			// Force a clean (blank) frame when settings change to Audio only
+			if (recv_desc.bandwidth ==
+			    NDIlib_recv_bandwidth_audio_only) {
+				obs_source_output_video(obs_source,


Per https://docs.obsproject.com/reference-sources#c.obs_source_output_video
Just use:

obs_source_output_video(obs_source, nullptr);

Will update everywhere and get rid of the config.blank_frame then

paulpv · 2024-08-01T23:22:32Z

src/obs-ndi-source.cpp

+			if (config.bandwidth ==
+			    NDIlib_recv_bandwidth_audio_only) {
+				// Force a clean frame when source is updated 
+				obs_source_output_video(obs_source,


Per https://docs.obsproject.com/reference-sources#c.obs_source_output_video
Just use:

obs_source_output_video(obs_source, nullptr);

src/obs-ndi-source.cpp

paulpv · 2024-08-02T03:33:24Z

@Trouffman Looks good! I will test this my tonight or tomorrow before approving the code.

paulpv · 2024-08-02T08:01:44Z

Tests out fine on both Mac and Windows.
I tested combinations of:

Bandwidth: Highest, Lowest, Audio Only
Behavior: Keep source connected, Disconnect source when not visible
Keep last frame when disconnected: On, Off

All behaved as I would expect it to.

More importantly:

I closed OBS
Reopened OBS
Immediately start recording/streaming
Wait ~10 seconds
Stop recording/streaming
Open recording and verify audio can be heard

I will merge to master.

Thanks @Trouffman !

Trouffman · 2024-08-02T09:34:01Z

@paulpv Added clarity or the Bug fix :

BugFix : Audio is missing from audio-only NDI Source after OBS restart.

Main bug with workaround solution: #592

Should fix the issues :
#429
#578
#981
#906
#621
#467
#298

Trouffman and others added 2 commits July 21, 2024 15:06

Fix Audio-Only bug

49143f1

The audio-only was resetting the frame and settings parameter only when it was set. This created other issues that the "ndi receiver" reset was not done uniformly depending if there was video or audio only. This also created issue at launch to throw some error.

buildspec 4.14.0 -> 4.14.1

3b7dfdd

paulpv added bug audio Seeking Testers PRs with this label will package the plugin so that others can test labels Jul 21, 2024

paulpv requested review from Palakis, Trouffman and RayneYoruka July 21, 2024 22:10

paulpv self-assigned this Jul 21, 2024

Merge branch 'master' into audio_only_master

2fcb899

paulpv added 2 commits July 25, 2024 01:10

settings: add checkBoxAutoCheckForUpdates to opt out

b595d48

Merge branch 'master' into audio_only_master

3bd2fa7

Trouffman added 2 commits July 26, 2024 20:53

Re-work the way the Audio-Only option is managed.

5c8f5fe

Adding a first Frame creation before any drawing start should help with the "frame not seen", "blank frame" and "audio only not on recording" issues.

Removing commented-out code not ready for release.

4dda7c2

paulpv added 2 commits July 26, 2024 12:31

Merge branch 'master' into audio_only_master

23b6d1d

Merge branch 'master' into audio_only_master

041494f

build: fix clang-format error

cb40079

Merge branch 'master' into audio_only_master

0fd3765

paulpv force-pushed the audio_only_master branch from 8849a74 to 0fd3765 Compare July 27, 2024 18:29

Merge branch 'master' into audio_only_master

3fa4bff

paulpv commented Jul 30, 2024

View reviewed changes

paulpv marked this pull request as draft July 30, 2024 11:10

Remove arguable fix - Add new audio-only fix (simpler).

4f43e47

Merge branch 'master' into audio_only_master

06c6f27

paulpv commented Jul 31, 2024

View reviewed changes

src/obs-ndi-source.cpp Outdated Show resolved Hide resolved

Trouffman added 2 commits August 1, 2024 14:45

Clarify comments and log message

3e11a1d

correcting typo.

694a189

Trouffman marked this pull request as ready for review August 1, 2024 07:00

Trouffman mentioned this pull request Aug 1, 2024

Audio only patch #1027

Closed

Trouffman added 2 commits August 2, 2024 02:08

Clean-up audio-only fix + tweaks

89ddacd

updated logging description

aa4d8d6

paulpv commented Aug 1, 2024

View reviewed changes

Trouffman added 3 commits August 2, 2024 07:54

clang format update

63c1847

Removed obsolete blank_frame logic

f4d96a6

Fixing clang-format

77670ef

paulpv merged commit ab3c22b into master Aug 2, 2024
6 checks passed

paulpv deleted the audio_only_master branch August 2, 2024 08:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audio only master #1043

Audio only master #1043

paulpv commented Jul 21, 2024

Trouffman commented Jul 25, 2024

Trouffman commented Jul 26, 2024 •

edited

Loading

Trouffman commented Jul 26, 2024

Trouffman commented Jul 26, 2024

paulpv commented Jul 26, 2024

paulpv commented Jul 27, 2024

Trouffman commented Jul 27, 2024

paulpv commented Jul 27, 2024

Trouffman commented Jul 27, 2024

Trouffman commented Jul 27, 2024 •

edited

Loading

paulpv Jul 30, 2024 •

edited

Loading

paulpv Jul 30, 2024 •

edited

Loading

paulpv commented Jul 30, 2024 •

edited

Loading

paulpv commented Jul 30, 2024

Trouffman commented Jul 30, 2024

Trouffman commented Jul 30, 2024

Trouffman commented Jul 31, 2024

paulpv Jul 31, 2024

Trouffman Aug 1, 2024

paulpv Aug 1, 2024 •

edited

Loading

Trouffman Aug 1, 2024 •

edited

Loading

Trouffman Aug 1, 2024

paulpv commented Jul 31, 2024 •

edited

Loading

Trouffman commented Aug 1, 2024

paulpv Aug 1, 2024

Trouffman Aug 2, 2024

paulpv Aug 1, 2024

paulpv commented Aug 2, 2024

paulpv commented Aug 2, 2024 •

edited

Loading

Trouffman commented Aug 2, 2024

		@@ -440,6 +440,16 @@ void ndi_source_thread(void data)

		NDIlib_recv_create_v3_t *reset_recv_desc = &recv_desc;

		if (!s->running) {

Audio only master #1043

Audio only master #1043

Conversation

paulpv commented Jul 21, 2024

Trouffman commented Jul 25, 2024

Trouffman commented Jul 26, 2024 • edited Loading

Trouffman commented Jul 26, 2024

Trouffman commented Jul 26, 2024

paulpv commented Jul 26, 2024

paulpv commented Jul 27, 2024

Trouffman commented Jul 27, 2024

paulpv commented Jul 27, 2024

Trouffman commented Jul 27, 2024

Trouffman commented Jul 27, 2024 • edited Loading

paulpv Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

paulpv Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

paulpv commented Jul 30, 2024 • edited Loading

paulpv commented Jul 30, 2024

Trouffman commented Jul 30, 2024

Trouffman commented Jul 30, 2024

Trouffman commented Jul 31, 2024

paulpv Jul 31, 2024

Choose a reason for hiding this comment

Trouffman Aug 1, 2024

Choose a reason for hiding this comment

paulpv Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Trouffman Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Trouffman Aug 1, 2024

Choose a reason for hiding this comment

paulpv commented Jul 31, 2024 • edited Loading

Trouffman commented Aug 1, 2024

paulpv Aug 1, 2024

Choose a reason for hiding this comment

Trouffman Aug 2, 2024

Choose a reason for hiding this comment

paulpv Aug 1, 2024

Choose a reason for hiding this comment

paulpv commented Aug 2, 2024

paulpv commented Aug 2, 2024 • edited Loading

Trouffman commented Aug 2, 2024

Trouffman commented Jul 26, 2024 •

edited

Loading

Trouffman commented Jul 27, 2024 •

edited

Loading

paulpv Jul 30, 2024 •

edited

Loading

paulpv Jul 30, 2024 •

edited

Loading

paulpv commented Jul 30, 2024 •

edited

Loading

paulpv Aug 1, 2024 •

edited

Loading

Trouffman Aug 1, 2024 •

edited

Loading

paulpv commented Jul 31, 2024 •

edited

Loading

paulpv commented Aug 2, 2024 •

edited

Loading