Closed Bug 1279036 Opened 8 years ago Closed 8 years ago

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

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox48 --- unaffected
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: anthony.minessale, Assigned: mchiang)

References

Details

(Keywords: regression)

Attachments

(2 files, 5 obsolete files)

output2.txt
12.82 KB, text/plain
Details
WIP-02_retrieve-CMSampleBuffer-via-CMSampleBufferGetDataBuffer-for-mjpeg-case.patch
3.37 KB, patch
mchiang
: review+
lizzard
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
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.
Status: UNCONFIRMED → NEW
Rank: 10
Ever confirmed: true
OS: Unspecified → Mac OS X
Priority: -- → P1
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.
Blocks: 1180725
Summary: Video resolution over 640x480 not working properly in getUserMedia → Video resolution over 640x480 not working properly in getUserMedia on OSX (REGRESSION)
I'm seeing the camera light turn on, but no stream is returned.

Munroe, can you take a look?
Assignee: nobody → mchiang
(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.
When it doesn't work, onloadedmetadata never fires.
Flags: needinfo?(mchiang)
I cannot reproduce the issue with my MBP built-in camera or logitec c920.
jib, could you help verify the attached WIP?
Flags: needinfo?(mchiang)
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.
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
                                              ^^^^^^^^^^^^^
Flags: needinfo?(mchiang)
Results in comment 10 was on OSX 10.11.1 with both Logitech 910 and 930e (I don't have 920).
Attached patch WIP-02_remove_MJPEG.patch (obsolete) — — Splinter Review 
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.
Flags: needinfo?(mchiang)
Attached patch WIP-03_remove_MJPEG.patch (obsolete) — — Splinter Review 

        Attachment #8761783 -
        Attachment is obsolete: true

        Attachment #8761906 -
        Attachment is obsolete: true

    
    

    
  
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).

    
    

    
  
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.

    
    

    
  
(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

    
    

    
  


  
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.

        Attachment #8761908 -
        Attachment is obsolete: true

    
    

    
  
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.

        Attachment #8762513 -
        Flags: review?(jib) → review+

    
    

    
  
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.

        Attachment #8762611 -
        Flags: approval-mozilla-release?

    
    

    
  
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b779a53247e2
retrieve CMSampleBuffer via CMSampleBufferGetDataBuffer for mjpeg case. r=jib
Keywords: checkin-needed

    
    

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

Note, this is for 49.

        Attachment #8762611 -
        Flags: approval-mozilla-release? → approval-mozilla-aurora?

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/b779a53247e2
Status: NEW → RESOLVED
Closed: 8 years ago

          status-firefox50:
          affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50

    
    

    
  
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.

        Attachment #8762611 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: