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

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.

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?

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.

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

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.

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

[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

Andreas can you verify if your Nightly has already the patch from bug 1308481 included?

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!

(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 on attachment 8814514 [details] Bug 1320101 - Default to 2Mbps when no max bitrate is set. https://reviewboard.mozilla.org/r/95716/#review95764

Comment on attachment 8814515 [details] Bug 1320101 - Support renegotiations with changes to TIAS and simulcast params. https://reviewboard.mozilla.org/r/95718/#review95766

Comment on attachment 8814516 [details] Bug 1320101 - mNegotiatedMaxBitrate should be able to cap the max bitrate. https://reviewboard.mozilla.org/r/95720/#review95768

Comment on attachment 8814517 [details] Bug 1320101 - Pass mLastFramerateTenths by copy. https://reviewboard.mozilla.org/r/95722/#review95770

Comment on attachment 8814514 [details] Bug 1320101 - Default to 2Mbps when no max bitrate is set. https://reviewboard.mozilla.org/r/95716/#review95970

Comment on attachment 8814515 [details] Bug 1320101 - Support renegotiations with changes to TIAS and simulcast params. https://reviewboard.mozilla.org/r/95718/#review95974

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?

Comment on attachment 8814513 [details] Bug 1320101 - Differentiate between b=TIAS and simulcast stream max-br. https://reviewboard.mozilla.org/r/95714/#review95980

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]

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?

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?

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

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.

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.

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.

When this lands, remove the disable landed in bug 1319019

Comment on attachment 8814513 [details] Bug 1320101 - Differentiate between b=TIAS and simulcast stream max-br. https://reviewboard.mozilla.org/r/95714/#review97958

Comment on attachment 8814515 [details] Bug 1320101 - Support renegotiations with changes to TIAS and simulcast params. https://reviewboard.mozilla.org/r/95718/#review97960

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

https://hg.mozilla.org/mozilla-central/rev/b0d0bb5e3cff https://hg.mozilla.org/mozilla-central/rev/8e249f961750 https://hg.mozilla.org/mozilla-central/rev/38e008ff60f0 https://hg.mozilla.org/mozilla-central/rev/337f8b562367 https://hg.mozilla.org/mozilla-central/rev/afdc89065874

Note: these patches compile. I have not even tried running them yet.

Reland of bug 1320101 patch 5 due to partial backout via mismerge in bug 1250326 MozReview-Commit-ID: GNWRNnwX9pk

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.

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

reopening since it effectively was backed out

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

https://hg.mozilla.org/mozilla-central/rev/84568b4acdcc https://hg.mozilla.org/mozilla-central/rev/45500e7a10c3