Setting b=TIAS caps us at 2kbps

RESOLVED FIXED in Firefox 53

Status

()

P2
normal
Rank:
25
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: pehrsons, Assigned: jesup)

Tracking

51 Branch
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 unaffected, firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(11 attachments)

73.38 KB, image/png
Details
95.14 KB, image/png
Details
413.54 KB, text/plain
Details
2.12 MB, text/plain
Details
58 bytes, text/x-review-board-request
jesup
: review+
bwc
: review+
Details | Review
58 bytes, text/x-review-board-request
bwc
: review+
Details | Review
58 bytes, text/x-review-board-request
jesup
: review+
bwc
: review+
Details | Review
58 bytes, text/x-review-board-request
bwc
: review+
Details | Review
58 bytes, text/x-review-board-request
jesup
: review+
Details | Review
7.89 KB, patch
pehrsons
: feedback+
pkerr
: feedback+
Details | Diff | Splinter Review
14.70 KB, patch
pehrsons
: feedback-
jesup
: feedback?
pkerr
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
I see this in Nightly 53 on Mac with full duplex audio. I do think it's more generic than these settings however.

To reproduce:

1. Join an appear.in room, mute yourself by hitting 'm' after capture has started
2. Open a new tab and join the same room, again muting (audio is irrelevant)
3. Repeat (2) until video has trouble keeping up between the first two tabs. I would usually add tabs until local audio capture (not over a peerconnection) would see some dropouts but I don't think it's needed to go that far. Worth trying if you can't reproduce otherwise.
4. Close all tabs except the first two.
5. Check that audio still works (might have latency in full duplex, but this is a known issue). This probably means video works too.

Expected:
Video frame rate should pick back up to same as local capture.

Actual:
Video frame rate is extremely low. One frame per 2-3 seconds for me.
about:webrtc reports a rapidly increasing number of dropped frames in the encoder. This is fine under high load but it should recover...
(Reporter)

Comment 1

2 years ago
Created attachment 8814153 [details]
Graph of the stat packetsSent marked when b=TIAS got applied

Looking at the collected stats for one of these connections it looks like a renegotiation with b=TIAS:384000 triggers this. Setting that in setRemoteDescription() correlates with the point marked in the graph in this attachment.
(Reporter)

Comment 2

2 years ago
Created attachment 8814173 [details]
Graph of droppedFrames and other encoder stats

This shows it very clearly.

When b=TIAS:256000 is applied, droppedFrames and framerateStdDev start growing significantly. bitrateMean and framerateMean start shrinking continuously.

Also note that at the point of the marker droppedFrames is increasing by 400 per second.

40 minutes later it's increasing by 35000 per second.

That's clearly wrong. Are we accumulating the accumulator?
(Reporter)

Updated

2 years ago
Attachment #8814173 - Attachment description: Screenshot 2016-11-24 17.56.46.png → Graph of droppedFrames and other encoder stats
(Reporter)

Comment 3

2 years ago
And clearly we need more testing of b=TIAS and stats across the board. Preferably in many combinations. Some sanity checks in mochitests at the very least.
(Reporter)

Updated

2 years ago
Summary: High CPU load causes frame drops in video encoder that does not recover → High CPU load and setting b=TIAS causes frame drops in video encoder that do not recover
Created attachment 8814179 [details]
dump of api traces + getStats

I grabbed the raw dump from the session and attached it. It can be imported with https://fippo.github.io/webrtc-dump-importer/rtcstats
(Reporter)

Comment 5

2 years ago
Comment 2 is PC_1 in the dump in comment 4, though not all data made it in it seems. The most interesting bits are there.
(Reporter)

Comment 6

2 years ago
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #2)
> 40 minutes later it's increasing by 35000 per second.
> (...)
> Are we accumulating the accumulator?

40*60*20 (we were at 20 FPS) = 48000. Pretty close.
(Reporter)

Comment 7

2 years ago
[0] shows how we update the frame rate that we base our bitrate calculations on. Note that it's triggered by getStats and is the frame rate coming out of the encoder rather than of the camera. The encoder may (and is, in this case) drop frames.


[0] http://searchfox.org/mozilla-central/rev/b4e6fa3527436bdbcd9dd2e1982382fe423431f3/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp#197
Created attachment 8814209 [details]
another dump that andreas is describing
Andreas can you verify if your Nightly has already the patch from bug 1308481 included?
Flags: needinfo?(pehrson)
(Reporter)

Comment 10

2 years ago
I do. Here's my build if you want to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4a72f32a0f8

And I cannot repro in 52!
status-firefox52: --- → unaffected
status-firefox53: --- → affected
Flags: needinfo?(pehrson)
(Reporter)

Comment 11

2 years ago
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #5)
> Comment 2 is PC_1 in the dump in comment 4, though not all data made it in
> it seems. The most interesting bits are there.

This was wrong.

Comment 1 and comment 4 are for the same connection, PC_1.

And comment 2 and comment 8, also PC_1.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Updated

2 years ago
Assignee: nobody → pehrson
Status: NEW → ASSIGNED
(Assignee)

Comment 17

2 years ago
mozreview-review
Comment on attachment 8814514 [details]
Bug 1320101 - Default to 2Mbps when no max bitrate is set.

https://reviewboard.mozilla.org/r/95716/#review95764
Attachment #8814514 - Flags: review+
(Assignee)

Comment 18

2 years ago
mozreview-review
Comment on attachment 8814515 [details]
Bug 1320101 - Support renegotiations with changes to TIAS and simulcast params.

https://reviewboard.mozilla.org/r/95718/#review95766
Attachment #8814515 - Flags: review?(rjesup) → review+
(Assignee)

Comment 19

2 years ago
mozreview-review
Comment on attachment 8814516 [details]
Bug 1320101 - mNegotiatedMaxBitrate should be able to cap the max bitrate.

https://reviewboard.mozilla.org/r/95720/#review95768
Attachment #8814516 - Flags: review+
(Assignee)

Comment 20

2 years ago
mozreview-review
Comment on attachment 8814517 [details]
Bug 1320101 - Pass mLastFramerateTenths by copy.

https://reviewboard.mozilla.org/r/95722/#review95770
Attachment #8814517 - Flags: review?(rjesup) → review+
Rank: 25
Priority: -- → P2

Comment 21

2 years ago
mozreview-review
Comment on attachment 8814514 [details]
Bug 1320101 - Default to 2Mbps when no max bitrate is set.

https://reviewboard.mozilla.org/r/95716/#review95970
Attachment #8814514 - Flags: review?(docfaraday) → review+

Comment 22

2 years ago
mozreview-review
Comment on attachment 8814515 [details]
Bug 1320101 - Support renegotiations with changes to TIAS and simulcast params.

https://reviewboard.mozilla.org/r/95718/#review95974
Attachment #8814515 - Flags: review?(docfaraday) → review+

Comment 23

2 years ago
mozreview-review
Comment on attachment 8814516 [details]
Bug 1320101 - mNegotiatedMaxBitrate should be able to cap the max bitrate.

https://reviewboard.mozilla.org/r/95720/#review95976

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:1199
(Diff revision 1)
> -  if (mNegotiatedMaxBitrate != 0 && mNegotiatedMaxBitrate > out_max) {
> +  if (mNegotiatedMaxBitrate != 0 && mNegotiatedMaxBitrate < out_max) {
>      out_max = mNegotiatedMaxBitrate;
>    }

MinIgnoreZero?
Attachment #8814516 - Flags: review?(docfaraday) → review+

Comment 24

2 years ago
mozreview-review
Comment on attachment 8814513 [details]
Bug 1320101 - Differentiate between b=TIAS and simulcast stream max-br.

https://reviewboard.mozilla.org/r/95714/#review95980
Attachment #8814513 - Flags: review?(docfaraday) → review+

Updated

2 years ago
See Also: → bug 1319019
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Updated

2 years ago
Attachment #8814513 - Flags: review+ → review?(docfaraday)
(Reporter)

Updated

2 years ago
Attachment #8814515 - Flags: review?(rjesup)
Attachment #8814515 - Flags: review?(docfaraday)
Attachment #8814515 - Flags: review+
(Assignee)

Comment 30

2 years ago
mozreview-review
Comment on attachment 8814513 [details]
Bug 1320101 - Differentiate between b=TIAS and simulcast stream max-br.

https://reviewboard.mozilla.org/r/95714/#review97210

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:708
(Diff revision 2)
>      video_codec.resolution_divisor = 1; // We could try using it to handle odd resolutions
>  #endif
>      video_codec.qpMax = 56;
>      video_codec.numberOfSimulcastStreams = 1;
>      video_codec.simulcastStream[0].jsScaleDownBy =
> -        codecConfig->mEncodingConstraints.scaleDownBy;
> +        codecConfig->mSimulcastEncodings[0].constraints.scaleDownBy;

scaleDownBy is IIRC per layer.  I think this is ok though since this is setting it for [0]
Attachment #8814513 - Flags: review?(rjesup) → review+
(Assignee)

Comment 31

2 years ago
mozreview-review
Comment on attachment 8814515 [details]
Bug 1320101 - Support renegotiations with changes to TIAS and simulcast params.

https://reviewboard.mozilla.org/r/95718/#review97212

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:743
(Diff revisions 1 - 2)
>  
> -  // So we can comply with b=TIAS/b=AS/maxbr=X when input resolution changes
> +  video_codec.mode = mCodecMode;
> -  mNegotiatedMaxBitrate = MinIgnoreZero(mPrefMaxBitrate, video_codec.maxBitrate);
>  
>    if (mSendingWidth != 0) {
> -    // We're already in a call and are reconfiguring (perhaps due to
> +    MutexAutoLock lock(mCodecMutex);

Is there any deadlock risk holding this while operating on ViECodec/etc?
Attachment #8814515 - Flags: review?(rjesup) → review+

Comment 32

2 years ago
mozreview-review
Comment on attachment 8814513 [details]
Bug 1320101 - Differentiate between b=TIAS and simulcast stream max-br.

https://reviewboard.mozilla.org/r/95714/#review97240

So, I'm a little unclear on what we ought to be doing with the stuff in mCurSendCodecConfig->mEncodingConstraints vs mCurSendCodecConfig->mSimulcastEncodings[0].constraints. It seems like we should be doing MinIgnoreZero here, but maybe in some cases not?

::: media/webrtc/signaling/src/media-conduit/VideoConduit.h:272
(Diff revision 2)
>    unsigned int SendingMaxFs() override {
>      if(mCurSendCodecConfig) {
> -      return mCurSendCodecConfig->mEncodingConstraints.maxFs;
> +      return mCurSendCodecConfig->mSimulcastEncodings[0].constraints.maxFs;
>      }
>      return 0;
>    }
>  
>    unsigned int SendingMaxFr() override {
>      if(mCurSendCodecConfig) {
> -      return mCurSendCodecConfig->mEncodingConstraints.maxFps;
> +      return mCurSendCodecConfig->mSimulcastEncodings[0].constraints.maxFps;
>      }
>      return 0;
>    }

Hmm. What if the root encoding constraint is smaller?

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:1264
(Diff revision 2)
> -    if (mCurSendCodecConfig->mEncodingConstraints.maxFs)
> +    if (mCurSendCodecConfig->mSimulcastEncodings[0].constraints.maxFs)
>      {
> -      uint32_t max_fs = mCurSendCodecConfig->mEncodingConstraints.maxFs;
> +      uint32_t max_fs = mCurSendCodecConfig->mSimulcastEncodings[0].constraints.maxFs;

We can use SendingMaxFs() here, right?
Attachment #8814513 - Flags: review?(docfaraday)

Comment 33

2 years ago
mozreview-review
Comment on attachment 8814515 [details]
Bug 1320101 - Support renegotiations with changes to TIAS and simulcast params.

https://reviewboard.mozilla.org/r/95718/#review97244

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:1298
(Diff revisions 1 - 2)
> -    if (mCurSendCodecConfig->mEncodingConstraints.maxFs)
> +    if (mCurSendCodecConfig->mSimulcastEncodings[0].constraints.maxFs)
>      {
> -      uint32_t max_fs = mCurSendCodecConfig->mEncodingConstraints.maxFs;
> +      uint32_t max_fs = mCurSendCodecConfig->mSimulcastEncodings[0].constraints.maxFs;

SendingMaxFs(), right?

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:2000
(Diff revisions 1 - 2)
> -  if (codecInfo->mEncodingConstraints.maxFps > 0) {
> -    cinst.maxFramerate = codecInfo->mEncodingConstraints.maxFps;
> +  if (codecInfo->mSimulcastEncodings[0].constraints.maxFps > 0) {
> +    cinst.maxFramerate = codecInfo->mSimulcastEncodings[0].constraints.maxFps;
>    } else {
>      cinst.maxFramerate = DEFAULT_VIDEO_MAX_FRAMERATE;
>    }
>  
>    // Defaults if rates aren't forced by pref.  Typically defaults are
>    // overridden on the first video frame.
>    cinst.minBitrate = mMinBitrate ? mMinBitrate : 200;
>    cinst.startBitrate = mStartBitrate ? mStartBitrate : 300;
>    cinst.targetBitrate = cinst.startBitrate;
> -  cinst.maxBitrate = MinIgnoreZero(2000U, codecInfo->mEncodingConstraints.maxBr/1000);
> +  cinst.maxBitrate = MinIgnoreZero(2000U, codecInfo->mTias/1000);
>    // not mNegotiatedMaxBitrate! cinst.maxBitrate is the max for the codec, which will be overridden
>    cinst.maxBitrate = MinIgnoreZero(cinst.maxBitrate, mPrefMaxBitrate);
>  
>    if (cinst.codecType == webrtc::kVideoCodecH264)
>    {
>  #ifdef MOZ_WEBRTC_OMX
>      cinst.resolution_divisor = 16;
>  #endif
>      // cinst.codecSpecific.H264.profile = ?
>      cinst.codecSpecific.H264.profile_byte = codecInfo->mProfile;
>      cinst.codecSpecific.H264.constraints = codecInfo->mConstraints;
>      cinst.codecSpecific.H264.level = codecInfo->mLevel;
>      cinst.codecSpecific.H264.packetizationMode = codecInfo->mPacketizationMode;
> -    if (codecInfo->mEncodingConstraints.maxMbps > 0) {
> +    if (codecInfo->mSimulcastEncodings[0].constraints.maxMbps > 0) {

These seem to be duplicate hunks from another changeset...
Attachment #8814515 - Flags: review?(docfaraday)
(Reporter)

Comment 34

2 years ago
mozreview-review-reply
Comment on attachment 8814513 [details]
Bug 1320101 - Differentiate between b=TIAS and simulcast stream max-br.

https://reviewboard.mozilla.org/r/95714/#review97210

> scaleDownBy is IIRC per layer.  I think this is ok though since this is setting it for [0]

It is per layer, but we're also setting it on a single layer.
(Reporter)

Comment 35

2 years ago
mozreview-review-reply
Comment on attachment 8814513 [details]
Bug 1320101 - Differentiate between b=TIAS and simulcast stream max-br.

https://reviewboard.mozilla.org/r/95714/#review97240

So no logic has really changed - but we're now skipping using mCurSendCodecConfig->mEncodingConstraints as the authoritative source (I removed it) since it was just a dupe of mCurSendCodecConfig->mSimulcastEncodings[0].constraints anyway.

> Hmm. What if the root encoding constraint is smaller?

Which is the root encoding constraint?

If there's one for the whole m-section shouldn't it live in JsepTrackNegotiatedDetails instead? And then become a member of mCurSendCodecConfig when we translate them. Like I did for TIAS.

Perhaps it makes sense to bring back mCurSendCodecConfig->mEncodingConstraints to group those constraints, but then we should make it a new struct so things are explicit IMHO. The way it was we were mixing root track constraints with track encoding constraints in an upper layer (see `NegotiatedDetailsToVideoCodecConfigs` in MediaPipelineFactory.cpp).

I don't think I'm regressing anything here. Perhaps we should take care of the root constraints properly in a follow-up?

Also, do we have tests for them? That would make things a lot a lot easier. Both in terms of regressions and understanding the internal model we use.
(Reporter)

Comment 36

2 years ago
mozreview-review-reply
Comment on attachment 8814515 [details]
Bug 1320101 - Support renegotiations with changes to TIAS and simulcast params.

https://reviewboard.mozilla.org/r/95718/#review97244

> These seem to be duplicate hunks from another changeset...

Yeah, MozReview got confused when I removed some patches for the second push. Try the 0-2 diff.
(Assignee)

Comment 37

2 years ago
When this lands, remove the disable landed in bug 1319019
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 40

2 years ago
mozreview-review
Comment on attachment 8814513 [details]
Bug 1320101 - Differentiate between b=TIAS and simulcast stream max-br.

https://reviewboard.mozilla.org/r/95714/#review97958
Attachment #8814513 - Flags: review?(docfaraday) → review+

Comment 41

2 years ago
mozreview-review
Comment on attachment 8814515 [details]
Bug 1320101 - Support renegotiations with changes to TIAS and simulcast params.

https://reviewboard.mozilla.org/r/95718/#review97960
Attachment #8814515 - Flags: review?(docfaraday) → review+
Comment hidden (mozreview-request)

Comment 43

2 years ago
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b0d0bb5e3cff
Default to 2Mbps when no max bitrate is set. r=bwc,jesup
https://hg.mozilla.org/integration/autoland/rev/8e249f961750
mNegotiatedMaxBitrate should be able to cap the max bitrate. r=bwc,jesup
https://hg.mozilla.org/integration/autoland/rev/38e008ff60f0
Pass mLastFramerateTenths by copy. r=jesup
https://hg.mozilla.org/integration/autoland/rev/337f8b562367
Differentiate between b=TIAS and simulcast stream max-br. r=bwc,jesup
https://hg.mozilla.org/integration/autoland/rev/afdc89065874
Support renegotiations with changes to TIAS and simulcast params. r=bwc,jesup
(Reporter)

Updated

2 years ago
Summary: High CPU load and setting b=TIAS causes frame drops in video encoder that do not recover → Setting b=TIAS caps us at 2kbps
(Reporter)

Updated

2 years ago
Depends on: 1328142
(Assignee)

Comment 45

2 years ago
Created attachment 8823174 [details] [diff] [review]
Differentiate between b=TIAS and simulcast stream max-br

Note: these patches compile.  I have not even tried running them yet.
Attachment #8823174 - Flags: feedback?(pkerr)
Attachment #8823174 - Flags: feedback?(pehrson)
(Assignee)

Updated

2 years ago
Assignee: pehrson → rjesup
(Assignee)

Comment 46

2 years ago
Created attachment 8823175 [details] [diff] [review]
Support renegotiations with changes to TIAS and simulcast params

Reland of bug 1320101 patch 5 due to partial backout via mismerge in bug 1250326

MozReview-Commit-ID: GNWRNnwX9pk
Attachment #8823175 - Flags: feedback?(pkerr)
Attachment #8823175 - Flags: feedback?(pehrson)
(Reporter)

Comment 47

2 years ago
Comment on attachment 8823174 [details] [diff] [review]
Differentiate between b=TIAS and simulcast stream max-br

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

Seems correctly applied to me.
Attachment #8823174 - Flags: feedback?(pehrson) → feedback+
(Reporter)

Comment 48

2 years ago
Comment on attachment 8823175 [details] [diff] [review]
Support renegotiations with changes to TIAS and simulcast params

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

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +507,5 @@
>    // SetSendCodec()
>  
>    if (mSendingWidth != 0) {
>      // We're already in a call and are reconfiguring (perhaps due to
> +    // ReplaceTrack).

This comment was removed in the original patch because we have more detailed ones below.

@@ +528,5 @@
> +      // We update the resolutions in the send codec to match the current
> +      // settings.  Framerate is already set.
> +      width = mSendingWidth;
> +      height = mSendingHeight;
> +      // Bitrates are set in the loop below

Bitrates are set below, but not resolution of simulcast streams. The original patch did that. This is the main reason for f-.

@@ +1419,5 @@
>  
> +  unsigned int framerate = SelectSendFrameRate(mCurSendCodecConfig,
> +                                               mSendingFramerate,
> +                                               mSendingWidth,
> +                                               mSendingHeight);

Where are these changes coming from? Secondary reason for f-.

@@ +1501,5 @@
>      uint32_t new_width = (width / simStream.jsScaleDownBy);
>      uint32_t new_height = (height / simStream.jsScaleDownBy);
>      video_stream.width = width;
>      video_stream.height = height;
> +    // XXX this should depend on the final values (below) of video_stream.width/height, not

Introducing new XXXs doesn't seem right.

@@ +1507,3 @@
>      video_stream.max_framerate = mSendingFramerate;
> +    SelectBitrates(video_stream.width, video_stream.height,
> +                   // XXX formerly was MinIgnoreZero(jsMaxBitrate, codec_max_bitrate)

ditto
Attachment #8823175 - Flags: feedback?(pehrson) → feedback-

Updated

2 years ago
Attachment #8823174 - Flags: feedback?(pkerr) → feedback+
(Assignee)

Comment 49

2 years ago
reopening since it effectively was backed out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

2 years ago
Blocks: 1330318

Comment 50

2 years ago
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84568b4acdcc
Differentiate between b=TIAS and simulcast stream max-br. r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/45500e7a10c3
Support renegotiations with changes to TIAS and simulcast params r=pehrsons

Comment 51

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/84568b4acdcc
https://hg.mozilla.org/mozilla-central/rev/45500e7a10c3
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Blocks: 1359854
You need to log in before you can comment on or make changes to this bug.