obsproject
mac-avcapture: Use list-based format selector
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.
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
https://github.com/obsproject/obs-studio/pull/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:
- [x] My code has been run through clang-format.
- [x] I have read the contributing document.
- [x] My code is not on the master branch.
- [x] The code has been tested.
- [x] All commit messages are properly formatted and commits squashed where appropriate.
- [x] I have included updates to all appropriate documentation.
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.
Sidebar that I just thought of; now that the scaffolding is in place to store computed properties in an AVCaptureDeviceFormat+Comparable category, The format's list description (given by [OBSAVCapture descriptionFromFormat:] presently) could also be a lazily computed property in the same way, which would definitely be more efficient...
edit: The description is now also a computed property, and the category is now OBSListable.
Some submodules went weird on the latest commit, and in the process of fixing it I went ahead and rebased and squashed the commits.
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!
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:
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:
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?
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.
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.
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
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!
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 .
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.
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."
Just wanted to note that there are still two "changes requested" flags on this, but both of these were resolved a few weeks back, so should be given another pass.
Made one small fix; I wanted to make sure this PR logs the format a device is configuring with, and I noticed that we were logging the internal representation of the format (which is base64 scrambled) rather than the human readable version; now fixed. Also rebased; will squash commits whenever we get around to looking at this again post-31.
@RytoEX @jcm93 What's the status here? Can we make this land at the next best possible release?
As far as I am concerned it is in good shape, and I remain able to address any other feedback. I will rebase and squash the commits.
@RytoEX @jcm93 What's the status here? Can we make this land at the next best possible release?
This didn't land for 31.0. The intent at the time was to land it for the next release.
Per discussions out of band, I am adding one last feature to this in a separate commit; if the user selects a format that does not support their previously configured frame rate, we will fall back to an appropriate FPS supported by the format based on OBS's configured output frame rate. This fallback frame rate is determined by the new function fallbackFrameRateForFormat:.
After sitting on it, I think that the benefits are large enough that the addition of this functionality is merited. Namely, it seems like selecting a navigating to a format in the properties window should try to offer immediate feedback to help the user select a format, rather than requiring fiddling with the frame rate selector on each selection just to get a visual preview. Hitting cancel in the window is always a reliable "bail out" to return to the previous committed configuration.
While it is a relatively small addition, it's nontrivial enough that I am adding it in a separate commit so it can be looked at, and I can squash the commits later.
I will request re-reviews for the two approvals here since it seems appropriate.
Added a second commit to address some visual noise when there are a great many FPS ranges present on a format.
The approach taken in the second commit simplifies the FPS ranges description if there are more than ten ranges, showing the minimum and maximum FPS in the array of ranges and the number of ranges. This can happen if a card advertises many discrete values in the form of ranges, i.e. 30-30, 45-45, 60-60.
This scenario is currently disambiguated from a "genuine range" with a parenthetical indicating the number of ranges. So for example a format advertising 60 ranges between 1 and 120 fps would show "1 - 120 FPS (60 values)". While a genuine FPS range between 1-120 (i.e. any frame rate between 1-120 is supported) would show just "1 - 120 FPS".
This is not perfect, but it seems necessary to handle this edge case if a format has, for example, 60 FPS ranges, each representing discrete integers between 1 and 60.
My final request for this is gonna be to change the discrete FPS listings to be from low to high, so that it's consistent with FPS ranges being low to high.
Done. I went ahead and squashed all commits, so I reproduced the change below; it is just a one liner to enumerate the sorted ranges in reverse order when specifically generating the description. I also documented that it uses ascending order. Lastly I fixed a tiny issue where, for the "over 10 ranges" case, we were using the ranges from the API (presumably-sorted) rather than guaranteed-sorted, when getting the minimum and maximum rate.
---
plugins/mac-avcapture/OBSAVCapture.h | 2 +-
plugins/mac-avcapture/OBSAVCapture.m | 7 ++++---
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/plugins/mac-avcapture/OBSAVCapture.h b/plugins/mac-avcapture/OBSAVCapture.h
index 3f1a4216e..b0468d4fd 100644
--- a/plugins/mac-avcapture/OBSAVCapture.h
+++ b/plugins/mac-avcapture/OBSAVCapture.h
@@ -264,7 +264,7 @@ static const UInt32 kMaxFrameRateRangesInDescription = 10;
+ (FourCharCode)fourCharCodeFromFormat:(OBSAVCaptureVideoFormat)format withRange:(enum video_range_type)videoRange;
-/// Generates a string describing an array of frame rate ranges.
+/// Generates a string describing an array of frame rate ranges. The frame rate ranges are described in ascending order.
/// - Parameter ranges: [NSArray](https://developer.apple.com/documentation/foundation/nsarray?language=objc) of [AVFrameRateRange](https://developer.apple.com/documentation/avfoundation/avframeraterange?language=objc), such as might be provided by an `AVCaptureDeviceFormat` instance's [videoSupportedFrameRateRanges](https://developer.apple.com/documentation/avfoundation/avcapturedeviceformat/1387592-videosupportedframerateranges) property.
/// - Returns: A new [NSString](https://developer.apple.com/documentation/foundation/nsstring?language=objc) instance that describes the frame rate ranges.
+ (NSString *)frameRateDescription:(NSArray<AVFrameRateRange *> *)ranges;
diff --git a/plugins/mac-avcapture/OBSAVCapture.m b/plugins/mac-avcapture/OBSAVCapture.m
index 64118173a..1dab839b4 100644
--- a/plugins/mac-avcapture/OBSAVCapture.m
+++ b/plugins/mac-avcapture/OBSAVCapture.m
@@ -1053,7 +1053,7 @@ + (NSString *)frameRateDescription:(NSArray<AVFrameRateRange *> *)ranges
}];
NSString *frameRateDescription;
NSMutableArray *frameRateDescriptions = [[NSMutableArray alloc] initWithCapacity:ranges.count];
- for (AVFrameRateRange *range in sortedRangesDescending) {
+ for (AVFrameRateRange *range in [sortedRangesDescending reverseObjectEnumerator]) {
double minFrameRate = round(range.minFrameRate * 100) / 100;
double maxFrameRate = round(range.maxFrameRate * 100) / 100;
if (minFrameRate == maxFrameRate) {
@@ -1074,8 +1074,9 @@ + (NSString *)frameRateDescription:(NSArray<AVFrameRateRange *> *)ranges
frameRateDescription = [frameRateDescriptions componentsJoinedByString:@", "];
frameRateDescription = [frameRateDescription stringByAppendingString:@" FPS"];
} else if (frameRateDescriptions.count > kMaxFrameRateRangesInDescription) {
- frameRateDescription = [NSString stringWithFormat:@"%.0f-%.0f FPS (%lu values)", ranges.lastObject.minFrameRate,
- ranges.firstObject.maxFrameRate, ranges.count];
+ frameRateDescription =
+ [NSString stringWithFormat:@"%.0f-%.0f FPS (%lu values)", sortedRangesDescending.lastObject.minFrameRate,
+ sortedRangesDescending.firstObject.maxFrameRate, sortedRangesDescending.count];
}
return frameRateDescription;
}
--