Closed Bug 1273734 Opened 8 years ago Closed 8 years ago

[AVFoundation] Expose different FPS range as discrete capability

Categories

(Core :: WebRTC: Audio/Video, defect, P2)

Product:
Core
Component:
WebRTC: Audio/Video
Platform:
Unspecified
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: mchiang, Assigned: mchiang)

References

Details

Attachments

(1 file)

Bug 1273734 - [AVFoundation] Expose different FPS range as discrete capability;
58 bytes, text/x-review-board-request
jib
: review+
Details 
      No description provided.
Could you provide more detail?  Also, how important is this?  Does this block any other bug on AVFoundation?
Rank: 22
Priority: -- → P2
This is a follow-up bug of bug 1180725.
It allows us to access different FPS as a discrete choices.

Jan-Ivar Bruaroey [:jib] wrote in Bug 1180725 comment 14:
> > +    AVFrameRateRange* maxFrameRateRange = nil;
> > +
> > +    for ( AVFrameRateRange* range in format.videoSupportedFrameRateRanges ) {
> > +        if ( range.maxFrameRate > maxFrameRateRange.maxFrameRate ) {
> > +            maxFrameRateRange = range;
> 
> Interesting, does this mean there might be lower frame rates available here?
> If so, our existing code gives us no way to access them as we basically
> treat maxFPS as FPS.
> 
> Might there be a way to expose them as discrete choices? If so maybe we
> should file a follow-up?
Depends on: 1180725
On importance: we've had developers complain that Firefox does not support frame rate constraints, even on non-OSX. I can see how they draw that conclusion, when, for example, with my Logitech C910, 26 of 29 modes were 30Hz with AVFoundation. If the answer is (near) always 30, then it may look like it doesn't do much. 

Turns out Windows doesn't fare much better *for the same resolutions*, except it additionally lists a number of high-res-low-(max)-rate modes that are absent in AVFoundation for some reason: 

Capability: 1600 x 1200 x  5 maxFps, I420
Capability: 1600 x 1200 x  5 maxFps, RGB24
Capability: 1600 x 1200 x 15 maxFps, MJPEG
Capability: 1712 x  960 x 15 maxFps, I420
Capability: 1712 x  960 x 15 maxFps, MJPEG
Capability: 1712 x  960 x 15 maxFps, RGB24
Capability: 1792 x 1008 x 15 maxFps, I420
Capability: 1792 x 1008 x 15 maxFps, MJPEG
Capability: 1792 x 1008 x 15 maxFps, RGB24
Capability: 1920 x 1080 x  5 maxFps, I420
Capability: 1920 x 1080 x  5 maxFps, RGB24
Capability: 2048 x 1536 x  5 maxFps, I420
Capability: 2048 x 1536 x  5 maxFps, RGB24
Capability: 2048 x 1536 x 15 maxFps, MJPEG
Capability: 2592 x 1944 x  5 maxFps, I420
Capability: 2592 x 1944 x  5 maxFps, RGB24
Capability: 2592 x 1944 x 10 maxFps, MJPEG

In any case, not a great selection for users who want low frame rates for e.g. transmission reasons.

This made me wonder if I've been confusing maxFps with fps, essentially: Have we been offering only the MAX fps, when cameras could be outputting lower frame rates?

The code review of bug 1180725 made me thing this was a possibility with AVFoundation at least. Maybe other platforms too.
Those low-max-rate modes are likely because uncompressed modes (I420, etc) are bandwidth hogs on the USB interface (and CPU/controller used by the camera).

While I'm not an expert on AVFoundation, generally the framerates reported by a camera would be the max. In fact, the CaptureCapability structure has width, height, maxFPS, types, delay, etc.  No hint of minFPS.  

The implication here is that you can ask a camera for any framerate below "max", though ones that are a clean factor of "max" may be preferred, depending on how it responds (does it use an electronic shutter to provide 8fps?  Or choose the next-lowest "even" factor for decimating capture at the highest rate? (8fps requested, 5 provided)  Or does it decimate down to the requested rate from a max-rate source, providing "jumpy" video?  (8fps provided by dropping 22 of 30 frames).

Generally, though, you let the camera determine frame rate, which may change dynamically according to lighting, or you lock it to a fixed rate (preferably a factor, per above, unless it can natively provide odd framerates).  Usually the first.
Note BTW, all those rates are factors of 30, implying a native 30fps capture rate in the sensor.
Not sure how AVFoundation is implemented, but according to my experience in android, usually the camera driver can configure the camera sensor output framerate to any value smaller than the maximum via setting its exposure time. For example, 33ms interval lead to 30 fps, 40ms interval lead to 25fps.

Some platform camera framework allows us to configure a fixed fps while others, i.e., AVFoundation, provide us a fps range (minFPS, maxFPS). We indicate a range and the output fps will fluctuate in this range depending on the environment brightness.

Take my mac book built-in camera as example, for all resolution & pixel format, AVFoundation only shows one range (1, 30). Because CaptureCapability structure only contains maxFPS, so I put 30 here. That doesn't mean AVFoundation accept any fps request under 30. If someone call getusermedia with the fps constraint < 30, it will be rejected.

This bug is for some other plugged-in cameras, which may make AVFoundation show more ranges for the same resolution & pixel format. For example, we may get these from AVFoundation:

1600 x 1200 x 1 minFps x  5 maxFps, I420
1600 x 1200 x 1 minFps x 15 maxFps, I420
1600 x 1200 x 1 minFps x  5 maxFps, RGB24
1600 x 1200 x 1 minFps x 15 maxFps, RGB24
 640 x  480 x 1 minFps x  5 maxFps, I420
 640 x  480 x 1 minFps x 15 maxFps, I420
 640 x  480 x 1 minFps x  5 maxFps, RGB24
 640 x  480 x 1 minFps x 15 maxFps, RGB24

With the patch in 1180725, we will only put these in CaptureCapability:

1600 x 1200 x 15 maxFps, I420
1600 x 1200 x 15 maxFps, RGB24
 640 x  480 x 15 maxFps, I420
 640 x  480 x 15 maxFps, RGB24

In this bug, we would like to provide these in CaptureCapability:

1600 x 1200 x  5 maxFps, I420
1600 x 1200 x 15 maxFps, I420
1600 x 1200 x  5 maxFps, RGB24
1600 x 1200 x 15 maxFps, RGB24
 640 x  480 x  5 maxFps, I420
 640 x  480 x 15 maxFps, I420
 640 x  480 x  5 maxFps, RGB24
 640 x  480 x 15 maxFps, RGB24
Assignee: nobody → mchiang
Comment on attachment 8805498 [details]
Bug 1273734 - [AVFoundation] Expose different FPS range as discrete capability;

https://reviewboard.mozilla.org/r/89226/#review88688

Performance concern and a potential null-dereference.

::: media/webrtc/trunk/webrtc/modules/video_capture/mac/avfoundation/video_capture_avfoundation_info_objc.mm:163
(Diff revision 1)
> -    for ( AVFrameRateRange* range in format.videoSupportedFrameRateRanges ) {
> -        if ( range.maxFrameRate > maxFrameRateRange.maxFrameRate ) {
> -            maxFrameRateRange = range;
> +    for (uint32_t index = 0 , count = 0 ; index < [captureDevice formats].count ; index++) {
> +        format = (AVCaptureDeviceFormat*)[[captureDevice formats]objectAtIndex:index];
> +        if (count + format.videoSupportedFrameRateRanges.count > capabilityId) {
> +            videoDimensions = CMVideoFormatDescriptionGetDimensions(format.formatDescription);
> +            frameRateRange = [format.videoSupportedFrameRateRanges objectAtIndex:(capabilityId - count)];
> +            break;
> +        } else {
> +            count += format.videoSupportedFrameRateRanges.count;
>          }
>      }

Higher-level code [1] typically loops on GetCapabilities(), so I worry this changes the performance from O(N) to O(N*N).

Can we instead do capabilityId = formatIndex << 16 + (index & 0xffff); ?

[1] https://dxr.mozilla.org/mozilla-central/rev/1561c917ee27c3ea04bd69467e5b8c7c08102f2a/dom/media/webrtc/MediaEngineCameraVideoSource.cpp#225-245

::: media/webrtc/trunk/webrtc/modules/video_capture/mac/avfoundation/video_capture_avfoundation_info_objc.mm:176
(Diff revision 1)
>          }
>      }
>  
>      *width = videoDimensions.width;
>      *height = videoDimensions.height;
> -    *maxFPS = maxFrameRateRange.maxFrameRate;
> +    *maxFPS = frameRateRange.maxFrameRate;

If I pass in a high number as capabilityId, won't frameRateRange be nil here and crash?
Attachment #8805498 - Flags: review?(jib) → review-
Comment on attachment 8805498 [details]
Bug 1273734 - [AVFoundation] Expose different FPS range as discrete capability;

https://reviewboard.mozilla.org/r/89226/#review88688

> Higher-level code [1] typically loops on GetCapabilities(), so I worry this changes the performance from O(N) to O(N*N).
> 
> Can we instead do capabilityId = formatIndex << 16 + (index & 0xffff); ?
> 
> [1] https://dxr.mozilla.org/mozilla-central/rev/1561c917ee27c3ea04bd69467e5b8c7c08102f2a/dom/media/webrtc/MediaEngineCameraVideoSource.cpp#225-245

Yes, we can modify the snippet https://dxr.mozilla.org/mozilla-central/rev/1561c917ee27c3ea04bd69467e5b8c7c08102f2a/dom/media/webrtc/MediaEngineCameraVideoSource.cpp#227-230

Do you prefer doing this change for all platform or for Mac only (use macro XP_MACOSX to isolate the change)?

> If I pass in a high number as capabilityId, won't frameRateRange be nil here and crash?

I will add protection here.
Comment on attachment 8805498 [details]
Bug 1273734 - [AVFoundation] Expose different FPS range as discrete capability;

https://reviewboard.mozilla.org/r/89226/#review88688

> Yes, we can modify the snippet https://dxr.mozilla.org/mozilla-central/rev/1561c917ee27c3ea04bd69467e5b8c7c08102f2a/dom/media/webrtc/MediaEngineCameraVideoSource.cpp#227-230
> 
> Do you prefer doing this change for all platform or for Mac only (use macro XP_MACOSX to isolate the change)?

Wait a minute, it may not be so easy because in this snippet (https://dxr.mozilla.org/mozilla-central/rev/1561c917ee27c3ea04bd69467e5b8c7c08102f2a/dom/media/webrtc/MediaEngineCameraVideoSource.cpp#227-230) we don't have the information of formatIndex here.
Comment on attachment 8805498 [details]
Bug 1273734 - [AVFoundation] Expose different FPS range as discrete capability;

https://reviewboard.mozilla.org/r/89226/#review89674

You're right, sorry, I hadn't thought through the consequences of the suggestion I made, but you’ve made it work!

In hindsight I worry I may have overstated the problem. On re-inspection, I see the existing code looked up the format and traversed the ranges, whereas your new code (Rev 1) traversed the formats and looked up the range, so with the caller's looping considered, it was probably already O(N*N). There are probably more formats than ranges (~64 on my logitech cam), so it maybe got a tiny bit slower, but perhaps not enough to have worried about or worth maintaining a map? What do you think? I’m good to leave it in (lookup may even be a bit faster now), or go back to what you had, now that you check for overflow, up to you! Nicely done though.

::: media/webrtc/trunk/webrtc/modules/video_capture/mac/avfoundation/video_capture_avfoundation_info_objc.mm:143
(Diff revisions 1 - 3)
> +        for (int frameRateIndex = 0;
> +                frameRateIndex < (int) format.videoSupportedFrameRateRanges.count;
> +                frameRateIndex++) {
> +            [capabilityMap addObject: [NSNumber numberWithInt:((formatIndex << 16) + (frameRateIndex & 0xffff))]];
> +            count++;

count is not used in loop, so ++ can be lifted outside as:

    count += format.videoSupportedFrameRateRanges.count;

in case the compiler doesn't optimize it.
Attachment #8805498 - Flags: review?(jib) → review+
 warning: conflicts while merging media/webrtc/trunk/webrtc/modules/video_capture/mac/avfoundation/video_capture_avfoundation_info_objc.mm! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue) 

can you take a look at this, thanks!
Flags: needinfo?(mchiang)
Keywords: checkin-needed
Flags: needinfo?(mchiang)
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/be3718436994
[AVFoundation] Expose different FPS range as discrete capability; r=jib
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/be3718436994
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Blocks: 1180725
No longer depends on: 1180725
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: