Closed Bug 1180725 Opened 4 years ago Closed 4 years ago

Use AVFoundation for camera capture on OSX

Categories

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

Unspecified
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox42 --- affected
firefox49 --- fixed
Blocking Flags:

People

(Reporter: jib, Assigned: mchiang)

References

(Depends on 2 open bugs)

Details

Attachments

(2 files, 3 obsolete files)

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.
backlog: --- → webRTC+
Rank: 27
Priority: -- → P2
Assignee: nobody → mchiang
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.
Attachment #8751170 - Flags: feedback?(rjesup)
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 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?
Attachment #8751170 - Flags: feedback?(rjesup) → feedback+
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!
Attachment #8751170 - Flags: review?(jib) → review+
Summary: Investigate using AVFoundation for camera capture on OSX → Use AVFoundation for camera capture on OSX
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.
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*
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?
Flags: needinfo?(rjesup)
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
Flags: needinfo?(rjesup)
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 :-)
correct the dates
Attachment #8751170 - Attachment is obsolete: true
Attachment #8752403 - Flags: review?(jib)
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?
Attachment #8752403 - Flags: review?(jib) → review-
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.
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
Attachment #8752403 - Attachment is obsolete: true
Attachment #8754440 - Flags: review?(jib)
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.
Attachment #8754440 - Flags: review?(jib) → review+
(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.
Blocks: 1275217
https://hg.mozilla.org/mozilla-central/rev/9fa7e2a17ee9
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Duplicate of this bug: 1246348
Depends on: 1279036
Duplicate of this bug: 1280449
See Also: → 1302059
Depends on: 1297029
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?
(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.
(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
No longer blocks: 1273734
Depends on: 1273734
No longer blocks: 1193640
Depends on: 1193640
No longer blocks: 1275217
Depends on: 1275217
Depends on: 1323775
You need to log in before you can comment on or make changes to this bug.