[camera_avfoundation] enable more than 30 fps #7394

misos1 · 2024-08-12T17:19:15Z

Camera plugin was crashing when I tried to set fps to 60 on most media presets (except maybe on 1280x720, although tested device supports 60 fps for up to 1920x1440 and can do 240 fps on 1280x720) because when is activeVideoMinFrameDuration and activeVideoMaxFrameDuration set to fps outside of what active format supports it throws exception. Now it tries to find a format which supports fps closest to wanted fps and clamps it if it cannot be set to exact value to prevent crashes. It searches for formats with the exact same resolution. For example in format list it can be like "1920x1080 { 3- 30 fps}", "1920x1080 { 3- 60 fps}" and "1920x1080 { 6-120 fps}, but when setting sessionPreset then "1920x1080 { 3- 30 fps}" is selected by default. On the tested device there are 2 "media subtypes" 420f and 420v for each format where the first is denoted as "supports wide color" in debug description and the system has tendency to choose this one. So it tries to preserve the media subtype to what is preferred by the system and this is also added to highestResolutionFormatForCaptureDevice (with lower priority than max resolution/fps). Also there was nested lockForConfiguration and unlockForConfiguration when using ResolutionPreset.max together with setting up fps.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

stuartmorgan · 2024-08-21T11:46:30Z

@misos1 Could you split this into two different PRs? Having non-trivial changes to two unrelated plugins in the same PR makes review much more difficult.

hellohuanlin · 2024-08-27T18:10:50Z

packages/camera/camera_avfoundation/CHANGELOG.md


+* Adds possibility to use any supported FPS and fixes crash when using unsupported FPS.


*Adds ability to...

hellohuanlin · 2024-08-27T18:14:27Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

+      double bestFrameRate = [self frameRateForFormat:bestFormat closestTo:targetFrameRate];
+      double minDistance = fabs(bestFrameRate - targetFrameRate);
+      int bestSubTypeScore = 1;
+      for (AVCaptureDeviceFormat *format in _captureDevice.formats) {


can you also move this to a helper function?

stuartmorgan · 2024-08-30T18:02:23Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

@@ -211,20 +211,50 @@ - (instancetype)initWithMediaSettings:(FCPPlatformMediaSettings *)mediaSettings
  [_motionManager startAccelerometerUpdates];

  if (_mediaSettings.framesPerSecond) {
+    [_mediaSettingsAVWrapper beginConfigurationForSession:_videoCaptureSession];
+
+    // Possible values for presets are hard-coded in FLT interface having


Why is all this code moving out of the locked section; is that safe?

I did not see anything saying that the order of beginConfiguration and lockForConfiguration is important or that setting the session preset must be under that lock. There is example where it is used in that order: https://developer.apple.com/documentation/avfoundation/avcapturedevice/1389221-activeformat?language=objc

I moved setCaptureSessionPreset outside of lock because it can itself call lockForConfiguration and unlockForConfiguration and I did not see anything about whether that lock is recursive.

Seems lockForConfiguration is actually recursive from my observation (I could not get mentioned exceptions until now) as locking twice and unlocking once does not cause exceptions but unlocking twice yes. So I can revert this change (?).

Also it seems not all commitConfiguration calls go through _mediaSettingsAVWrapper.

stuartmorgan · 2024-08-30T18:05:06Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

-        [_captureDevice unlockForConfiguration];
-        return nil;
+      // find the format which frame rate ranges are closest to the wanted frame rate
+      CMVideoDimensions targetRes = self.videoDimensionsForFormat(_captureDevice.activeFormat);


You've added several calls to self in this init method. The fact that there was already such a call is a problem, and that should not be made worse by adding more.

If there are general, stateless utilities that you want to call from init, they should be turned into freestanding static functions.

stuartmorgan · 2024-08-30T18:06:24Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

-        [_videoCaptureSession commitConfiguration];
-        [_captureDevice unlockForConfiguration];
-        return nil;
+      // find the format which frame rate ranges are closest to the wanted frame rate


Comments need to follow standard sentence rules. https://google.github.io/styleguide/objcguide.html#comments

stuartmorgan · 2024-08-30T18:06:58Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

-        [_captureDevice unlockForConfiguration];
-        return nil;
+      // find the format which frame rate ranges are closest to the wanted frame rate
+      CMVideoDimensions targetRes = self.videoDimensionsForFormat(_captureDevice.activeFormat);


Please don't abbreviate variable names. https://google.github.io/styleguide/objcguide.html#naming

stuartmorgan · 2024-08-30T18:09:44Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

+        FourCharCode subType = CMFormatDescriptionGetMediaSubType(format.formatDescription);
+        int subTypeScore = subType == preferredSubType ? 1 : 0;
+        if (distance < minDistance ||
+            (distance == minDistance && subTypeScore > bestSubTypeScore)) {


I'm confused by this condition. bestSubTypeScore is initialized to 1, and subTypeScore is always either 0 or 1. How could this last piece of the condition ever be true?

I'm not clear on what this score even is; this whole algorithm needs some explanatory comment at the beginning.

if distance < minDistance then bestSubTypeScore can be overwritten by 0. This line explains it: subTypeScore = subType == preferredSubType ? 1 : 0, the score of the subtype is 1 when it is the preferred subtype and 0 otherwise. This is how to choose by minimum distance and then from formats which have equal distance choose that one with preferred subtype, it gives to subtypes some order. Instead of subTypeScore > bestSubTypeScore there could be something like bestSubType != preferredSubType && subType == preferredSubType but that makes it less clear as it deviates from simple way how to order something by multiple properties current.A < other.A || (current.A == other.A && current.B < other.B).

but that makes it less clear

I strongly disagree that abstracting the thing that matters—whether the current type is a preferred type—behind a completely arbitrary numeric value makes this code easier to understand. The code you have there that you described as "less clear" makes sense to me when I read it, whereas I did not understand what this score was, or why something that could only have two possible values was being called a score in the first place.

stuartmorgan · 2024-08-30T18:12:17Z

packages/camera/camera_avfoundation/CHANGELOG.md


+* Adds possibility to use any supported FPS and fixes crash when using unsupported FPS.


Where is the new test that triggers the codepath that used to crash?

When the real captureDevice.activeVideoMinFrameDuration (or ...Max...) is set to value outside of what is supported by active format it immediately throws an objc exception. For tests, mocked wrappers are used so this cannot be triggered. But the test testSettings_ShouldSelectFormatWhichSupports60FPS tries to set 60 fps which actually triggered this crash on my device for media presets other than 1280x720.

stuartmorgan · 2024-09-17T11:06:18Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

@@ -251,6 +254,55 @@ - (instancetype)initWithMediaSettings:(FCPPlatformMediaSettings *)mediaSettings
  return self;
 }

+static void selectBestFormatForRequestedFrameRate(


These helpers should be defined above their call sites; I'm suprised this is even compiling.

stuartmorgan · 2024-09-17T11:07:51Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

@@ -251,6 +254,55 @@ - (instancetype)initWithMediaSettings:(FCPPlatformMediaSettings *)mediaSettings
  return self;
 }

+static void selectBestFormatForRequestedFrameRate(


Per the Obj-C style guide, all declarations require declaration comments explaining them.

In this case, that should include covering what "select" means exactly, given that this doesn't return anything. Presumably it's modifying one or more arguments, but I can't tell without reading the implementation.

stuartmorgan · 2024-09-17T11:08:13Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

+  mediaSettings.framesPerSecond = @(bestFrameRate);
+}
+
+static double bestFrameRateForFormat(AVCaptureDeviceFormat *format, double targetFrameRate) {


This also needs a comment (e.g., explaining what "best" means).

stuartmorgan · 2024-09-17T11:14:43Z

...ages/camera/camera_avfoundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCam.m

+    double distance = fabs(frameRate - targetFrameRate);
+    FourCharCode subType = CMFormatDescriptionGetMediaSubType(format.formatDescription);
+    if (distance < minDistance || (distance == minDistance && subType == preferredSubType &&
+                                   bestSubType != preferredSubType)) {


Here and in the very similar code below, I think you could improve the readability of the complex condition by extracting a local variable just before this if:

BOOL isBetterTypeMatch = subType == preferredSubType && bestSubType != preferredSubType;

Then you can use that in the if condition. (Yes, this reduces short-circuiting, but this isn't an expensive calculation.)

Or maybe subType == preferredSubType can be extracted into variable subTypeIsPreferred and instead of bestSubType will be stored bestSubTypeIsPreferred then that condition would become distance == minDistance && subTypeIsPreferred && !bestSubTypeIsPreferred (which is actually similar to that original with scores just there is bool instead of int and a && !b instead of a > b).

github-actions bot added p: camera platform-ios platform-macos p: video_player labels Aug 12, 2024

misos1 force-pushed the more_fps branch 8 times, most recently from 75250e0 to 3f0e949 Compare August 14, 2024 17:43

enable more than 30 fps

6c769f9

misos1 force-pushed the more_fps branch from 3f0e949 to 6c769f9 Compare August 14, 2024 18:35

misos1 marked this pull request as ready for review August 14, 2024 18:46

misos1 requested a review from hellohuanlin as a code owner August 14, 2024 18:46

misos1 added 2 commits August 14, 2024 21:11

Merge branch 'main' into more_fps

4920305

correctly set _previewSize

4f1483f

misos1 added 2 commits August 21, 2024 17:50

Merge branch 'main' into more_fps

6c210c9

split camera changes from more_fps

30a4856

misos1 changed the title ~~[video_player_avfoundation, camera_avfoundation] enable more than 30 fps~~ [camera_avfoundation] enable more than 30 fps Aug 21, 2024

github-actions bot removed the p: video_player label Aug 21, 2024

hellohuanlin reviewed Aug 27, 2024

View reviewed changes

hellohuanlin requested a review from stuartmorgan August 27, 2024 18:15

stuartmorgan requested changes Aug 30, 2024

View reviewed changes

extract format selection into function and more

5e1f249

misos1 force-pushed the more_fps branch from 8de5b00 to dd6ca09 Compare September 5, 2024 19:19

revert reordering lockDevice with beginConfigurationForSession

5007358

misos1 force-pushed the more_fps branch from dd6ca09 to 5007358 Compare September 5, 2024 19:21

misos1 added 2 commits September 5, 2024 21:31

fix format

65f8113

fix format

27868df

misos1 requested review from stuartmorgan and hellohuanlin September 5, 2024 19:52

stuartmorgan requested changes Sep 17, 2024

View reviewed changes

misos1 added 2 commits September 19, 2024 19:33

add comments and small changes

3983273

fix format

1d8f05a

misos1 requested a review from stuartmorgan September 19, 2024 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[camera_avfoundation] enable more than 30 fps #7394

[camera_avfoundation] enable more than 30 fps #7394

misos1 commented Aug 12, 2024 •

edited

Loading

stuartmorgan commented Aug 21, 2024

hellohuanlin Aug 27, 2024

hellohuanlin Aug 27, 2024

stuartmorgan Aug 30, 2024

misos1 Sep 4, 2024 •

edited

Loading

misos1 Sep 5, 2024

stuartmorgan Aug 30, 2024

stuartmorgan Aug 30, 2024

stuartmorgan Aug 30, 2024

stuartmorgan Aug 30, 2024

misos1 Aug 30, 2024 •

edited

Loading

stuartmorgan Aug 30, 2024

stuartmorgan Aug 30, 2024

misos1 Sep 4, 2024

stuartmorgan Sep 17, 2024

stuartmorgan Sep 17, 2024

stuartmorgan Sep 17, 2024

stuartmorgan Sep 17, 2024

misos1 Sep 18, 2024


		* Adds possibility to use any supported FPS and fixes crash when using unsupported FPS.

[camera_avfoundation] enable more than 30 fps #7394

Are you sure you want to change the base?

[camera_avfoundation] enable more than 30 fps #7394

Conversation

misos1 commented Aug 12, 2024 • edited Loading

Pre-launch Checklist

stuartmorgan commented Aug 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misos1 Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misos1 Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misos1 commented Aug 12, 2024 •

edited

Loading

misos1 Sep 4, 2024 •

edited

Loading

misos1 Aug 30, 2024 •

edited

Loading