Skip to content

Conversation

@pobrn
Copy link
Contributor

@pobrn pobrn commented Aug 15, 2025

Description + Motivation and Context

A node might choose to use an enumerated choice pod to report the set of
supported sizes. Currently that is not supported. And what's worse, since
SPA_POD_OPT_Rectangle() is used, if the node uses an enum choice, then
{,this_}resolution stays uninitialized but spa_pod_parse_object() will
not report errors because the field is considered optional.

Fix this by using spa_pod_get_values(), which can be used for concrete
values, and both SPA_CHOICE_{None,Enum}.

Since the resolutions are parsed in two separate places, and since the
frame rates require essentially the same treatment a new function is
introduced to handle the common parts.

How Has This Been Tested?

With the current pipewire master branch, with a video node that uses enumerated choice for size, as well as nodes that don't use enumerated choices.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

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.

@pobrn pobrn force-pushed the pw_cam_size_choice branch 2 times, most recently from 2111490 to aca61e3 Compare August 15, 2025 18:07
@WizardCM WizardCM added the Bug Fix Non-breaking change which fixes an issue label Aug 16, 2025
@PatTheMav
Copy link
Member

Why is this PR opened as a Draft PR? In general we want to keep the amount of Draft PRs to a minimum (ideally zero) as those clutter the already-growing list of open PRs.

If this PR is ready for production, it should also be ready for review. If it is not ready for review, it should not have been opened.

@pobrn pobrn force-pushed the pw_cam_size_choice branch from aca61e3 to b830979 Compare August 21, 2025 16:58
@pobrn
Copy link
Contributor Author

pobrn commented Aug 21, 2025

Oh, okay.

@pobrn pobrn marked this pull request as ready for review August 21, 2025 17:00
@pobrn pobrn force-pushed the pw_cam_size_choice branch 2 times, most recently from 7db3dab to c6c927e Compare August 28, 2025 16:21
@pobrn
Copy link
Contributor Author

pobrn commented Aug 28, 2025

Rebased after #11741.

@pobrn
Copy link
Contributor Author

pobrn commented Sep 30, 2025

Is there anything I can do to help move this forward?

@pobrn pobrn force-pushed the pw_cam_size_choice branch from c6c927e to 4c385fd Compare September 30, 2025 15:52
@tytan652 tytan652 added the UI/UX Anything to do with changes or additions to UI/UX elements. label Sep 30, 2025
@tytan652
Copy link
Collaborator

tytan652 commented Sep 30, 2025

Since this PR is changing the UI/UX, you have to add screenshot of the properties window with enumerated sizes.

The PR should be blocked until it's done.

Edit: The second commit also breaks the expected UI/UX set in #11741

@tytan652 tytan652 added Enhancement Improvement to existing functionality New Feature New feature or plugin and removed Bug Fix Non-breaking change which fixes an issue labels Sep 30, 2025
@pobrn
Copy link
Contributor Author

pobrn commented Sep 30, 2025

Since this PR is changing the UI/UX, you have to add screenshot of the properties window with enumerated sizes.

I'm not sure it does? Or you mean the second commit?

Edit: The second commit also breaks the expected UI/UX set in #11741

I'm not sure what to do. Video nodes backed by libcamera do not advertise frame rates (at least as of yet). I think omitting the frame rate from the format description is better than ignoring the format param altogether (making the camera unusable). Any suggestions?


you have to add screenshot of the properties window with enumerated sizes

`pw-dump` showing the format
        "EnumFormat": [
          ...
          {
            "mediaType": "video",
            "mediaSubtype": "raw",
            "format": "YUY2",
            "size": {
              "default": { "width": 640, "height": 480 },
              "alt1": { "width": 176, "height": 144 },
              "alt2": { "width": 320, "height": 240 },
              "alt3": { "width": 352, "height": 288 },
              "alt4": { "width": 640, "height": 360 },
              "alt5": { "width": 640, "height": 480 }
            },
            "colorRange": "16-235",
            "colorMatrix": "bt601",
            "transferFunction": "bt709",
            "colorPrimaries": "bt709"
          }
        ],
obs interface showing the formats kép

@jcm93
Copy link
Contributor

jcm93 commented Oct 6, 2025

For what it's worth, the choice I made in #10895 was also to omit a parameter in the format string if that parameter was empty or otherwise meaningless (and a format composed of all-meaningless values would be coalesced to the string "Default"). On macOS this was very rare, but could be the case for devices like wired iPhone screen sharing.

On macOS, the source of truth for this source type is also just whatever frame data we ultimately actually receive; the format object is something of a glorified hint for the internal logic, and is not strictly relied upon for the mechanics of the source to work.

@tytan652
Copy link
Collaborator

tytan652 commented Oct 6, 2025

I guess I made a bad assumption that every camera node would have framerate in their formats (but the assumption was kind of existing, I just made it more "tangible"). But the

Then the UI/UX needs to be patched to be functional with an optional framerate (e.g. disable the framerate combobox).

@pobrn
Copy link
Contributor Author

pobrn commented Oct 6, 2025

Then the UI/UX needs to be patched to be functional with an optional framerate (e.g. disable the framerate combobox).

Please see the latest commit. Is that what you had in mind?

Details image image

@pobrn
Copy link
Contributor Author

pobrn commented Nov 1, 2025

Is there any way this can progress? I see #12781 addresses the same thing as the last commit here.

@tytan652
Copy link
Collaborator

tytan652 commented Nov 2, 2025

#12781 is a bug fix for a specific "regression" that appeared OBS 32 meant to be merge for a patch release without adding features.

This PR is a feature addition.

PS: There is an overlap, but this PR intends to add support for enumerated sizes

pobrn added 2 commits January 17, 2026 10:46
Add a new function `get_values_from_pod()` to provide a wrapper around
`spa_pod_get_values()` with extra checks, and that ignores the default
value for `SPA_CHOICE_Enum`.
A node might choose to use an enumerated choice pod to report the set of
supported sizes. Currently that is not supported. And what's worse, since
`SPA_POD_OPT_Rectangle()` is used, if the node uses an enum choice, then
`{,this_}resolution` stays uninitialized but `spa_pod_parse_object()` will
not report errors because the field is considered optional.

Fix this by handling `SPA_CHOICE_Enum` as well, and ignoring the unsupported
choice types.
@pobrn pobrn force-pushed the pw_cam_size_choice branch from 60ce58c to 4146312 Compare January 17, 2026 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement Improvement to existing functionality New Feature New feature or plugin UI/UX Anything to do with changes or additions to UI/UX elements.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants