6.0.0 "Visibility behavior" update (Text & logic) #1074

paulpv · 2024-08-09T22:28:19Z

This all started with #993

I love the feature!
I even personally understand how it works and is supposed to be used.

But, it seems [to me] oddly difficult to expect other users to understand how to use it and other developers to understand how it is implemented (and how to maintain the code).
It has even been the source of some tricky recent bugs. ☹️

The current UI is this:

* Behavior: (dropdown)
  1. Keep source connected
  2. Disconnect source when not visible
* [ ] Keep last frame when disconnected (checkbox)

That, at first, seems fine and mostly intuitive.

But when someone starts to use the feature things can get confusing.

What does it mean to keep the source connected?
What does it mean to disconnect a source?
What is visibility? In Program or Preview or both?
Is the checkbox related to the Behavior dropdown or not? They both say "disconnect".
So is the checkbox being on [or off] saying that the last frame will be kept [or blanked] if there is any disconnect, or only if there is a disconnect due to visibility?
So does the checkbox have any meaning if Behavior is Keep source connected?

I could go on with at least another half dozen or more questions.

I propose something like the following that, although currently ugly, does help, I think, clear up some confusion:

* Visibility Behavior: (dropdown)
  1. Not-Visible: Keep Active
  2. Not-Visible: Pause; Visible: Show Black Frame & Resume
  3. Not-Visible: Pause; Visible: Show Last Frame & Resume

This is what is implemented in this PR, but it mostly for discussion.

But, to implement [something like] this we would have to migrate the settings from the old/current values to the new values.
So that complicates the code a little bit.

What do y'all think?

CC @Trouffman @haakonnessjoen @RayneYoruka

Trouffman · 2024-08-16T15:57:37Z

Still thinking about the best way to communicate that setting.
We could have a breaking change in V6. This is a relatively new feature. Not ideal but could solve some issues.

RayneYoruka · 2024-08-16T16:02:20Z

It seems the cc got lost in my email.. woops! I like the proposed solution, it's clear and direct without being too cluttered or confusing

Trouffman · 2024-08-17T11:35:16Z

I am growing more and more a fan of this drop-down menu. BUT with one change :

Change "Show Black Frame" by "Clean source. Content"

And "show last frame" to "Show last source content"

Why not using "frame"?

it is a technical term, not obvious for everyone at first (and this is the main issue this PR aim to solve)
and "frame" is used in statistics for the encoder (output/recording), also in error messages for encoder issues like "frames missed to rendering lag" etc

-> interestingly OBS does not use "frame" anywhere else, but refer to "Canva"/"boundary box", etc. which push me to not use frame as well. :D

Trouffman · 2024-08-18T09:58:19Z

src/ndi-source.cpp

+			// ISSUE: Do we need to do both this and in `ndi_source_update`?
 			if (recv_desc.bandwidth ==


This is needed to "clean" the Source Content when switching to Audio Only i we want to keep the original "user experience" behavior as in :

Open NDI Source settings

Change bandwidth to audio only

as soon as this is selected > it will "clear" the source content/frame

On "save" > this should be calling the ndi_source_update ()

Ths goes along side with the UX we want for users : any changes made in the source settings shoud apply right away or only on save?
Default today is expected as : "apply as soon as changed". Commit to config on save. Revert on Cancel.

If we want to keep this > This should stay here.

Trouffman · 2024-08-18T10:04:55Z

src/ndi-source.cpp

+		// ISSUE: I am not 100% convinced that we want to do this **here**.
+		// It depends on how we phrase/word the UI text.
+		// Do we really want to blank the video here **after** it stops,
+		// or should we really be blanking the video just **before** it restarts?
+		// Why can't we just blank the video in the receiver thread?
+		// Isn't that what the code did before the changes in this area?
+		// Why was that not sufficient? What were the problems with that?


Valid suggestion. IIRC on why this was moved here was because in previous commit this was keeping a black frame when the source was part of a scene in preview > but that was a bug as well.

Technically we should be okay to have this source texture "cleaned" when we restart the thread. - Will test and confirm

Trouffman · 2024-08-24T04:24:29Z

Following up on the text itself, the VLC plugin is like this:

Which we could put like this:

On the behavior:
A simplified implementation is "Actively clean the frame on source update" - always EXCEPT "keep last frame and NOT audio-only"

Doing the update only in the "thread" > that require to add extra for managing the audio-only use-case.
Also tricky as it did not work well when changing the bandwidth while the source is hidden/inactive would not honor the setting either.

RayneYoruka · 2024-08-24T04:29:48Z

Which we could put like this:

I like how this is, this is the most informative yet clean and easy to understand. We should push this

…rd compatibility.

Trouffman · 2024-08-24T07:12:40Z

In the last Commit on this PR before this message the key elements are :

Clean Frame when starting (actually, on the source update) - works great so far (even when changing settings in an inactive source)
Update of the text
clean-up of the variable names to be more inline with the wording
Management of the migration from 4.14.x option scheme to 6.0.x
cleaning & Formating of the comments in the code when not necessary anymore.

paulpv · 2024-08-24T17:41:45Z

Following up on the text itself, the VLC plugin is like this:

Where did you find that?
I looked in VLC after installing NDI Tools and could use the feature but could not find that UI setting.

I do like that wording!
Having never seen that before I am flattered that their UI is almost identical to what I proposed :)

I hesitated wording it "Pause", since that it not actually what it is doing.
It is a fake Pause, meaning really just a Stop and don't clear the last frame.

Given VLC's wording, I do like differentiating between Stop and Pause that way:

Stop blanks the last frame (understood we might not to call it "frame")
Pause does keeps [does not blank] the last frame (understood we might not to call it "frame")

@Trouffman Given these terms/paradigms, there are still a few minor inconsistencies in the code variable/string names. Truly they probably are consistent, but the UI is presenting a fake "Pause", whereas the code is what is actually being done. So it really is just semantics of whether the variable/string names should follow the fake terms/paradigm, or the true code implementation.

Trouffman · 2024-08-24T20:57:31Z

Where did you find that?
I looked in VLC after installing NDI Tools and could use the feature but could not find that UI setting.

IIRC : Add VLC source then select a video file to playback. Then double click on that file.

I really like the difference between pause and stop as well, hence why I kept them in the visible part.

The "stop" in the variable name is kept as it is closer to the actual "logic". So the inconsistency should be between the front facing and the internal logic for it only. (Aka just us, the devs/maintainers.

I don't mind changing it.

I don't like the large if/else if part for the configuration migration. > Maybe we should have a function at loading that is "migrate to new settings" where we only address the configuration changes (so the check happen only when loading the plugin (aka once), instead of everytime it is used. - not sure how that would work tbh.)

…loading a scene in preview and no NDI stream is detected (at launch)

Trouffman

There is still a bug that the NDI Source does not display when:

OBS start
Source is ONLY in a scene in Preview
Source is not available (so not connected)

Still figuring out why this is a problem.

EDIT : This is only happening with NDI source that do not send constant stream (like static image with no audio).
This can happen with "NDI Pattern Generator". Could not reproduce this with any other NDI Sources.

Trouffman · 2024-09-09T14:47:39Z

Confirming the following :

New behavior options

Keepalive
Source stays connected even when not visible / not in-use
Stop & Reset
Source disconnect when not displayed (preview/program/display) - Reset the last received frame when it becomes visible/used again
NB: this can appear as fast "blink-once" for the source in slow environment
Pause
Source disconnect when not displayed (preview/program/display) - keep the last received frame when it becomes visible/used again
NB: This can appear as a tiny "start-lag" for source in slow environment. If the source has not received anything since the last OBS start, this will be transparent/not visible.

New default setting

The default behavior for any new NDI Source is : the "Pause" option.

Migration from previous settings

Any NDI Source that have been configured / created in prior versions will migrate to the new behavior automatically as below:

~~

NDI Source had no specific "behavior" or custom behavior as "keep connected" settings -> New setting = Keepalive
This is consistent with the previous behavior and should be a transparent migration
Note for future reviewer : the "keep last frame" has no effect with this use-case, so it is not checked.
NDI source had a custom "disconnect" behavior (introduced in 4.14.0)
If "keep last frame" is ticked -> New setting = Pause
If "keep last frame" is not ticked -> New setting = Stop & Reset
~~

EDIT : we had to drop the "backward compatibility port" this is the new migration behavior:

Any NDI Source that have been configured / created in prior v6.0.0 versions will migrate to the new behavior automatically to "KeepAlive"

Argument to support this new default behavior

Non-breaking for previously configured sources
Should reduce the bandwidth usage and "performance" issues on most systems (not running on 10G+)
Should reduce the number of support request for "performance", "bandwidth usage", "NDI video lag", or "latency issues"

Trouffman · 2024-09-09T16:54:23Z

src/ndi-source.cpp

 		(s->config.hw_accel_enabled != new_hw_accel_enabled);
 	s->config.hw_accel_enabled = new_hw_accel_enabled;

+	// Clean the frame content when settings change unless requested otherwise


THis might be useful only for th eaudio only afterall.

Trouffman · 2024-09-10T02:30:43Z

Seems recent changes on the main branch are impacting.

Known issue so far.

Issue 1 :

When an NDI source is connected to the NDI Pattern Generator with a static tone, there is no sound in audio / recording right after OBS launch.

This could be reproduced with other sources as well (inconsistently)
This happen when using "Pause" behavior

Issue 2 :

Changing the Source name in the drop down (settings tab of a source) > This does not change the source right away, neither when clicking save. The source must be toggle on/off for this to apply.
This happen when using "Pause" behavior

Issue 3 :
editing a scene, then clicking "cancel" will still reset the frame.
This happen when using "Reset"
NB : this does NOT happen if clicking the X icon to close. (seem part of the ""ndi_source_update"

12:02:43.179: [DistroAV] 'NDI 1' +ndi_source_update(…)
12:02:43.180: [DistroAV] 'NDI 1' ndi_source_update: Behavior Blank Frame: Deactivate source output video (Actively reset the frame content)
12:02:43.180: [DistroAV] ndi_source_update: 'NDI 1' behavior='stop_resume_blank'
12:02:43.180: [DistroAV] 'NDI 1' ndi_source_update: NDI Source 'DELLICIOUS (OBS)' selected.
12:02:43.180: [DistroAV] 'NDI 1' -ndi_source_update(…)

More being tested before this can be merged.

Trouffman · 2024-09-10T04:25:49Z

Confirmed : these issues are from the main 6.0.0, not this one.

RayneYoruka · 2024-09-10T04:33:51Z

Seems recent changes on the main branch are impacting.

Known issue so far.

Issue 1 :

When an NDI source is connected to the NDI Pattern Generator with a static tone, there is no sound in audio / recording right after OBS launch.

It's really interesting I haven't caught this bug, specially with how much I've been messing with the audio via NDI lately (Like changing audio sources on the fly)

BitRate27 · 2024-09-17T12:45:11Z

It doesn't look like we are setting reset_ndi_receiver to true when the behavior changes. Is this on purpose?

…his help solves some dev challenges, and stay uniform with the way the other settings are managed already. Drawback : This introduce a breaking change for user of v4.14.x that configured a custom behavior.

Trouffman · 2024-09-27T23:14:07Z

It doesn't look like we are setting reset_ndi_receiver to true when the behavior changes. Is this on purpose?

Yes this is expected as this will be set when the source settings is confirmed, not when we pick from the dropdown.

The issue was because we were not checking the frame to remenber properly.

In this latest commit i had to get rid of all the "text-based" settings for the behavior and fallback to using int. It simplifies the codebase and it seems to work great, solving the challenges we had recently.

There should be some more clean-up to be done before release, but this should resolve most of the bugs we had identified for this release.

Add support for detecting changes in ndi_source_name when editing the settings of a source. Add freeing up memoery for ndi_source_name changes. Add logic to clean the frame content when resetting the ndi_receiver thread.

Trouffman · 2024-10-01T14:30:13Z

The latest commit address issues when a change of ndi source name would not be detected properly and not reset the content on a change.
no
This has been tested locally (Mac) and seems to react properly to the changes in behavior options and source name.

Trouffman · 2024-10-02T13:39:50Z

New behavior options

Keepalive
Source stays connected even when not visible / not in-use
Stop & Reset
Source disconnect when not displayed (preview/program/display) - Reset the last received frame when it becomes visible/used again
NB: this can appear as fast "blink-once" for the source in slow environment
Pause
Source disconnect when not displayed (preview/program/display) - keep the last received frame when it becomes visible/used again
NB: This can appear as a tiny "start-lag" for source in slow environment. If the source has not received anything since the last OBS start, this will be transparent/not visible.

New default setting

The default behavior for any new NDI Source is : the "Pause" option.

Migration from previous settings

Any NDI Source that have been configured / created in prior v6.0.0 versions will migrate to the new behavior automatically to "KeepAlive"

All "custom" behavior set in 4.14.x are reverted to KeepAlive - This is a breaking change.

patcreak

im confused

source: WIP/RFC/discussion about how to make Behavior more intuitive

558e207

paulpv requested review from RayneYoruka and Trouffman August 9, 2024 22:28

paulpv self-assigned this Aug 9, 2024

paulpv changed the base branch from master to 6.0.0_actual August 9, 2024 22:35

Merge branch '6.0.0_actual' into 6.0.0_behavior_text

761bdb2

paulpv marked this pull request as ready for review August 14, 2024 09:48

Merge branch '6.0.0_actual' into 6.0.0_behavior_text

c7e535e

paulpv marked this pull request as draft August 14, 2024 21:58

paulpv added 2 commits August 15, 2024 02:28

Merge branch '6.0.0_actual' into 6.0.0_behavior_text

55280e7

Merge branch '6.0.0_actual' into 6.0.0_behavior_text

3ad5de6

Base automatically changed from 6.0.0_actual to master August 16, 2024 06:26

Trouffman reviewed Aug 18, 2024

View reviewed changes

Trouffman added 2 commits August 24, 2024 02:57

Update text to be more friendly, rename variables and reworked backwa…

0a76fb6

…rd compatibility.

formating

7236756

re ordering + adding comments. Still a bug that no scen display when …

7f5d3bb

…loading a scene in preview and no NDI stream is detected (at launch)

Trouffman reviewed Sep 3, 2024

View reviewed changes

Trouffman added 2 commits September 3, 2024 16:16

Extra clean-up or deprectaed fucntions

f49aded

clang Format

cf83bcc

Trouffman changed the title ~~6.0.0 behavior text~~ 6.0.0 "Visibility behavior" update (Text & logic) Sep 3, 2024

Merge branch 'master' into 6.0.0_behavior_text

eb94ea2

Update all languages

dabd98f

Trouffman mentioned this pull request Sep 4, 2024

Clearing confusion with "Behavior" settings #1046

Closed

Trouffman added 2 commits September 9, 2024 10:51

Changed default Source behavior to "Pause". See PR Comment for logic.

98de9d3

Merge branch 'master' into 6.0.0_behavior_text

89555d0

Trouffman added the !DO NOT MERGE! For PRs that should not be merged (incomplete or just PoC) label Sep 9, 2024

Trouffman reviewed Sep 9, 2024

View reviewed changes

Rework of all behavior settings logic to use int instead of string. T…

be1e740

…his help solves some dev challenges, and stay uniform with the way the other settings are managed already. Drawback : This introduce a breaking change for user of v4.14.x that configured a custom behavior.

BitRate27 and others added 5 commits September 28, 2024 07:45

Copy ndi_source_name string instead of pointer to string

011d921

Remove const from ndi_source_name

e0c2e4f

Use bstrdup instead of bzalloc

0a72abf

Add a fix for cleaning the frame when ther receiver is reset.

ed6ab3e

Merge pull request #1100 from BitRate27/copystr

7ce672b

Add support for detecting changes in ndi_source_name when editing the settings of a source. Add freeing up memoery for ndi_source_name changes. Add logic to clean the frame content when resetting the ndi_receiver thread.

Trouffman self-requested a review October 2, 2024 13:23

Trouffman approved these changes Oct 2, 2024

View reviewed changes

Trouffman merged commit 86fc73f into master Oct 2, 2024

Trouffman deleted the 6.0.0_behavior_text branch October 2, 2024 13:40

Trouffman removed the !DO NOT MERGE! For PRs that should not be merged (incomplete or just PoC) label Oct 2, 2024

Trouffman mentioned this pull request Oct 5, 2024

Recognize ndi_source_name change #1092

Closed

RayneYoruka mentioned this pull request Feb 8, 2025

Feature Req: "mute" filter when parent source is disabled/muted in OBS #138

Closed

patcreak reviewed Apr 13, 2025

View reviewed changes

This comment was marked as off-topic.

Sign in to view

DistroAV deleted a comment from RayneYoruka Apr 13, 2025

		// ISSUE: Do we need to do both this and in `ndi_source_update`?
		if (recv_desc.bandwidth ==

Uh oh!

6.0.0 "Visibility behavior" update (Text & logic) #1074

6.0.0 "Visibility behavior" update (Text & logic) #1074

Uh oh!

Conversation

paulpv commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trouffman commented Aug 16, 2024

Uh oh!

RayneYoruka commented Aug 16, 2024

Uh oh!

Trouffman commented Aug 17, 2024

Uh oh!

Trouffman Aug 18, 2024

Choose a reason for hiding this comment

Uh oh!

Trouffman Aug 18, 2024

Choose a reason for hiding this comment

Uh oh!

Trouffman commented Aug 24, 2024

Uh oh!

RayneYoruka commented Aug 24, 2024

Uh oh!

Trouffman commented Aug 24, 2024

Uh oh!

paulpv commented Aug 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trouffman commented Aug 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trouffman left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trouffman commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

New behavior options

New default setting

Migration from previous settings

Argument to support this new default behavior

Uh oh!

Trouffman Sep 9, 2024

Choose a reason for hiding this comment

Uh oh!

Trouffman commented Sep 10, 2024

Uh oh!

Trouffman commented Sep 10, 2024

Uh oh!

RayneYoruka commented Sep 10, 2024

Uh oh!

BitRate27 commented Sep 17, 2024

Uh oh!

Trouffman commented Sep 27, 2024

Uh oh!

Trouffman commented Oct 1, 2024

Uh oh!

Trouffman commented Oct 2, 2024

New behavior options

New default setting

Migration from previous settings

Uh oh!

patcreak left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

paulpv commented Aug 9, 2024 •

edited

Loading

paulpv commented Aug 24, 2024 •

edited

Loading

Trouffman commented Aug 24, 2024 •

edited

Loading

Trouffman left a comment •

edited

Loading

Trouffman commented Sep 9, 2024 •

edited

Loading