Skip to content

addref to obs_source_t* before getProperties #1701

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.

Sign up for GitHub

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

Merged
sandboxcoder merged 2 commits into from
May 21, 2026
Merged

addref to obs_source_t* before getProperties #1701

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
167efab
addref to obs_source_t* before getProperties
sandboxcoder May 20, 2026
586a064
use OBSSource to addref
sandboxcoder May 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions obs-studio-server/source/osn-source.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,8 +250,10 @@ void osn::Source::IsConfigurable(void *data, const int64_t id, const std::vector

void osn::Source::GetProperties(void *data, const int64_t id, const std::vector<ipc::value> &args, std::vector<ipc::value> &rval)
{
// Attempt to find the source asked to load.
obs_source_t *src = osn::Source::Manager::GetInstance().find(args[0].value_union.ui64);
// Attempt to find the source asked to load. Get a strong reference to help
// guarantee the source is not destroyed while we are attempting to retrieve
// properties.
OBSSource src = osn::Source::Manager::GetInstance().find(args[0].value_union.ui64);
Comment on lines +253 to +256
Copy link
Copy Markdown
Contributor Author

@sandboxcoder sandboxcoder May 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good suggestion. However,osn::Source::Manager is a generic Singleton container. This suggestion means the ref counting belongs at the container level where we increment the ref count upon assignment (actually, the suggestion actually means just add a new findStrongRef function)

Copy link
Copy Markdown
Contributor Author

@sandboxcoder sandboxcoder May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I do not think I need to add this since OBSSource should implicitly handle this.

Copy link
Copy Markdown
Contributor Author

@sandboxcoder sandboxcoder May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed here: #1707

if (src == nullptr) {
Comment thread
sandboxcoder marked this conversation as resolved.
Comment thread
sandboxcoder marked this conversation as resolved.
PRETTY_ERROR_RETURN(ErrorCode::InvalidReference, "Source reference is not valid.");
}
Expand Down
31 changes: 31 additions & 0 deletions tests/osn-tests/src/test_osn_source.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,37 @@ describe(testName, () => {
});
});

it('Get properties of browser source while releasing concurrently does not crash', async function() {
if (obs.skipSource(EOBSInputTypes.BrowserSource)) { this.skip(); } // Skip if browser source is not supported
const iterations = 20;
const promises: Promise<void>[] = [];

for (let i = 0; i < iterations; i++) {
const input = osn.InputFactory.create(EOBSInputTypes.BrowserSource, 'browser_concurrent_' + i);
expect(input).to.not.equal(undefined, GetErrorMessage(ETestErrorMsg.CreateInput, EOBSInputTypes.BrowserSource));

Comment thread
sandboxcoder marked this conversation as resolved.
// Race getProperties against release on separate async ticks
const getProps = new Promise<void>(resolve => {
setImmediate(() => {
try { input.properties; } catch (_) {}
resolve();
});
});

const releaseSource = new Promise<void>(resolve => {
setImmediate(() => {
try { input.release(); } catch (_) {}
resolve();
});
});

promises.push(getProps, releaseSource);
}

// If the race condition is present, one of these will crash the OBS server process
await Promise.all(promises);
Comment on lines +594 to +622
Copy link
Copy Markdown
Contributor Author

@sandboxcoder sandboxcoder May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true, it appears this is mostly a stress test in its' current form. Would likely need to convert this into a C++ Catch2 test

Copy link
Copy Markdown
Contributor Author

@sandboxcoder sandboxcoder May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will hopefully be resolved by #1706

});

it('Set enabled and get it for all filter types', () => {
obs.filterTypes.forEach(function(filterType) {
logInfo(testName, 'Testing filter type: ' + filterType);
Expand Down
2 changes: 1 addition & 1 deletion tests/osn-tests/util/obs_handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,7 +527,7 @@ export class OBSHandler {
}

skipSource(inputType: string) {
if (process.platform === 'darwin') {
if (process.platform === 'darwin' && this.isCI()) {
if (inputType === 'browser_source' ||
inputType === 'window_capture' ||
inputType === 'monitor_capture' ||
Expand Down
Loading