Closed Bug 1502313 Opened 6 years ago Closed 6 years ago

Adding video to Facebook audio call on Linux fails even if the permission is granted

Categories

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

All
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla65
Tracking Status
firefox-esr60 --- unaffected
firefox63 --- unaffected
firefox64 + verified
firefox65 + verified

People

(Reporter: obotisan, Assigned: pehrsons)

References

Details

(Keywords: regression)

Attachments

(1 file)

[Affected versions]:
- Beta 64.0b3
- Nightly 65.0a1.

[Affected platforms]:
- Ubuntu 16.04
- Ubuntu 18.04  


[Steps to reproduce]:
1. Log into Facebook.
2. Click on one of the friends from the list on the right side of the site.
3. Instead of messaging them try to call them. 
4. Enable the video. 

[Expected result]:
- The video starts without any issues.

[Actual result]:
- Nothing happens. The video doesn't start and if the user tries to activate it again, the permission doorhanger for the webcam is displayed again. 

[Regression]:
- This is a regression. I will try to find the pushlog as soon as possible. 

[Additional Notes]:
- If you wait a while, the call fails. 
- This issue is not reproducing on Chrome.
OS: Unspecified → Linux
Hardware: Unspecified → All
I tested Nightly 64 from Aug 14 and it was good. Nightly 64 from Oct 20 was bad, though my STR was slightly different:

3. Make a voice call
4. Enable video
5. Approve gUM with same mic as chosen for the voice call
6. Verify video is showing
7. Disable video
8. Verify video goes black, camera light turns off
9. Enable video

Expected:
Button changes from crossed-over-camera to camera, camera light turns on and I can see myself in the corner.

Actual:
Button state changes as I hold down the mouse button, but after releasing nothing has changed.



One difference I noted is that AudioStreamTrack/VideoStreamTrack/LocalMediaStream are now gone. It could be something on Facebook's side depending on these for Firefox only.
Rank: 12
Priority: -- → P2
I finally found the regression. Hope this helps.

Last good revision: f0aeab777cf0ae2d7a2b06b48a0254ab10e3794e (2018-09-24)
First bad revision: ae4bb7377de29271d14f5ff56f08adec53f8fe0b  (2018-09-25)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f0aeab777cf0ae2d7a2b06b48a0254ab10e3794e&tochange=ae4bb7377de29271d14f5ff56f08adec53f8fe0b
Interesting. Can you confirm whether this works as expected on 63 or 64 before bug 1481152 landed?
Flags: needinfo?(oana.botisan)
I tried with Firefox 63.0 - build 2 and the bug is not reproducing. 
On the other hand on devEdition 64.0b1 the bug is already reproducing. The only version of 64 that is not affected is the Nightly 64.0a1 from 2018-09-24.
Flags: needinfo?(oana.botisan)
(In reply to Andreas Pehrson [:pehrsons] from comment #3)
> Interesting. Can you confirm whether this works as expected on 63 or 64
> before bug 1481152 landed?

That was the wrong bug number, sorry. I meant before bug 1404977 landed.

I'll run some tests myself.
Andreas which bug is it you think is causing problems: bug 1481152 or bug 1404977?
Flags: needinfo?(apehrson)
(In reply to Oana Botisan from comment #0)
> [Steps to reproduce]:
> 1. Log into Facebook.
> 2. Click on one of the friends from the list on the right side of the site.
> 3. Instead of messaging them try to call them. 

Does call mean an audio only call in this case?

> 4. Enable the video. 

And then you add video to the running audio call?

Does the video never work for you? Because for Andreas it sounds like only adding, removing and then re-enabling the video causes problems?
Flags: needinfo?(oana.botisan)
Assignee: nobody → apehrson
(In reply to Nils Ohlmeier [:drno] from comment #7)
> (In reply to Oana Botisan from comment #0)
> > [Steps to reproduce]:
> > 1. Log into Facebook.
> > 2. Click on one of the friends from the list on the right side of the site.
> > 3. Instead of messaging them try to call them. 
> 
> Does call mean an audio only call in this case?
> 
> > 4. Enable the video. 
> 
> And then you add video to the running audio call?
> 
> Does the video never work for you? Because for Andreas it sounds like only
> adding, removing and then re-enabling the video causes problems?

Yes, that was what I meant. I should have explained the steps better.
Ii I start a video chat from the beginning I encounter no issues, only when I start a voice call and then enabled the video.
Flags: needinfo?(oana.botisan)
When I start a video call and disable, then enable the video again, I encounter no issues either.
(In reply to Nils Ohlmeier [:drno] from comment #6)
> Andreas which bug is it you think is causing problems: bug 1481152 or bug
> 1404977?

Bug 1481152 is causing problems; it limits the number of concurrent audio input streams to 1 on linux, because bug 1404977 revealed that we have pretty severe performance problems when opening subsequent input streams through pulse.

Bug 1481152 was meant to revert our behavior to what we had prior to bug 1404977, with one exception: prior to bug 1404977 we could open any number of input streams of the same device in the same child process (they were using the same pulse stream), and now we can only open one (they're all separate pulse streams). This is why facebook broke.

Facebook requests audio-only gUM when you start a voice call. When you then enable video they request audio+video gUM. I expect this to fail since we don't allow a second stream for the audio from that device. This is not what I saw, so I'll have to look into why. It matches what QA saw however.

This difference could be because I have turned off the cubeb sandbox (i.e., remoting) with "media.cubeb.sandbox" set to false. This has shown to give us better audio perf when opening and closing streams. In general video meetings work much better with this off.

I see the following options short-term:
- We can raise the limit of the number of concurrent input streams on linux with "media.cubeb_max_input_streams", with the side effect that opening a second will often take several seconds, and may cause excessive buffering in pulse. Opening more seems to scale exponentially.
- We can turn off audio remoting on linux, and remove or raise the input stream limit per above. This improves perf and brings linux closer to the other platforms, but leaves us completely without audio remoting so changes would largely go untested and unused.
- We can communicate this change in behavior and have services work around this instead, until we have improved the perf situation. For facebook it *seems* like a simple change to request only an additional video track instead of audio+video, when enabled video on a voice call.

Personally I think we should turn off audio remoting because this situation is not sustainable. With it enabled there's often audio underruns and buffering issues that make video calls with more than one remote bad. This seems like a political issue however.

Long-term we obviously need to get to the bottom of this perf issue and any remoting issues making it worse. So far, the people who have looked at it have said it's a pulse issue that is mostly out of our hands. Perhaps we can go deeper and work around it or even fix it in pulse, but we don't have the resources at the moment to do either, unless we cut something else.
Flags: needinfo?(apehrson) → needinfo?(drno)
Ah, yes, I can only reproduce like QA with remoting *enabled* because only then do we apply the input stream limit, [1].

In comment 1 I wasn't able to re-enable video with remoting *disabled* but today I could, so I'm not sure there's an issue around that on our end but I doubt it, since audio was working fine. It could also be something that Facebook fixed in the meantime.

[1] https://searchfox.org/mozilla-central/rev/8848b9741fc4ee4e9bc3ae83ea0fc048da39979f/dom/media/webrtc/MediaEngineWebRTCAudio.cpp#667
Flags: needinfo?(drno)
Summary: Video on Facebook can't be enabled even if the permission is granted → Adding video to Facebook audio call on Linux fails even if the permission is granted
Thank you for digging in and finding the root cause here!

The idea of turning off cubeb remoting is not so much a political issue, as more that we are currently moving all platforms to remoting to be able to increase the security of Firefox users (stricter sandboxes). So I don't think that is a really good option.

I lean towards doing the long term solution and increase the amount of input streams to two to be able to handle this scenario here if needed (assuming that is not a scenario which happens a offten).
(In reply to Nils Ohlmeier [:drno] from comment #12)
> Thank you for digging in and finding the root cause here!
> 
> The idea of turning off cubeb remoting is not so much a political issue, as
> more that we are currently moving all platforms to remoting to be able to
> increase the security of Firefox users (stricter sandboxes). So I don't
> think that is a really good option.

That's what I mean. If it wasn't political we wouldn't switch until it had decent performance.


> I lean towards doing the long term solution and increase the amount of input
> streams to two to be able to handle this scenario here if needed (assuming
> that is not a scenario which happens a offten).

I'll put up the patch to increase the limit to 2 streams, but I'll also take some measurements on the latency induced by opening a second stream.
After more testing I have to go back on some of my previous claims.

So without the limit we will handle sharing of the same device (two gUM tracks will result in one pulse stream) and deny starting a second device, from within the same child process.

Starting the same or other devices from other child processes is creating new pulse streams as I expected.


I'm still a bit confused with expectations here, but I think that the limit can be removed in light of this. Requesting gUM in another tab while in a heavy call seems like a very narrow use case.
No longer blocks: 1404977
Status: NEW → ASSIGNED
The limit only applied within a child process.

However, within a child process we already share cubeb stream when requesting
the same device multiple times, and disallow capturing from more than one device
at a time.

This limit no longer has any effect.
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/25c74b36e32a
Remove concurrent audio input device limit. r=padenot
https://hg.mozilla.org/mozilla-central/rev/25c74b36e32a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Please request Beta approval on this when you get a chance.
Flags: qe-verify+
Flags: needinfo?(apehrson)
Comment on attachment 9021459 [details]
Bug 1502313 - Remove concurrent audio input device limit. r?padenot

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1481152

User impact if declined: Websites that request an audio twice or more won't work. An example of this is going from a voice call to a video call on Facebook.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: See comment 0, steps 1, 2; and comment 1, steps 3, 4, 5. Expectation in comment 0.

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This is removing an artificial limit we put in earlier - a plain backout commit. It turned out to be wrong and we already handle the cases the limit disallowed. This is true for beta and nightly both.

String changes made/needed: None
Flags: needinfo?(apehrson)
Attachment #9021459 - Flags: approval-mozilla-beta?
Comment on attachment 9021459 [details]
Bug 1502313 - Remove concurrent audio input device limit. r?padenot

[Triage Comment]
Fixes website bustage if audio is requested twice or more. Approved for 64.0b6.
Attachment #9021459 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I retested everything using the latest Nightly 65.0a1 and beta 64.0b6 on Ubuntu 16.04 x32 and Ubuntu 18.04 x64. I can't reproduce the issue anymore. I think this bug is fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: