Closed Bug 1382433 Opened 7 years ago Closed 7 years ago

Slow video in local preview window in Nightly on appear.in

Categories

(Core :: WebRTC, defect, P2)

56 Branch
Unspecified
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: mjf, Assigned: mchiang)

References

Details

(Keywords: stale-bug, Whiteboard: regression)

Attachments

(2 files, 1 obsolete file)

In appear.in calls (both 1:1 and 1:n) today I noticed my local preview window was super slow and smeared when I moved. Others on the 1:n call noticed my video to them wasn't great either. Using mozregression, I tracked it down to this: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7e2c3de976857db485370c5fdecf70990216847b&tochange=c846596b4cb8cd51375621dd43fc75e0a887c61a
User agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:56.0) Gecko/20100101 Firefox/56.0
Depends on: 1374938
Whiteboard: regression
Interestingly, this morning after my laptop sat idle overnight (I did not log out, I did not restart, machine did not sleep) I can no longer reproduce this issue. Yesterday it was reliably reproducible. I tried multiple times (on AC power, on battery) and had no luck making it happen again today. *sigh*
Saw this again today. It was with a fresh restart of Nightly. Version 57.0a1 Build ID 20170809100326 User Agent Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:57.0) Gecko/20100101 Firefox/57.0
Saw this again today. It was with a fresh restart of Nightly. Version 57.0a1 Build ID 20170809100326 User Agent Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:57.0) Gecko/20100101 Firefox/57.0
This is a P1 bug without an assignee. P1 are bugs which are being worked on for the current release cycle/iteration/sprint. If the bug is not assigned by Monday, 28 August, the bug's priority will be reset to '--'.
Keywords: stale-bug
Still valid?
Rank: 17
Flags: needinfo?(mfroman)
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
I haven't seen this lately, so I guess we can call it gone.
Flags: needinfo?(mfroman)
We found a way to reproduce the bug. steps: 1. Use MBP 2016 early model (without usb-c connector). 2. go to appear.in and join a room. 3. cover the camera completely. You will see a dark screen with some noise. (Actually you can found the fps is around ~1 fps by observing the noise) 4. uncover the camera. Now you will observe the bug.
Assignee: nobody → mchiang
Currently we query supported AVFrameRateRange and choose the best one. [1] Then we configure camera min & max fps according to the chosen AVFrameRateRange. [2] MBP 2016 early model built-in camera only support one fps range (1-30) for each resolution. So we set the min fps to 1. When the camera is covered and the brightness is very low, the fps will reduce to almost 1 and cause this slow video issue when the camera is uncovered. The fps will increase gradually but it takes several (10+) seconds. After reading the avfoundation document and doing some experiments, AVFrameRateRange expresses a range of valid frame rates as minimum and maximum rate. We can narrow down the range if necessary. So I would like to change the code to use fixed fps instead of floating fps to avoid very low fps. [1] http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/media/webrtc/trunk/webrtc/modules/video_capture/mac/avfoundation/video_capture_avfoundation_objc.mm#129-133 [2] http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/media/webrtc/trunk/webrtc/modules/video_capture/mac/avfoundation/video_capture_avfoundation_objc.mm#150-156
I have confirmed chrome also use fixed frame rate by running this page https://jsfiddle.net/eo1ewsL7/ with MBP 2016 early model.
Comment on attachment 8912636 [details] Bug 1382433 - use fixed fps instead of floating fps to avoid very low fps. https://reviewboard.mozilla.org/r/183966/#review189520 ::: media/webrtc/trunk/webrtc/modules/video_capture/mac/avfoundation/video_capture_avfoundation_objc.mm:150 (Diff revision 1) > if ([captureConnection isVideoMinFrameDurationSupported]) { > [captureConnection setVideoMinFrameDuration:bestFrameRateRange.minFrameDuration]; > } > > if ([captureConnection isVideoMaxFrameDurationSupported]) { > - [captureConnection setVideoMaxFrameDuration:bestFrameRateRange.maxFrameDuration]; > + [captureConnection setVideoMaxFrameDuration:bestFrameRateRange.minFrameDuration]; I'm not sure I understand the fix. Code comments may help. But even after reading comment 12 I'm not sure I understand the scope of the problem. This fix doesn't appear to be solely for MBP 2016 early model. How will this affect other models? How will this affect users using the frameRate constraint?
Also, are we sure the symptoms in comment 0 and comment 10 are the same? Michael, can you confirm if you're seeing comment 10 ? Or are you by chance seeing bug 1397844 (attachment 8905608 [details])?
Flags: needinfo?(mfroman)
(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #15) > I'm not sure I understand the fix. Code comments may help. I will add more comments. The basic idea is to set the same value to min & max fps to achieve fixed frame rate. > > But even after reading comment 12 I'm not sure I understand the scope of the > problem. I recorded the symptom. Please refer to https://youtu.be/6n3fWZUlCMs The problem is if we configure camera as variant fps between 1-30, when the brightness is very low, camera aec will low down the fps to ~1 in order to increase the exposure time. Meanwhile, aec rely on the new images' brightness as the reference to adjust the fps. ~1 fps means aec will only get a new image as the reference in one second. Obviously, mbp 2016 early model built-in camera aec is not very aggresive, it won't adjust frame rate & ex[osure setting until receiving several over-exposured samples. > > This fix doesn't appear to be solely for MBP 2016 early model. How will this > affect other models? I checked MBP 2016 late model and Logitec c920 webcam, their supported ranges are reported as {30, 30} {15, 15} {10, 10} So for this kind of camera devices, we already set fixed fps to hardware. > > How will this affect users using the frameRate constraint? User will get more compatible behavior. When we expose the hardware capabilities, we only expose maxFPS [1]. FitnessDistance calculate the distance according to the maxFPS [2]. If user set a minimum fps to 30, we will choose a capability with maxFPS=30 but actually we use variant fps {1-30} (for MBP 2016 early model only), which would violate the constraint. [1] http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/media/webrtc/trunk/webrtc/modules/video_capture/mac/avfoundation/video_capture_avfoundation_info_objc.mm#212 [2] http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/dom/media/webrtc/MediaEngineCameraVideoSource.cpp#73
Comment on attachment 8912636 [details] Bug 1382433 - use fixed fps instead of floating fps to avoid very low fps. https://reviewboard.mozilla.org/r/183966/#review189724 r=me with comments, but see concerns in comment 19.
Attachment #8912636 - Flags: review?(jib) → review+
(In reply to Munro Mengjue Chiang [:mchiang] from comment #17) > I recorded the symptom. Please refer to https://youtu.be/6n3fWZUlCMs Thanks, that's different from bug 1397844. > I checked MBP 2016 late model and Logitec c920 webcam, their supported > ranges are reported as > {30, 30} > {15, 15} > {10, 10} > > So for this kind of camera devices, we already set fixed fps to hardware. Are you sure? How are you logging these ranges? How to they map to what constraints reveal? I'm asking because I also have an MBP 2016 late model (MacBook Pro (15-inch, 2016) the one with touchbar) and a c920 and I see the following in MOZ_LOG (repeated for every resolution), even with your patch here built locally: MediaManager Capability: 640 x 480 x 31 maxFps, MJPEG, Unknown codec. Distance = 32 MediaManager Capability: 640 x 480 x 25 maxFps, MJPEG, Unknown codec. Distance = 166 MediaManager Capability: 640 x 480 x 20 maxFps, MJPEG, Unknown codec. Distance = 333 MediaManager Capability: 640 x 480 x 16 maxFps, MJPEG, Unknown codec. Distance = 466 MediaManager Capability: 640 x 480 x 10 maxFps, MJPEG, Unknown codec. Distance = 666 MediaManager Capability: 640 x 480 x 8 maxFps, MJPEG, Unknown codec. Distance = 733 MediaManager Capability: 640 x 480 x 5 maxFps, MJPEG, Unknown codec. Distance = 833 I'm assuming MJPEG is the c920. For my internal camera, I still see every frame-rate possible frame rate (even with your patch): MediaManager Capability: 640 x 480 x 31 maxFps, UYVY, Unknown codec. Distance = 32 MediaManager Capability: 640 x 480 x 30 maxFps, UYVY, Unknown codec. Distance = 0 MediaManager Capability: 640 x 480 x 29 maxFps, UYVY, Unknown codec. Distance = 33 MediaManager Capability: 640 x 480 x 28 maxFps, UYVY, Unknown codec. Distance = 66 MediaManager Capability: 640 x 480 x 27 maxFps, UYVY, Unknown codec. Distance = 100 MediaManager Capability: 640 x 480 x 25 maxFps, UYVY, Unknown codec. Distance = 166 MediaManager Capability: 640 x 480 x 25 maxFps, UYVY, Unknown codec. Distance = 166 MediaManager Capability: 640 x 480 x 24 maxFps, UYVY, Unknown codec. Distance = 200 MediaManager Capability: 640 x 480 x 23 maxFps, UYVY, Unknown codec. Distance = 233 MediaManager Capability: 640 x 480 x 22 maxFps, UYVY, Unknown codec. Distance = 266 MediaManager Capability: 640 x 480 x 20 maxFps, UYVY, Unknown codec. Distance = 333 MediaManager Capability: 640 x 480 x 20 maxFps, UYVY, Unknown codec. Distance = 333 MediaManager Capability: 640 x 480 x 19 maxFps, UYVY, Unknown codec. Distance = 366 MediaManager Capability: 640 x 480 x 18 maxFps, UYVY, Unknown codec. Distance = 400 MediaManager Capability: 640 x 480 x 16 maxFps, UYVY, Unknown codec. Distance = 466 MediaManager Capability: 640 x 480 x 16 maxFps, UYVY, Unknown codec. Distance = 466 MediaManager Capability: 640 x 480 x 15 maxFps, UYVY, Unknown codec. Distance = 500 MediaManager Capability: 640 x 480 x 14 maxFps, UYVY, Unknown codec. Distance = 533 MediaManager Capability: 640 x 480 x 13 maxFps, UYVY, Unknown codec. Distance = 566 MediaManager Capability: 640 x 480 x 12 maxFps, UYVY, Unknown codec. Distance = 600 MediaManager Capability: 640 x 480 x 10 maxFps, UYVY, Unknown codec. Distance = 666 MediaManager Capability: 640 x 480 x 10 maxFps, UYVY, Unknown codec. Distance = 666 MediaManager Capability: 640 x 480 x 8 maxFps, UYVY, Unknown codec. Distance = 733 MediaManager Capability: 640 x 480 x 8 maxFps, UYVY, Unknown codec. Distance = 733 MediaManager Capability: 640 x 480 x 7 maxFps, UYVY, Unknown codec. Distance = 766 MediaManager Capability: 640 x 480 x 5 maxFps, UYVY, Unknown codec. Distance = 833 MediaManager Capability: 640 x 480 x 4 maxFps, UYVY, Unknown codec. Distance = 866 MediaManager Capability: 640 x 480 x 4 maxFps, UYVY, Unknown codec. Distance = 866 MediaManager Capability: 640 x 480 x 2 maxFps, UYVY, Unknown codec. Distance = 933 MediaManager Capability: 640 x 480 x 1 maxFps, UYVY, Unknown codec. Distance = 966 Does that match your expectations?
Flags: needinfo?(mchiang)
(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #16) > Also, are we sure the symptoms in comment 0 and comment 10 are the same? > > Michael, can you confirm if you're seeing comment 10 ? Or are you by chance > seeing bug 1397844 (attachment 8905608 [details])? Definitely haven't seen bug 1397844. I can repro comment 10. Mine is a 2015 MBP.
Flags: needinfo?(mfroman)
(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #19) > Are you sure? How are you logging these ranges? How to they map to what > constraints reveal? > > I'm asking because I also have an MBP 2016 late model (MacBook Pro (15-inch, > 2016) the one with touchbar) and a c920 and I see the following in MOZ_LOG > (repeated for every resolution), even with your patch here built locally: > > MediaManager Capability: 640 x 480 x 31 maxFps, MJPEG, Unknown codec. > Distance = 32 > MediaManager Capability: 640 x 480 x 25 maxFps, MJPEG, Unknown codec. > Distance = 166 > MediaManager Capability: 640 x 480 x 20 maxFps, MJPEG, Unknown codec. > Distance = 333 > MediaManager Capability: 640 x 480 x 16 maxFps, MJPEG, Unknown codec. > Distance = 466 > MediaManager Capability: 640 x 480 x 10 maxFps, MJPEG, Unknown codec. > Distance = 666 > MediaManager Capability: 640 x 480 x 8 maxFps, MJPEG, Unknown codec. > Distance = 733 > MediaManager Capability: 640 x 480 x 5 maxFps, MJPEG, Unknown codec. > Distance = 833 > > I'm assuming MJPEG is the c920. > > For my internal camera, I still see every frame-rate possible frame rate > (even with your patch): > > MediaManager Capability: 640 x 480 x 31 maxFps, UYVY, Unknown codec. > Distance = 32 > MediaManager Capability: 640 x 480 x 30 maxFps, UYVY, Unknown codec. > Distance = 0 > MediaManager Capability: 640 x 480 x 29 maxFps, UYVY, Unknown codec. > Distance = 33 > MediaManager Capability: 640 x 480 x 28 maxFps, UYVY, Unknown codec. > Distance = 66 > MediaManager Capability: 640 x 480 x 27 maxFps, UYVY, Unknown codec. > Distance = 100 > MediaManager Capability: 640 x 480 x 25 maxFps, UYVY, Unknown codec. > Distance = 166 > MediaManager Capability: 640 x 480 x 25 maxFps, UYVY, Unknown codec. > Distance = 166 > MediaManager Capability: 640 x 480 x 24 maxFps, UYVY, Unknown codec. > Distance = 200 > MediaManager Capability: 640 x 480 x 23 maxFps, UYVY, Unknown codec. > Distance = 233 > MediaManager Capability: 640 x 480 x 22 maxFps, UYVY, Unknown codec. > Distance = 266 > MediaManager Capability: 640 x 480 x 20 maxFps, UYVY, Unknown codec. > Distance = 333 > MediaManager Capability: 640 x 480 x 20 maxFps, UYVY, Unknown codec. > Distance = 333 > MediaManager Capability: 640 x 480 x 19 maxFps, UYVY, Unknown codec. > Distance = 366 > MediaManager Capability: 640 x 480 x 18 maxFps, UYVY, Unknown codec. > Distance = 400 > MediaManager Capability: 640 x 480 x 16 maxFps, UYVY, Unknown codec. > Distance = 466 > MediaManager Capability: 640 x 480 x 16 maxFps, UYVY, Unknown codec. > Distance = 466 > MediaManager Capability: 640 x 480 x 15 maxFps, UYVY, Unknown codec. > Distance = 500 > MediaManager Capability: 640 x 480 x 14 maxFps, UYVY, Unknown codec. > Distance = 533 > MediaManager Capability: 640 x 480 x 13 maxFps, UYVY, Unknown codec. > Distance = 566 > MediaManager Capability: 640 x 480 x 12 maxFps, UYVY, Unknown codec. > Distance = 600 > MediaManager Capability: 640 x 480 x 10 maxFps, UYVY, Unknown codec. > Distance = 666 > MediaManager Capability: 640 x 480 x 10 maxFps, UYVY, Unknown codec. > Distance = 666 > MediaManager Capability: 640 x 480 x 8 maxFps, UYVY, Unknown codec. > Distance = 733 > MediaManager Capability: 640 x 480 x 8 maxFps, UYVY, Unknown codec. > Distance = 733 > MediaManager Capability: 640 x 480 x 7 maxFps, UYVY, Unknown codec. > Distance = 766 > MediaManager Capability: 640 x 480 x 5 maxFps, UYVY, Unknown codec. > Distance = 833 > MediaManager Capability: 640 x 480 x 4 maxFps, UYVY, Unknown codec. > Distance = 866 > MediaManager Capability: 640 x 480 x 4 maxFps, UYVY, Unknown codec. > Distance = 866 > MediaManager Capability: 640 x 480 x 2 maxFps, UYVY, Unknown codec. > Distance = 933 > MediaManager Capability: 640 x 480 x 1 maxFps, UYVY, Unknown codec. > Distance = 966 > > Does that match your expectations? Sorry for the confusion. I didn't list all entries. Here is the complete supported frame rate for VGA. It matches the MediaManger log. MBP 2016 late model built-in camera range: min: 30.000030, max: 30.000030, ceil(max): 31 range: min: 29.000049, max: 29.000049, ceil(max): 30 range: min: 28.000067, max: 28.000067, ceil(max): 29 range: min: 27.000027, max: 27.000027, ceil(max): 28 range: min: 26.000026, max: 26.000026, ceil(max): 27 range: min: 25.000000, max: 25.000000, ceil(max): 25 range: min: 24.000038, max: 24.000038, ceil(max): 25 range: min: 23.000032, max: 23.000032, ceil(max): 24 range: min: 22.000022, max: 22.000022, ceil(max): 23 range: min: 21.000021, max: 21.000021, ceil(max): 22 range: min: 20.000000, max: 20.000000, ceil(max): 20 range: min: 19.000029, max: 19.000029, ceil(max): 20 range: min: 18.000018, max: 18.000018, ceil(max): 19 range: min: 17.000009, max: 17.000009, ceil(max): 18 range: min: 16.000000, max: 16.000000, ceil(max): 16 range: min: 15.000015, max: 15.000015, ceil(max): 16 range: min: 14.000014, max: 14.000014, ceil(max): 15 range: min: 13.000013, max: 13.000013, ceil(max): 14 range: min: 12.000005, max: 12.000005, ceil(max): 13 range: min: 11.000011, max: 11.000011, ceil(max): 12 range: min: 10.000000, max: 10.000000, ceil(max): 10 range: min: 9.000001, max: 9.000001, ceil(max): 10 range: min: 8.000000, max: 8.000000, ceil(max): 8 range: min: 7.000002, max: 7.000002, ceil(max): 8 range: min: 6.000002, max: 6.000002, ceil(max): 7 range: min: 5.000000, max: 5.000000, ceil(max): 5 range: min: 4.000000, max: 4.000000, ceil(max): 4 range: min: 3.000000, max: 3.000000, ceil(max): 4 range: min: 2.000000, max: 2.000000, ceil(max): 2 range: min: 1.000000, max: 1.000000, ceil(max): 1 Logitec C920 range: min: 30.000030, max: 30.000030, ceil(max): 31 range: min: 24.000038, max: 24.000038, ceil(max): 25 range: min: 20.000000, max: 20.000000, ceil(max): 20 range: min: 15.000015, max: 15.000015, ceil(max): 16 range: min: 10.000000, max: 10.000000, ceil(max): 10 range: min: 7.500002, max: 7.500002, ceil(max): 8 range: min: 5.000000, max: 5.000000, ceil(max): 5
Flags: needinfo?(mchiang)
Attached patch print_fps.patch (obsolete) (deleted) — Splinter Review
Attach the fps range log patch
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/3c4afc844a6a use fixed fps instead of floating fps to avoid very low fps. r=jib
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
FWIW it seems odd to me that we report 31 for a range of min: 30.000030, max: 30.000030. Is this some sort of common practice?
Flags: needinfo?(mchiang)
When we call startCapture() with the capability (maxFPS=31), setCaptureHeight() will try to match a range which's max fps, e.g., 30.000030, is the closest (but smaller than or equal) to the target, e.g., 31. [1] [1] http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/media/webrtc/trunk/webrtc/modules/video_capture/mac/avfoundation/video_capture_avfoundation_objc.mm#130
Flags: needinfo?(mchiang)
Seems like this could be solved in the implementation, e.g. round to 3 or 4 decimals here (e.g. add 0.0001 to the left of the > comparison). Unless there's some side-effect I'm not seeing, I think I'd prefer rounding in the implementation over exposing ceiling values (31) to JS.
Attached patch rounding.patch (deleted) — Splinter Review
(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #27) > Seems like this could be solved in the implementation, e.g. round to 3 or 4 > decimals here (e.g. add 0.0001 to the left of the > comparison). > > Unless there's some side-effect I'm not seeing, I think I'd prefer rounding > in the implementation over exposing ceiling values (31) to JS. Rounding is indeed better. What about just rounding to integer?
Attachment #8913588 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: