Skip to content

Conversation

@gxalpha
Copy link
Member

@gxalpha gxalpha commented Aug 23, 2025

Description

With v1 of this function, it's unclear where exactly the data pointer comes from or what it is. In fact, this is not determined by libobs, but the consumer. libobs assumes that the caller of obs_property_button_clicked passes an obs_context_data pointer, and then passes the data pointer of that obs_context_data (which, btw, is an internal implementation detail!) as the data pointer to the callback.
In OBS Studio, this is always the private data of the associated object. However, this assumes that there even is such an object (source/encoder/etc), even though properties are meant to be free-standing. This is not just philosophical, because with obs_get_source_properties you can actually get an obs_properties_t that isn't associated with any specific source, at which point you have no idea what the data pointer will be.

For this reason, obs_properties_add_button v1 needs to go. obs_properties_add_button2 can be used as a drop-in replacement. With v2, it's well-defined that the pointer you're passing as priv is the pointer you get back in the callback as data. If you don't care about it, simply pass NULL/nullptr.

Once v1 is removed in the future, obs_property_button_clicked could be replaced with a variant that doesn't take a second argument, as that argument will no longer be used anywhere (although the existing second argument could also just be marked as unused, that would avoid an arguably unneeded breakage).

Contains #12528 to avoid conflicts (I assume that that PR will be merged before this one, but otherwise it's fine for this to be merged first and the other one to be closed).
Depends on obsproject/obs-browser#496.

Motivation and Context

This method is problematic as described above. obs_properties_add_button is basically a drop-in replacement that doesn't have those issues.

How Has This Been Tested?

macOS 26.
Tested that the "Syphon License" button of the syphon source still works.
Tested that the "Open Plug-in Interface" / "Close Plug-in Interface" buttons of the VST filter properties still work.

Screenshot of the docs:
image

CI will fail until obsproject/obs-browser#496 is merged and the submodule is updated.

Types of changes

  • Tweak (non-breaking change to improve existing functionality)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@gxalpha gxalpha added Enhancement Improvement to existing functionality Code Cleanup Non-breaking change which makes code smaller or more readable labels Aug 23, 2025
@gxalpha
Copy link
Member Author

gxalpha commented Aug 26, 2025

Rebased on master now that the browser update is merged (#12535), CI should work now.

v1 of obs_properties_add_button will be deprecated soon.
v1 of obs_properties_add_button will be deprecated soon.
v1 of obs_properties_add_button will be deprecated soon.
Also fixes UNUSED_PARAMETER calls in the callbacks for parameters that
are not actually unused, and moves the other ones to the top (as in most
of the rest of the project).
With v1 of this function, it's unclear where exactly the data pointer
comes from or what it is. In fact, this is not determined by libobs, but
the consumer. libobs assumes that the caller of
obs_property_button_clicked passes an obs_context_data pointer, and then
passes the data pointer of that obs_context_data as the data pointer to
the callback.
In OBS Studio, this is always the private data of the associated object.
However, this assumes that there even is such an object (source/encoder/
etc), even though properties are meant to be free-standing. This is not
just philosophical, because with obs_get_source_properties you can
actually get an obs_properties_t that isn't associated with any specific
source, at which point you have no idea what the data pointer will be.

For this reason, obs_properties_add_button v1 needs to go.
obs_properties_add_button2 can be used as a drop-in replacement.
With v2, it's well-defined that the pointer you're passing as priv is
the pointer you get back in the callback as data. If you don't care
about it, simply pass NULL/nullptr.

Once v1 is removed in the future, obs_property_button_clicked should be
replaced with a variant that doesn't take a second argument, as that
argument will no longer be used anywhere.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Code Cleanup Non-breaking change which makes code smaller or more readable Enhancement Improvement to existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants