1403714 - Firefox stops sending video in low-bandwidth conditions and doesn't recover

Status: VERIFIED FIXED

(Core :: WebRTC, defect, P1)

Description

[Nils Ohlmeier [:drno]]

Something causes Firefox on appear.in in rooms with an SFU to stop sending video some time into the call.
Audio continues to work, but from a users perspective the remote video freezes. And on the network it was confirmed that Firefox no longer sends video packets.

Comment 1

[Nils Ohlmeier [:drno]]

Andreas, I just assigned this to you because I heard you are looking into this. Feel free to correct, if that was wrong information.

Comment 2

[Andreas Pehrson [:pehrsons]]

I am looking indeed.

Comment 3

[Andreas Pehrson [:pehrsons]]

It's the VideoAdapter in VideoConduit that tells us to drop frames, from [1].

It only logs it every 90 frames though, which is probably why I missed it in earlier logs.

Here the first log in my last run;
> 2017-09-28 09:55:16.906098 UTC - [VideoFrameConverter #1]: D/webrtc_trace (videoadapter.cc:151): VAdapt Drop Frame: scaled 0 / out 3907 / in 3997 Changes: 0 Input: 640x480 timestamp: 0 Output: i0


[1] http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/media/webrtc/trunk/webrtc/media/base/videoadapter.cc#151

Comment 4

[Andreas Pehrson [:pehrsons]]

[Tracking Requested - why for this release]:
In a WebRTC call, typically with multiple participants, Firefox might stop sending video. Audio still works.

In a bit more detail:
When max-fs is *not* present in the sdp we represent it as having the value 0. If we then are under enough load, or perhaps a bandwidth constraint, that webrtc.org code (our encoder stack, more or less) considers it useful to scale the video stream we send down a notch, we tell it to use a maximum frame size of 0 pixels (from the max-fs value), effectively bringing sent video to a halt.

In full detail: see http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp#1913.

Comment 5

[Philipp Hancke [:fippo]]

this might potentially affect P2P interop with chrome which does not set max-fr or max-fs in the SDP

Comment 6

[Andreas Pehrson [:pehrsons]]

A confirmed workaround is for the application to set max-fs in the sdp.

Comment 7

[Andreas Pehrson [:pehrsons]]

This seems easiest to trigger by being in a low-bandwidth environment.

Comment 8

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1403714 - Only regard max_fs when set explicitly.

Review commit: https://reviewboard.mozilla.org/r/184674/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184674/

Comment 9

[Dan Minor [:dminor]]

Comment on attachment 8913301 [details]
Bug 1403714 - Only regard max_fs when set explicitly.

https://reviewboard.mozilla.org/r/184674/#review189836

Thank you for investigating and fixing this.

It would be nice to have a test for this, but as far as I know we have no way to simulate a low bandwidth situation from a Mochitest and the media conduit gtests are not in good shape.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:1912
(Diff revision 1)
>      // limit sink wants based upon max-fs constraint
>      int max_fs = mCurSendCodecConfig->mEncodingConstraints.maxFs*(16*16);
>      rtc::Optional<int> max_pixel_count = wants.max_pixel_count;
>      rtc::Optional<int> max_pixel_count_step_up = wants.max_pixel_count_step_up;
>  
> -    if (max_pixel_count.value_or(max_fs) > max_fs) {
> +    if (max_fs > 0 && max_pixel_count.value_or(max_fs) > max_fs) {

I think this would read better with one
if(max_fs > 0)
and the existing if statements nested inside.

Comment 10

[Andreas Pehrson [:pehrsons]]

I have confirmed this fix locally and can no longer reproduce. I'll file a followup for covering this with a unit test.

To reproduce I put one client in an appear.in premium room and loaded it with clients from other machines.

Appear.in might soon deploy the workaround from comment 6 to their sfu so don't bet on these steps working for long.


I imagine the easiest way to reproduce would be to do a 1-on-1 with a remote machine over a weak link, like wifi in a loaded office. Or use a network shaper to limit bandwidth and perhaps introduce packet loss.
To make sure the service is eligible for the bug, also ensure that the sdps in use don't contain max-fs on about:webrtc.

Comment 11

[Philipp Hancke [:fippo]]

possibly adding b=TIAS lines in the tests could help. Depends on how long you need to get...

Comment 12

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8913301 [details]
Bug 1403714 - Only regard max_fs when set explicitly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184674/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6314d54af8b8
Only regard max_fs when set explicitly. r=dminor

Comment 14

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

We'll want to get uplift requests for beta 57 and release 56 in place, and also manual QA verification if possible on a (Try) build of release with the patch, so we're in a position to consider it as a ride-along for any 56 point release.  It's a Very Safe patch.

And thanks!

Comment 15

[Nils Ohlmeier [:drno]]

Comment on attachment 8913301 [details]
Bug 1403714 - Only regard max_fs when set explicitly.

Approval Request Comment
[Feature/Bug causing the regression]: 1393687
[User impact if declined]: High chances that Firefox during a WebRTC stops sending video.
[Is this code covered by automated tests?]: The positive case from bug 1393687 yes, but the actual faulty behavior not yet.
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: Yes please.
- Open https://jsfiddle.net/yxbLvjm6/14/
- Allow access to camera and microphone
- Verify that two videos of yourself appear in the lower right corner
- Leave the call running for 30 seconds
- Verify that the both videos are still showing movement and are not frozen
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: The change is only a one line fix which ensure that the maximum bandwidth never goes to zero.
[String changes made/needed]: N/A

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 57 since it's a recent regression. Liz FYI, potential dot release ride-along.

Comment 17

[Nils Ohlmeier [:drno]]

Comment on attachment 8913301 [details]
Bug 1403714 - Only regard max_fs when set explicitly.

Approval Request Comment
[Feature/Bug causing the regression]: 1393687
[User impact if declined]: High chances of Firefox stopping to send video if the machine gets under load.
[Is this code covered by automated tests?]: The patch from bug 1393687 has test coverage, but does not cover the case causing this bug here. Automated test will come in bug 1404039.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: Yes please.
- Open https://jsfiddle.net/yxbLvjm6/14/
- Allow access to camera and microphone
- Verify that two videos of yourself appear in the lower right corner
- Leave the call running for 30 seconds
- Verify that the both videos are still showing movement and are not frozen
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: The change is only a one line fix which ensure that the maximum bandwidth never goes to zero.
[String changes made/needed]: N/A

Comment 18

[Nils Ohlmeier [:drno]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=09c519bd62b6eb2fe383ce594e722edfacb9fd82

Comment 19

[Nils Ohlmeier [:drno]]

(In reply to Nils Ohlmeier [:drno] from comment #18)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=09c519bd62b6eb2fe383ce594e722edfacb9fd82

Try build on Beta.

Comment 20

[Nils Ohlmeier [:drno]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=904910d851d2bab9d84d6a662a2027a9447ee0c6

Comment 21

[Nils Ohlmeier [:drno]]

(In reply to Nils Ohlmeier [:drno] from comment #20)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=904910d851d2bab9d84d6a662a2027a9447ee0c6

Try run for release.

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6314d54af8b8

Comment 23

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

We could consider this as a ridealong, looks like it just missed uplift for the RC2 build in bug 1393687. 
The fix there didn't sound urgent or like a new problem (see https://bugzilla.mozilla.org/show_bug.cgi?id=1393687#c13 and https://bugzilla.mozilla.org/show_bug.cgi?id=1393687#c49)  

Is this a new regression? Is it a widespread problem? Any different from the situation on 9/18?

Comment 24

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

Also am I understanding correctly that appear.in has a fix coming?

Comment 25

[Andreas Pehrson [:pehrsons]]

This is a new regression caused by the patch in bug 1393687 that made it into 56, see [1]. Thus this was present on 9/18, but not something we tested for as part of that bug. I would note though that what bug 1393687 fixed and got uplifted did indeed regress in 56. There was a second part that landed later, was not uplifted, and regressed in 52 according to reports.

It affects everyone sending video in WebRTC to other clients than Firefox, so I'd say widespread, yes. Appear.in has deployed the workaround in comment 6 for both their premium and regular rooms. Whether other services have worked around it I'm not sure, but plenty have been reached out to.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1393687#c35

Comment 26

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

OK, thanks for the further explanation (and the try run on release, btw!) 
Are there other services/sites we can test with?

Andrei, can your team verify the fix?  I know your team is overloaded with 57 testing, so I am trying to keep 56 requests to a minimum...

Comment 27

[Ke Shen]

Can the max-fs setting show up only in offer SDP? Or the answer SDP need to have this setting?

Comment 28

[Philipp Hancke [:fippo]]

lizzard: appear.in was fixed for 56 and 57 in the p2p mode. If you can change the useragent (which can be done by setting in general.useragent.override apparently) to the FF58 one you should still be able to reproduce in 56 or 57 with the following steps:
1) go to https://appear.in/some-random-room-name-you-use-for-mozilla-qa?bandwidth=100 in Firefox
") go to https://appear.in/some-random-room-name-you-use-for-mozilla-qa?bandwidth=100 in Chrome, on the same machine
This froze pretty instantly for me.

ke shen: add max-fs to any setRemoteDescription call that does not have it, regardless of the type.

Comment 29

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

(In reply to Ke Shen from comment #27)
> Can the max-fs setting show up only in offer SDP? Or the answer SDP need to
> have this setting?

This is more a question for the IETF, but max-fs can be in the answer only - it says "don't send me anything larger than this please".  There's no negotiation around it; either side can specify a max-fs or not specify it.

Comment 30

[Nils Ohlmeier [:drno]]

(In reply to Randell Jesup [:jesup] from comment #29)
> (In reply to Ke Shen from comment #27)
> > Can the max-fs setting show up only in offer SDP? Or the answer SDP need to
> > have this setting?
> 
> This is more a question for the IETF, but max-fs can be in the answer only -
> it says "don't send me anything larger than this please".  There's no
> negotiation around it; either side can specify a max-fs or not specify it.

So to prevent this bug from affecting your service max-fs needs to be in the remote SDP. Depending on if Firefox is the offerer or answerer, its needs to be in the opposite one.

So safest is to put some value in all SDPs. The values can also simply be insanely high. As long as it's present and not 0 you should get hit by this bug.

Comment 31

[Nils Ohlmeier [:drno]]

I just tried verification with 58 build id 20170929100122 on https://jsfiddle.net/yxbLvjm6/14/, but it fails (it still freezes).

Comment 32

[Nils Ohlmeier [:drno]]

(In reply to Nils Ohlmeier [:drno] from comment #31)
> I just tried verification with 58 build id 20170929100122 on
> https://jsfiddle.net/yxbLvjm6/14/, but it fails (it still freezes).

Doubled checked. The latest build of Nightly does not have the fix yet.

Comment 33

[Nils Ohlmeier [:drno]]

I just verified with the fiddle that the problem is fixed in 58.0a1 (2017-09-29) with build id 20170929220356.

Comment 34

[Philipp Hancke [:fippo]]

https://jsfiddle.net/o5subvcb/1/ shows how to munge the SDP to add max-fs (and max-fr) to both VP8 and VP9

Comment 35

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8913301 [details]
Bug 1403714 - Only regard max_fs when set explicitly.

Fix a recent regression, taking it.
Should be in 57b5

Comment 36

[Oana Botisan, Desktop QA]

I managed to reproduce the bug using 56.0 build 6 and beta 57.0b4 on Windows 10x64. I tested using the steps from comment 17 and comment 28. 

I retested everything using the latest Nightly 58.0a1 on Windows 10x64, Ubuntu 16.04x64 and Mac OS 10.11 and the bug is not reproducing anymore. But I have to mention that there is still some jerkiness in the received video.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/98673c4c15cc

Comment 38

[Nils Ohlmeier [:drno]]

(In reply to Oana Botisan from comment #36)
> I retested everything using the latest Nightly 58.0a1 on Windows 10x64,
> Ubuntu 16.04x64 and Mac OS 10.11 and the bug is not reproducing anymore. But
> I have to mention that there is still some jerkiness in the received video.

Thanks for checking. And yes jerkiness of the video in this special test case is expected as the test artificially limits the bandwidth to a super low value to trigger the bug. So we don't expect the video to ever look great. The important factor is if the second, remote video moves at all.

Comment 39

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

Let's keep this tracked in 56 and we can uplift it as a ridealong in case we have a dot release driver.
We have a planned dot release for 64-bit migration only, which will likely happen first.

Comment 40

[xpeng]

the max-fr does not work in nightly 58.0a1(2017-10-12)64bit  &  Firefox Quantum 57.0b7(64bit)- beta , we open camera with 1080P 30fps , and set remote sdp with max-fs = 920, max-fr = 15, we find that the framerate send is more than 15fps .(VP8/VP9)
the case with codec h264 should also be considered  which control the framerate with max-mbps & max-fs.

Comment 41

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

Andreas, should we reopen this for 58 and possibly 57 because of comment 40? Did something else change?

Comment 42

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

Or, if we should open a followup bug instead, please do that.

Comment 43

[Dan Minor [:dminor]]

This appears to be a negative interaction between max-fr and max-fs when max-fs is set to a low value.

I did a bit of experimentation and with max-fr set to 15 and max-fs set to below 1300, the framerate is limited to around 16 - 18 frames per second rather than the 15 requested. If I don't set max-fr, the camera runs at 30. I'll open a new bug.

The parameters for h.264 is Bug 1376255.

Comment 44

[Dan Minor [:dminor]]

I filed Bug 1408462.

Comment 45

[Andreas Pehrson [:pehrsons]]

Thanks Dan. Liz, I consider the max-fr issue in comment 40 to be of much less importance than this, and I think it's irrelevant to this bug.

Comment 46

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

I'm putting together a 56 dot release now (with 3 weeks to go in the release cycle). 
If you would like to include this fix, can you request uplift to m-r? Thanks!

Comment 47

[Andreas Pehrson [:pehrsons]]

Thanks. There's an uplift request in comment 17.

Comment 48

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

Comment on attachment 8913301 [details]
Bug 1403714 - Only regard max_fs when set explicitly.

Ridealong for 56.0.2

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/a3ac637118b1

Comment 50

[Kohei Yoshino]

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/webrtc-video-may-stop-in-low-bandwidth-conditions/

Comment 51

[Oana Botisan, Desktop QA]

The bug was verified using the steps from comment 17 on RC 56.0.2 - build 1 and beta 57.0b11 on Windows 10 x64, Windows 7 x64, Ubuntu 16.04 x64 and macOS 10.13 and the bug is not reproducing anymore. Even after a few minutes the second video didn't freeze.