1279036 - Video resolution over 640x480 not working properly in getUserMedia on OSX (REGRESSION)

Status: RESOLVED FIXED

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

Description

[Anthony Minessale]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2762.0 Safari/537.36

Steps to reproduce:

Using this jsfiddle I tried to make a 720p gUM call.
https://jsfiddle.net/aynr0k5q/40/



Actual results:

It doesn't work, nothing happens


Expected results:

The video should have opened in a 16:9 window.
This does function properly on latest release.

Comment 1

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

14:47.70 INFO: Last good revision: 8e697c3542ffbe810d6ecc9912a510216471e99b
14:47.70 INFO: First bad revision: 9fa7e2a17ee96abc884352ec5482642b5da30a57
14:47.70 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8e697c3542ffbe810d6ecc9912a510216471e99b&tochange=9fa7e2a17ee96abc884352ec5482642b5da30a57

Looks like the AVFoundation switch. Bug 1180725.

Comment 2

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

I'm seeing the camera light turn on, but no stream is returned.

Munroe, can you take a look?

Comment 3

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

Minimized STR https://jsfiddle.net/9a35kzao/

Comment 4

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

Attachment: output2.txt

Comment 5

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

(In reply to Jan-Ivar Bruaroey [:jib] from comment #4)
>  Capability:  960 x  540 x 15 maxFps, YUY2, Unknown codec. Distance = 500
>  Capability: 1024 x  576 x 15 maxFps, YUY2, Unknown codec. Distance = 400
>  Chosen capability: 1280 x  720 x 30 maxFps, MJPEG, Unknown codec. Distance = 0

Interestingly, if I add frameRate: 15 to the constraints then it works.

I get 1024 x 576 x 15, which only exists in YUY2 (all MJPEG modes are 30 fps).

This seems to implicate MJPEG.

Comment 6

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

When it doesn't work, onloadedmetadata never fires.

Comment 7

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

I cannot reproduce the issue with my MBP built-in camera or logitec c920.
jib, could you help verify the attached WIP?

Comment 8

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

Attachment: WIP_remove_MJPEG.patch

Comment 9

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

Will do. Btw, I could have sworn I tested 1280x720 in bug 1180725 so I went back and retested and it regressed between the two patches in that bug.

Comment 10

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

Didn't work. With export NSPR_LOG_MODULES=MediaManager:5 , instead of MJPEG I now see:

> Chosen capability: 1280 x  720 x 30 maxFps, Unknown type, Unknown codec. Distance = 0
                                              ^^^^^^^^^^^^^

Comment 11

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

Results in comment 10 was on OSX 10.11.1 with both Logitech 910 and 930e (I don't have 920).

Comment 12

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

Attachment: WIP-02_remove_MJPEG.patch

The problem should be related to MJPEG format.
Somehow AVFoundation output is not as we expect.
Attachment 8752403 [details] [diff] for Bug 1180725 works because it hard-code output to BGRA.
Before obtaining a webcam supporting MJPEG output, I will remove the MJPEG capability first.

Comment 13

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

Attachment: WIP-03_remove_MJPEG.patch

Comment 14

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

What's the purpose in removing MJPEG support?  Without MJPEG it's hard to get high resolutions+framerates on a USB2 camera (i.e. almost all of them).

Comment 15

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

I agree with jesup. MJPEG used to work (see bug 1151628, bug 1166664, bug 1152016 etc.) and it still works for 640x480 and lower here.

Comment 16

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

(In reply to Munro Mengjue Chiang [:mchiang] from comment #13)
> WIP-03_remove_MJPEG.patch

Doesn't work for me. Same symptom, except now 640x480 doesn't work either. And the log output is disturbing:

> [Unnamed thread 0x12cfb7550]: D/MediaManager ChooseCapability: prefs: 640x480 @30-10fps
> [Unnamed thread 0x12cfb7550]: D/MediaManager Constraints: width: { min: -2147483647, max: 2147483647, ideal: 1280 }
> [Unnamed thread 0x12cfb7550]: D/MediaManager              height: { min: -2147483647, max: 2147483647, ideal: 720 }
> [Unnamed thread 0x12cfb7550]: D/MediaManager              frameRate: { min: -inf, max: inf }
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:    0 x    0 x  0 maxFps, I420, Unknown codec. Distance = 0
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:    0 x    0 x  0 maxFps, I420, Unknown codec. Distance = 0
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:    0 x    0 x  0 maxFps, I420, Unknown codec. Distance = 0
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:    0 x    0 x  0 maxFps, I420, Unknown codec. Distance = 0
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:    0 x    0 x  0 maxFps, I420, Unknown codec. Distance = 0
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:    0 x    0 x  0 maxFps, I420, Unknown codec. Distance = 0
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:    0 x    0 x  0 maxFps, I420, Unknown codec. Distance = 0
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:    0 x    0 x  0 maxFps, I420, Unknown codec. Distance = 0
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:    0 x    0 x  0 maxFps, I420, Unknown codec. Distance = 0
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:    0 x    0 x  0 maxFps, I420, Unknown codec. Distance = 0
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:    0 x    0 x  0 maxFps, I420, Unknown codec. Distance = 0
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:    0 x    0 x  0 maxFps, I420, Unknown codec. Distance = 0
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:    0 x    0 x  0 maxFps, I420, Unknown codec. Distance = 0
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:    0 x    0 x  0 maxFps, I420, Unknown codec. Distance = 0
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:    0 x    0 x  0 maxFps, I420, Unknown codec. Distance = 0
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:    0 x    0 x  0 maxFps, I420, Unknown codec. Distance = 0
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:  160 x  120 x 30 maxFps, YUY2, Unknown codec. Distance = 1708
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:  176 x  144 x 30 maxFps, YUY2, Unknown codec. Distance = 1662
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:  320 x  180 x 30 maxFps, YUY2, Unknown codec. Distance = 1500
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:  320 x  240 x 30 maxFps, YUY2, Unknown codec. Distance = 1416
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:  352 x  288 x 30 maxFps, YUY2, Unknown codec. Distance = 1325
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:  424 x  240 x 30 maxFps, YUY2, Unknown codec. Distance = 1334
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:  480 x  270 x 30 maxFps, YUY2, Unknown codec. Distance = 1250
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:  640 x  480 x 30 maxFps, YUY2, Unknown codec. Distance = 833
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:  800 x  448 x 30 maxFps, YUY2, Unknown codec. Distance = 752
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:  848 x  480 x 30 maxFps, YUY2, Unknown codec. Distance = 670
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:  800 x  600 x 24 maxFps, YUY2, Unknown codec. Distance = 541
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability:  960 x  540 x 15 maxFps, YUY2, Unknown codec. Distance = 500
> [Unnamed thread 0x12cfb7550]: D/MediaManager Capability: 1024 x  576 x 15 maxFps, YUY2, Unknown codec. Distance = 400
> [Unnamed thread 0x12cfb7550]: D/MediaManager Chosen capability:    0 x    0 x  0 maxFps, I420, Unknown codec. Distance = 0
> [Unnamed thread 0x12cfb7550]: D/MediaManager Video device 4097 allocated for https://fiddle.jshell.net

Comment 17

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

Attachment: WIP-01_retrieve-CMSampleBuffer-in-mjpeg-case.patch

Sorry, it's indeed a bad idea to remove it.
I have figured out the root cause.
The way to retrieve MJPEG image data and raw image data is different.
The attachment fixed the problem.
I have tested https://jsfiddle.net/9a35kzao/ and https://jsfiddle.net/aynr0k5q/40/.
Both work fine with the patch.

Comment 18

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

Attachment: WIP-01_retrieve-CMSampleBuffer-via-CMSampleBufferGetDataBuffer-for-mjpeg-case.patch

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

Comment 19

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

Comment on attachment 8762513 [details] [diff] [review]
WIP-01_retrieve-CMSampleBuffer-via-CMSampleBufferGetDataBuffer-for-mjpeg-case.patch

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

Awesome, works! r=me with nits.

Let's get this uplifted.

::: media/webrtc/trunk/webrtc/modules/video_capture/mac/avfoundation/video_capture_avfoundation_objc.mm
@@ +243,5 @@
>  
> +    if (blockBuffer) {
> +      char* baseAddress = NULL;
> +      size_t frameSize = 0;
> +      size_t lengthAtOffset = 0;

Nit: Technically redundant initialization, though I know opinions vary, so either way.

@@ +249,2 @@
>  
> +      if (lengthAtOffset == frameSize) {

Nit: Runtime if-test is redundant when offset is always 0.

@@ +251,5 @@
> +        VideoCaptureCapability tempCaptureCapability;
> +        tempCaptureCapability.width = _frameWidth;
> +        tempCaptureCapability.height = _frameHeight;
> +        tempCaptureCapability.maxFPS = _frameRate;
> +        tempCaptureCapability.rawType = _rawType;

This is duplicate code that could be lifted up a level to share.

Comment 20

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

Attachment: WIP-02_retrieve-CMSampleBuffer-via-CMSampleBufferGetDataBuffer-for-mjpeg-case.patch

carry r+ from jib
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac2c8f5bb9e2

Comment 21

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

Comment on attachment 8762611 [details] [diff] [review]
WIP-02_retrieve-CMSampleBuffer-via-CMSampleBufferGetDataBuffer-for-mjpeg-case.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1279036
[User impact if declined]: Failed to run getUserMedia with USB camera on OSX
[Describe test coverage new/current, TreeHerder]: Covered by existing unit tests.
[Risks and why]: Low risk. This change is limited to a wrapper for OSX AVFoundation framework
[String/UUID change made/needed]: None.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b779a53247e2
retrieve CMSampleBuffer via CMSampleBufferGetDataBuffer for mjpeg case. r=jib

Comment 23

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

Comment on attachment 8762611 [details] [diff] [review]
WIP-02_retrieve-CMSampleBuffer-via-CMSampleBufferGetDataBuffer-for-mjpeg-case.patch

Note, this is for 49.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b779a53247e2

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8762611 [details] [diff] [review]
WIP-02_retrieve-CMSampleBuffer-via-CMSampleBufferGetDataBuffer-for-mjpeg-case.patch

Fix for video regression from 49, ok to uplift to aurora.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dcc108611744