1180725 - Use AVFoundation for camera capture on OSX

Status: RESOLVED FIXED

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

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

QTkit exposes almost nothing about cameras on OSX, making constraints hard to write and get right even with guessing. QTKit is also deprecated in favor of AVFoundation, which sounds better:

Haakon Sporsheim [:haaspors] (Telenor) wrote in Bug 1131861 comment 4:
> Now, the video capture backend for OS X is currently based on QTkit which is
> deprecated in OS 10.9. Apple has a lot of good docs on how to transition
> from QTkit to AV foundation. Though AV foundation APIs are available from
> 10.7. AFAIK firefox for OS X is supported on 10.6 and later.
> 
> [... problems with QTkit not exposing any camera info e.g. frame rates]
> 
> AVFoundation on the other hand has APIs for getting AVCaptureDeviceFormat
> which also include AVFrameRateRange in plural.

We'd need to fall back to QTKit as long as we want to support 10.6. but it might not hurt to get started on this, as it would likely improve the camera constraints experience for most OSX users.

Comment 1

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: WIP-01-using-AVFoundation-for-camera-capture-on-OSX.patch

Comment 2

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: WIP-02-use-AVFoundation-for-camera-capture-on-OSX.patch

https://treeherder.mozilla.org/#/jobs?repo=try&tochange=e3ad4a1bee6d&fromchange=57ca92b1f37ffcdab33cd4f8abcbc97f3f22eb6a

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8751170 [details] [diff] [review]
WIP-02-use-AVFoundation-for-camera-capture-on-OSX.patch

Review of attachment 8751170 [details] [diff] [review]:
-----------------------------------------------------------------

Great! I'll get to the review soon.

In the meantime I notice this pulls down upstream code, so I'm cc'ing jesup to make sure this is in sync with versions and ok with licenses.

Comment 4

[Nils Ohlmeier [:drno]]

I just tested WIP-02 patch and it works as expected. Finally we correctly reject 1080p requests on the 720p FaceTime camera in my MacBook Pro. And the capturer thread appears to a be lot less busy when using AVFoundation. Would be great to get this landed.

Comment 5

[Nils Ohlmeier [:drno]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad5095dce50c

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8751170 [details] [diff] [review]
WIP-02-use-AVFoundation-for-camera-capture-on-OSX.patch

Review of attachment 8751170 [details] [diff] [review]:
-----------------------------------------------------------------

Seems reasonable.  Can you note in the bug where (which version) of code it's cherry-picked from?

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8751170 [details] [diff] [review]
WIP-02-use-AVFoundation-for-camera-capture-on-OSX.patch

Review of attachment 8751170 [details] [diff] [review]:
-----------------------------------------------------------------

All new files look like upstream code, so I skimmed those.

The rest of the changes looks good. If there's specific code you would like me to look closer at, let me know. Otherwise this lgtm.

I also pulled down and tested it, and it works to my satisfaction. This is great!

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I should note that my internal MBP cam is busted, so I did my testing using an external Logitech C930e camera. It was nice to finally see modes specific to the camera on Mac.

I see drno tested the internal MBP cam in comment 4, which is good.

Comment 9

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

The new files are not from upstream.
I made this patch by
1) copy the folder mac/qtkit
2) remove QTkit related code and implement these APIs with AVFoundation.
3) replace all naming *QTKit* to *AVFoundation*

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

In that case I'll look over the new files a second time.

Do we want to add some comments to that effect? Does the licensing require anything when we do this?

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8751170 [details] [diff] [review]
WIP-02-use-AVFoundation-for-camera-capture-on-OSX.patch

Review of attachment 8751170 [details] [diff] [review]:
-----------------------------------------------------------------

Note: if this is code we wrote (and didn't copy at all!) then the dates are wrong.  (they're wrong if we copied them as well, if we modified - at least should have 2016 added).

If we wrote it from scratch, Mozilla Foundation is listed in webrtc's AUTHORS file, so other than the date it should be good, and this makes it much easier to upstream

Comment 12

[Nils Ohlmeier [:drno]]

As we just have abandoned Mac OS X 10.6 - 10.8 for Firefox 49 on forward it might make sense to have a follow up ticket which removes the qtkit code once this ticket has landed :-)

Comment 13

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: WIP-03-use-AVFoundation-for-camera-capture-on-OSX.patch

correct the dates

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8752403 [details] [diff] [review]
WIP-03-use-AVFoundation-for-camera-capture-on-OSX.patch

Review of attachment 8752403 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, except that formats appear not to be exposed like they were for QTKit, and it seems to me they need to be, to ensure preferred format order (I420, ...).

Marking r- until that's resolved or explained.

::: media/webrtc/trunk/webrtc/modules/video_capture/mac/avfoundation/video_capture_avfoundation_info.mm
@@ +84,5 @@
> +                               CapabilityId:deviceCapabilityNumber
> +                           Capability_width:&capability.width
> +                          Capability_height:&capability.height
> +                          Capability_maxFPS:&capability.maxFPS]intValue];
> +    capability.rawType = kVideoBGRA;

I see we're hardcoding kVideoBGRA here.

When I use NSPR_LOG_MODULES=MediaManager:5 to compare my Logitech C910 on Win10 vs OSX (w/this patch), I see these differences:

  Windows                           OSX (w/this patch)
  -----------------------------------------------------
  160 x  120 x 30 maxFps, I420      160 x  120 x 30 maxFps, BGRA
  160 x  120 x 30 maxFps, MJPEG     160 x  120 x 30 maxFps, BGRA
  160 x  120 x 30 maxFps, RGB24

  (this pattern repeats for all the higher modes as well)

I see two identical entries in OSX, which isn't useful (hidden format difference)? There's also one less format than on Windows. Any inherent reason for that?

Can we not get the real formats? I worry streaming might be inefficient otherwise, because this comment [1] says our constraints algorithm prefers i420 for performance reasons, and this might mess that up, leaving us with an inefficient format when a better one exists.

Except for that, all the resolutions and frame rates are the same for my camera, except:

- The low-res 16:9 mode 640 x 360 x 60 (seen on Win) is missing. 
- Returns some additional 20hz and 24hz modes, like 752 x 416 x 24, yay!
- High-res 5hz modes (and one 10hz) modes missing, e.g. 1600 x 1200 x  5.

I gather these are driver differences, so just FYI.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineCameraVideoSource.cpp?rev=7e8bffba9688#297

@@ +97,5 @@
> +    // Not implemented. Mac doesn't use discrete steps in capabilities, rather
> +    // "analog". AVFoundation will do it's best to convert frames to what ever format
> +    // you ask for.
> +    WEBRTC_TRACE(webrtc::kTraceInfo, webrtc::kTraceVideoCapture, _id,
> +                 "NumberOfCapabilities is not supported on the Mac platform.");

GetBestMatchedCapability appears unused, except on Android, so we should be fine, but we should change this error message perhaps.

@@ +112,5 @@
> +    return [[_captureInfo
> +             displayCaptureSettingsDialogBoxWithDevice:deviceUniqueIdUTF8
> +             AndTitle:dialogTitleUTF8
> +             AndParentWindow:parentWindow AtX:positionX AndY:positionY]
> +             intValue];

I suspect this is dead code, but if QTKit does it...

@@ +122,5 @@
> +    // Not implemented. Mac doesn't use discrete steps in capabilities, rather
> +    // "analog". AVFoundation will do it's best to convert frames to what ever format
> +    // you ask for.
> +    WEBRTC_TRACE(webrtc::kTraceInfo, webrtc::kTraceVideoCapture, _id,
> +                 "NumberOfCapabilities is not supported on the Mac platform.");

Same here. Maybe fix error message to say something else at least.

::: media/webrtc/trunk/webrtc/modules/video_capture/mac/avfoundation/video_capture_avfoundation_info_objc.mm
@@ +55,5 @@
> +- (NSNumber*)displayCaptureSettingsDialogBoxWithDevice:(const char*)deviceUniqueIdUTF8
> +                    AndTitle:(const char*)dialogTitleUTF8
> +                    AndParentWindow:(void*) parentWindow
> +                    AtX:(uint32_t)positionX
> +                    AndY:(uint32_t) positionY

Unused?

@@ +140,5 @@
> +    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?

@@ +149,5 @@
> +    *height = videoDimensions.height;
> +    *maxFPS = maxFrameRateRange.maxFrameRate;
> +
> +    char mediatype[1024] = "";
> +    [format.mediaType getCString:mediatype maxLength: 1024 encoding:NSUTF8StringEncoding];

Is this the format type? Can we get it out?

::: media/webrtc/trunk/webrtc/modules/video_capture/mac/avfoundation/video_capture_avfoundation_objc.mm
@@ +127,5 @@
> +  AVFrameRateRange *bestFrameRateRange = nil;
> +
> +  for (AVCaptureDeviceFormat* format in [_captureDevice formats]) {
> +    CMVideoDimensions videoDimensions = CMVideoFormatDescriptionGetDimensions(format.formatDescription);
> +    if (videoDimensions.width == _frameWidth && videoDimensions.height == _frameHeight) {

Here the format used seems determined based on what fits the passed-in width, height and frame rate. But my camera - and I suspect most cameras - offers the same resolutions in all the formats, so wont this just pick whatever format is last in the internal list? That seems undesirable.

In contrast, with QTKit we got one capability per format, making the format choice explicit in the constraints algorithm. Could we do that here too?

Comment 15

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Thanks for the comment.
I will modify the patch accordingly.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #14)
> Can we not get the real formats? I worry streaming might be inefficient
> otherwise, because this comment [1] says our constraints algorithm prefers
> i420 for performance reasons, and this might mess that up, leaving us with
> an inefficient format when a better one exists.

I will expose the supported format instead of hardcoding it to kVideoBGRA.

> GetBestMatchedCapability appears unused, except on Android, so we should be
> fine, but we should change this error message perhaps.

I will modify the log.

> > +    WEBRTC_TRACE(webrtc::kTraceInfo, webrtc::kTraceVideoCapture, _id,
> > +                 "NumberOfCapabilities is not supported on the Mac platform.");
> 
> Same here. Maybe fix error message to say something else at least.

I will modify the log.

> > +- (NSNumber*)displayCaptureSettingsDialogBoxWithDevice:(const char*)deviceUniqueIdUTF8
> > +                    AndTitle:(const char*)dialogTitleUTF8
> > +                    AndParentWindow:(void*) parentWindow
> > +                    AtX:(uint32_t)positionX
> > +                    AndY:(uint32_t) positionY
> 
> Unused?

DisplayCaptureSettingsDialogBox is declared in class DeviceInfo(video_capture.h), we need this implementation here to avoid build error. I will modify the function body to return -1.

> > +    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?

OK, I will file a follow-up bug for the discrete choices.

> 
> @@ +149,5 @@
> > +    *height = videoDimensions.height;
> > +    *maxFPS = maxFrameRateRange.maxFrameRate;
> > +
> > +    char mediatype[1024] = "";
> > +    [format.mediaType getCString:mediatype maxLength: 1024 encoding:NSUTF8StringEncoding];
> 
> Is this the format type? Can we get it out?

format.mediaType denotes one of the following:

NSString *const AVMediaTypeVideo;
NSString *const AVMediaTypeAudio;
NSString *const AVMediaTypeText;
NSString *const AVMediaTypeClosedCaption;
NSString *const AVMediaTypeSubtitle;
NSString *const AVMediaTypeTimecode;
NSString *const AVMediaTypeTimedMetadata;
NSString *const AVMediaTypeMetadata;
NSString *const AVMediaTypeMuxed;

In our case, it should be always equal to AVMediaTypeVideo. I will remove this useless snipet.

We can get format type from format.formatDescription.CMPixelFormatType

> Here the format used seems determined based on what fits the passed-in
> width, height and frame rate. But my camera - and I suspect most cameras -
> offers the same resolutions in all the formats, so wont this just pick
> whatever format is last in the internal list? That seems undesirable.
> 
> In contrast, with QTKit we got one capability per format, making the format
> choice explicit in the constraints algorithm. Could we do that here too?

OK, I will modify the algorithm.

Comment 16

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

> > Can we not get the real formats? I worry streaming might be inefficient
> > otherwise, because this comment [1] says our constraints algorithm prefers
> > i420 for performance reasons, and this might mess that up, leaving us with
> > an inefficient format when a better one exists.
> 
> I will expose the supported format instead of hardcoding it to kVideoBGRA.
> 

> > Here the format used seems determined based on what fits the passed-in
> > width, height and frame rate. But my camera - and I suspect most cameras -
> > offers the same resolutions in all the formats, so wont this just pick
> > whatever format is last in the internal list? That seems undesirable.
> > 
> > In contrast, with QTKit we got one capability per format, making the format
> > choice explicit in the constraints algorithm. Could we do that here too?
> 
> OK, I will modify the algorithm.

About these two items, I have more finding today.
AVFoundation framework allows you to indicate the pixel format for its output.
In WIP-3, the output format is set to BGRA as follows.

  NSDictionary *newSettings = [NSDictionary dictionaryWithObjectsAndKeys:
                              [NSNumber numberWithUnsignedInt:kCVPixelFormatType_32ARGB], (id)kCVPixelBufferPixelFormatTypeKey,
                              nil];

No matter what pixel format the built-in or plugged-in camera output, we can get BGRA from AVFoundation.
Since getusermedia constraint doesn't contain pixel format, what about always using BGRA?

Comment 17

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Please ignore my previous comment.

Comment 18

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: WIP-04-use-AVFoundation-for-camera-capture-on-OSX.patch

1. expose the supported pixel format instead of hardcoding
2. remove unused function GetBestMatchedCapability
3. modify error log
4. determine the format based on width & height & fps & pixel format

Comment 19

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: diff-WIP3-WIP4.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=717dae8ec153

Comment 20

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8754440 [details] [diff] [review]
WIP-04-use-AVFoundation-for-camera-capture-on-OSX.patch

Review of attachment 8754440 [details] [diff] [review]:
-----------------------------------------------------------------

Lgtm with switch fixed.

::: media/webrtc/trunk/webrtc/modules/video_capture/mac/avfoundation/video_capture_avfoundation_utility.h
@@ +42,5 @@
> +    switch (fourcc) {
> +        case kCMPixelFormat_32ARGB:
> +            return webrtc::kVideoBGRA;
> +        case kCMPixelFormat_32BGRA:
> +            return webrtc::kVideoARGB;

These two look mixed up.

Comment 21

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #20)
> Comment on attachment 8754440 [details] [diff] [review]
> WIP-04-use-AVFoundation-for-camera-capture-on-OSX.patch
> 
> Review of attachment 8754440 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Lgtm with switch fixed.
> 
> :::
> media/webrtc/trunk/webrtc/modules/video_capture/mac/avfoundation/
> video_capture_avfoundation_utility.h
> @@ +42,5 @@
> > +    switch (fourcc) {
> > +        case kCMPixelFormat_32ARGB:
> > +            return webrtc::kVideoBGRA;
> > +        case kCMPixelFormat_32BGRA:
> > +            return webrtc::kVideoARGB;
> 
> These two look mixed up.

It should be right :)
One of them is named with big endian while the other is named with little endian.
When I use hardcode in WIP-03, I config AVFoundation pixel format to kCVPixelFormatType_32ARGB and report the format as kVideoBGRA in capability.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fa7e2a17ee9

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9fa7e2a17ee9

Comment 26

[pwattsmail]

FYI, the Manycam Virtual Webcam stopped working on OSX in Firefox 49, and I assume it is related to this switch. It hasn't been working on Chrome for a while, possibly because they were already on AVFoundation. 

We are a livestreaming site, and a decent number of our users use Manycam. Previously, we would tell them to use Firefox, but that is no longer an option (we are now telling them Firefox 48 as an emergency solution).

Is there anything that can be done to support virtual webcams like ManyCam, or do they need to change something on their end?

Comment 27

[Nils Ohlmeier [:drno]]

(In reply to pwattsmail from comment #26)
> FYI, the Manycam Virtual Webcam stopped working on OSX in Firefox 49, and I
> assume it is related to this switch. It hasn't been working on Chrome for a
> while, possibly because they were already on AVFoundation. 

Yes Chrome has been using AVFoundation for long time already.
 
> We are a livestreaming site, and a decent number of our users use Manycam.
> Previously, we would tell them to use Firefox, but that is no longer an
> option (we are now telling them Firefox 48 as an emergency solution).

If you need the support desperately you can also keep recommending Firefox ESR (which is right now Firefox 45), which should give you more time until another solution is available. https://www.mozilla.org/en-US/firefox/organizations/all/

> Is there anything that can be done to support virtual webcams like ManyCam,
> or do they need to change something on their end?

The problem is Firefox was using QTKit, which has been deprecated by Apple in OSX 10.9. So it can break any time with any new release of OSX. So we had to upgrade to a framework supported by Apple.
I can only guess that ManyCam might also still use older AV frameworks from Apple. But if I read ManyCams announcement of the new Mac 4.2 release it sounds like it become compatible with the next version of Chrome again. Not sure if that means that it is also going to compatible with Firefox again.

Comment 28

[Dmitriy Kravchenko]

(In reply to Nils Ohlmeier [:drno] from comment #27)
> I can only guess that ManyCam might also still use older AV frameworks from
> Apple.
ManyCam CoreMedia I/O plugin doesn't use neither AVFoundation, nor any older frameworks, like QTKit (there's no need for this really). Some more details are here: https://bugzilla.mozilla.org/show_bug.cgi?id=1308412