Skip to content
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.

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

mac-avcapture: Use list-based format selector #10895

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jcm93
Copy link
Contributor

@jcm93 jcm93 commented Jun 19, 2024

Description

Updates the properties window for macOS mac-avcapture sources to use a unified list-based format selector when in the non-preset mode. Also adds a contextual warning in the properties window to show when a device has macOS system effects active.

Screenshot 2024-07-10 at 9 39 50 AMScreenshot 2024-07-10 at 12 06 52 AM

The frame rate selector is retained, so that we can continue to have explicit control over the device frame rate. The list selectors for resolution, color space, pixel format and video range, however, have all been consolidated inside the single format selector.

Motivation and Context

#9344 provided a much-needed update to the Video Capture Device source on macOS and added a lower-latency Capture Card Device source. A number of factors, however, have conspired to make the new version of the source harder to use, for both novice and adept users:

  • The new version does not reimplement the 'auto-populate parameters' feature of the source on other platforms. When a property is updated, the user must check all other properties to ensure that the combination of selected parameters is still a valid configuration, which is not always immediately apparent.
  • Because of "quirks" with OBS's frame rate selector and the default size for the source properties window, it is not apparent to many users that the list selectors for pixel format, video range and color space exist 'below the fold' in the source properties window.

This PR aims to simplify and improve the UI for the source while also reducing the complexity of the property selection code.

  • All parameters are now visible in the source properties window at its default size. The only parameter that can be invalid after changing the selected format is the frame rate, and its status should be more apparent within the context of the window.
  • It is easier to understand at a glance what combinations of parameters are valid for a given device. Making an appropriate selection should now be a quicker process.

Additionally, the new effects warning in the properties window should help to alleviate some user confusion. On macOS, enabling an effect such as Studio Light or Portrait mode for one video input will often enable them on all inputs (filed as FB13302833). Users are often confused to see webcam effects on e.g. capture card inputs. With this PR, the source properties window displays a warning for active system effects, which should make this issue more apparent.

How Has This Been Tested?

Tested locally on an M1 Max machine running the macOS Sequoia beta with all video capture devices on hand, including the built-in webcam, Continuity Camera from an iPhone, a virtual camera, and a single capture card from a popular brand, to ensure both OBS source types remained in good working order.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Tweak (non-breaking change to improve existing functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)

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.

@WizardCM WizardCM added Bug Fix Non-breaking change which fixes an issue Enhancement Improvement to existing functionality macOS Affects macOS labels Jun 20, 2024
@derrod derrod self-requested a review June 22, 2024 17:53
Copy link
Member

@PatTheMav PatTheMav left a comment

Choose a reason for hiding this comment

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

Left some code-specific comments, overall I'd like to see more discussion about:

  1. Presentation of the formats in the dropdown (@Warchamp7 got some ideas there?)
  2. How to "upgrade" existing source configurations

@jcm93 jcm93 force-pushed the avcapturelist branch 5 times, most recently from af5e538 to 9ef9d69 Compare June 27, 2024 22:16
@jcm93
Copy link
Contributor Author

jcm93 commented Jun 28, 2024

Couple misc changes:

  • Fixed the reaction effects property issue
  • We were checking if the received buffers differed from the configured format by checking the video range; it seemed simpler to check the media subtype (fourCC) instead, since that will detect if (for whatever reason) the pixel format changes but not the range.

Secondly, added a migration routine in configureSession to attempt to configure with the legacy properties if there is no value for the new supported_format key. This seems to me like the only thing we need to do to cleanly migrate from the old properties. All new sources will contain just the supported_format key-value, and supported_format will start superseding the old properties for existing sources once the properties window for an existing source is opened. Tested with a couple sources to make sure migration worked.

Hopefully this should be in a pretty good state now.

Was intending to squash at the end, just let know if you want me to squash whenever.

@jcm93 jcm93 force-pushed the avcapturelist branch 2 times, most recently from 9719b24 to f5e0f4f Compare July 9, 2024 18:49
@jcm93

This comment was marked as resolved.

@jcm93 jcm93 force-pushed the avcapturelist branch 2 times, most recently from 829f71e to 57b315a Compare July 9, 2024 20:31
@jcm93 jcm93 force-pushed the avcapturelist branch 2 times, most recently from 1a81579 to 25ef7c3 Compare July 24, 2024 15:27
@jcm93
Copy link
Contributor Author

jcm93 commented Jul 24, 2024

Some submodules went weird on the latest commit, and in the process of fixing it I went ahead and rebased and squashed the commits.

@Warchamp7
Copy link
Member

Warchamp7 commented Jul 31, 2024

Got on a bit of a roll so went ahead and revamped sorting with my idea so it could be tested. Refactored to use sort descriptors on top of the computed properties.

Currently sorts by aspect ratio (rounded to two places) descending, then 'pixels per second' (i.e. resolution * fps) descending, then 'bits per pixel' ascending, with the idea that you would present less bandwidth-intense formats first for the same pixel throughput. Couple screenshots beneath; certainly needs more testing.

Assuming the images in this conversation are the current state of the lists, this looks great to me!

@jcm93
Copy link
Contributor Author

jcm93 commented Jul 31, 2024

The only difference from there to the current state is the decimal formatting. Here is the current state of the list format for the devices I have available:
Screenshot 2024-07-31 at 12 26 35 PMScreenshot 2024-07-31 at 12 26 48 PMScreenshot 2024-07-31 at 12 26 48 PM

@PatTheMav
Copy link
Member

The only difference from there to the current state is the decimal formatting. Here is the current state of the list format for the devices I have available: Screenshot 2024-07-31 at 12 26 35 PMScreenshot 2024-07-31 at 12 26 48 PMScreenshot 2024-07-31 at 12 26 48 PM

The third screenshot still show separate framerates, is that because each is its own framerate range or is that just an old screenshot?

Also I don't know what "Default" is supposed to mean in the second screenshot? Is it undefined? OBS will need to interpret it as some value, so which one is it and why not show the user?

@jcm93
Copy link
Contributor Author

jcm93 commented Aug 1, 2024

In the third screenshot, yes, each of those is its own frame rate range on the format where the range represents one discrete value (min and max are equal). I do not have a good sense of how common of a pattern this is. If it is very common, with long lists of ranges on each format, I think I might need to reconsider the string representation to be more simple for large numbers of frame rate ranges.

In the second screenshot, this is just the weird way Continuity Camera advertises itself. Its formats have no YUV matrix attached (or primaries or transfer function), so OBS concludes VIDEO_CS_DEFAULT. This is existing behavior. When OBS eventually receives sample buffers from Continuity Camera, they seem to be in the 709 color space. I agree that this isn't super desirable, but I don't think there's a great way around it without very special handling.

@PatTheMav
Copy link
Member

In the third screenshot, yes, each of those is its own frame rate range on the format where the range represents one discrete value (min and max are equal). I do not have a good sense of how common of a pattern this is. If it is very common, with long lists of ranges on each format, I think I might need to reconsider the string representation to be more simple for large numbers of frame rate ranges.

In the second screenshot, this is just the weird way Continuity Camera advertises itself. Its formats have no YUV matrix attached (or primaries or transfer function), so OBS concludes VIDEO_CS_DEFAULT. This is existing behavior. When OBS eventually receives sample buffers from Continuity Camera, they seem to be in the 709 color space. I agree that this isn't super desirable, but I don't think there's a great way around it without very special handling.

Right, yeah I dunno if the color space selection even makes sense, though I reported the discrepancy between selection and what is actually used in the sample buffers to Apple and I haven't checked if this has been changed in macOS 15 yet.

@Warchamp7
Copy link
Member

The only difference from there to the current state is the decimal formatting. Here is the current state of the list format for the devices I have available:

In that case this looks good on my end from the UI side

@PatTheMav
Copy link
Member

@jcm93 Could you rebase this please?

@jcm93
Copy link
Contributor Author

jcm93 commented Aug 20, 2024

Rebased, and also changed AVCaptureDeviceFormat+OBSListable.m's - (NSString *)obsPropertyListDescription to more gracefully handle bad values.

I chose to just not include string values if they are invalid, unspecified, or unknown. So if a device provides resolution, color space, pixel format, but not FPS, the string description will just be "<resolution> - <color space> - <pixel format>", instead of showing " fps" for an unspecified fps. This can sometimes happen for color spaces, i.e. Continuity Camera does not specify a color space in its format advertisements.

If the resultant string is completely empty, that means that all parameters are invalid, unspecified, or unknown. For this case, we show "Default" for the format instead of an empty string. This occurs with iPhone screen mirroring.

Hopefully (!) this PR should now be in a good state!

@Fenrirthviti Fenrirthviti added the Seeking Testers Build artifacts on CI label Aug 20, 2024
@RytoEX
Copy link
Member

RytoEX commented Sep 6, 2024

I'd like to see an approval from someone who runs macOS daily and I'd also like to see a UI/UX approval from @Warchamp7 .

@Warchamp7 Warchamp7 self-requested a review September 9, 2024 18:22
Copy link
Member

@PatTheMav PatTheMav left a comment

Choose a reason for hiding this comment

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

Looks fine apart from my other comment(s), also worked as expected in local tests.

plugins/mac-avcapture/OBSAVCapture.m Show resolved Hide resolved
@jcm93
Copy link
Contributor Author

jcm93 commented Sep 12, 2024

Per discussion out-of-band, we needed to add an internal representation for the value of device formats separate from their human-readable descriptions. This is so that we can freely change the format of device format descriptions if we want to without requiring any special handling to migrate sources. I did this in a very simplistic way with obsPropertyListInternalRepresentation, serializing string values with the dimensions, frame rate ranges, and raw values for the OBS color space enum and the CMFormatDescription subtype. I would welcome any scrutiny on whether there is a superior way to represent these internally than what I picked here.

For ease of review I did this as a separate commit, just let me know anytime you wish for me to squash.

@jcm93 jcm93 force-pushed the avcapturelist branch 2 times, most recently from ac4d7ef to d8db3c1 Compare September 14, 2024 05:43
@jcm93
Copy link
Contributor Author

jcm93 commented Sep 14, 2024

Revised to add a trivial base64 mask / hash for the internal value used by OBS to identify device formats. Leaving this value human-readable risked the impression that this value could or should be modified by users.

Masking this value also makes it a bit more difficult for OBS to elegantly respond in the event of a (hypothetical) future breakage in macOS that causes these four values to no longer be sufficient for selecting a format/configuring a device. But looking forward, a device format should still always be encapsulated by resolution, supported frame rate ranges, four character code, and color space, all four of which are included in the mask. So this mask seems to be fairly "future proof."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Non-breaking change which fixes an issue Enhancement Improvement to existing functionality macOS Affects macOS Seeking Testers Build artifacts on CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants