Closed Bug 1308481 Opened 8 years ago Closed 8 years ago

TIAS bitrate limitation does not work when resolution changes

Categories

(Core :: WebRTC, defect, P1)

Product:
Core
Component:
WebRTC
Version:
49 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: aabagian, Assigned: jesup)

Details

Attachments

(3 files, 4 obsolete files)

ff-wrtc-tias.jpg
(deleted), image/jpeg
Details
ff-wrtc-tias-2.png
(deleted), image/png
Details
process maxBr/TIAS setting for all codecs, not just H264
(deleted), patch
drno
: review+
Details | Diff | Splinter Review
Attached image ff-wrtc-tias.jpg (deleted) —
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.90 Safari/537.36 Vivaldi/1.4.589.11 Steps to reproduce: Added b=TIAS:1000000 to Remote SDP video m-section. Video resolution restrictions looks like {w:1280, h: 720} Actual results: While resulting resolution is 1280x720, the bitrate keeps 1000 kbps as it should. But when the browser decides to decrease resolution to 352x288, the bitrate increases to ~2100 kbps. Expected results: If resolution decreases (f.e. to 352x288), the bitrate should be kept the same (1000kbps).
The picture attached has been taken from a proprietary client, working with MacOS Firefox 49.01 WebRTC client via SFU.
Attached image ff-wrtc-tias-2.png (deleted) —
If resolution changes to 640x480, everything is OK. The problems happen only at 352x288 resolution.
Component: Untriaged → WebRTC
Product: Firefox → Core
Byron/Nils: is the TIAS bandwidth setting getting lost on a re-configure/re-negotiation? (since calling ConfigureSendMediaCodec was the only way we were ending up with 352x288 (CIF), which is now fixed). In general, if the factory is going to reconfigure the codecs, it needs to make sure that the resultant codec/conduit setup is consistent with the previous state. The point-fix for resolution may not be all that's needed.
Flags: needinfo?(drno)
Flags: needinfo?(docfaraday)
When renegotiation happens, we just use whatever was negotiated.
Flags: needinfo?(docfaraday)
Did you reproduce it ?
Whiteboard: [question 2016-10-24 to bwc]
I don't think this is a renegotiation thing. This is just a problem in the code that adjusts the bitrate when the resolution changes. We probably should be passing mMaxBitrate as the third param here: https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp#1401 Does that seem reasonable?
Flags: needinfo?(drno) → needinfo?(rjesup)
Whiteboard: [question 2016-10-24 to bwc] → [question 2016-10-27 to jesup]
(In reply to Byron Campen [:bwc] from comment #7) > I don't think this is a renegotiation thing. This is just a problem in the > code that adjusts the bitrate when the resolution changes. We probably > should be passing mMaxBitrate as the third param here: > > https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/ > media-conduit/VideoConduit.cpp#1401 > > Does that seem reasonable? mMaxBitrate is only the value of the pref for max bitrate; it has nothing to do with TIAS. Looking at the code, I see that tias appears to only be used to set the mEncodingConstraints.maxBr, and that is only used for H.264 in CodecConfigToWebRTCConfig() (and in a different way in simulcast, but that's not the tias rate).
Flags: needinfo?(rjesup)
Whiteboard: [question 2016-10-27 to jesup]
How about this?
Attachment #8805167 - Flags: review?(docfaraday)
Assignee: nobody → rjesup
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
TIAS gets passed down to the Video Conduit here as if it was passed as a constraint from JS: https://dxr.mozilla.org/mozilla-central/rev/3f4c3a3cabaf94958834d3a8935adfb4a887942d/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp#2028 So I'm not sure modifying stream.maxBitrate is going to help.
Comment on attachment 8805167 [details] [diff] [review] process maxBr/TIAS setting for all codecs, not just H264 Review of attachment 8805167 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp @@ +1969,5 @@ > cinst.minBitrate = mMinBitrate ? mMinBitrate : 200; > cinst.startBitrate = mStartBitrate ? mStartBitrate : 300; > cinst.targetBitrate = cinst.startBitrate; > + cinst.maxBitrate = 2000; > + if (codecInfo->mEncodingConstraints.maxBr > 0) { Simpler code for all of this would be: cinst.maxBitrate = MinIgnoreZero(2000, codecInfo->mEncodingConstraints.maxBr)/1000; cinst.maxBitrate = MinIgnoreZero(cinst.maxBitrate, mMaxBitrate);
Attachment #8805167 - Flags: review?(docfaraday)
> Simpler code for all of this would be: > > cinst.maxBitrate = MinIgnoreZero(2000, > codecInfo->mEncodingConstraints.maxBr)/1000; > cinst.maxBitrate = MinIgnoreZero(cinst.maxBitrate, mMaxBitrate); Yup, realized that shortly after uploading it (but was busy). (In reply to Nils Ohlmeier [:drno] from comment #10) > TIAS gets passed down to the Video Conduit here as if it was passed as a > constraint from JS: > > https://dxr.mozilla.org/mozilla-central/rev/ > 3f4c3a3cabaf94958834d3a8935adfb4a887942d/media/webrtc/signaling/src/media- > conduit/VideoConduit.cpp#2028 > > So I'm not sure modifying stream.maxBitrate is going to help. That's the max bitrate for each simulcast layer (which was why I commented about it being used a different way in simulcast).
More complete fix; maintains bitrate from maxBr/TIAS across input resolution changes and renegotiations/etc
Attachment #8805462 - Flags: review?(docfaraday)
Attachment #8805167 - Attachment is obsolete: true
VP8 was used when I was reproducing it.
Rank: 15
Priority: -- → P1
Comment on attachment 8805462 [details] [diff] [review] process maxBr/TIAS setting for all codecs, not just H264 Review of attachment 8805462 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/media-conduit/VideoConduit.h @@ +401,5 @@ > uint64_t mVideoLatencyAvg; > uint32_t mMinBitrate; > uint32_t mStartBitrate; > uint32_t mMaxBitrate; > + uint32_t mCurrentMaxBitrate; As far as I can tell, |mMaxBitrate| is solely used to represent the value of the media.peerconnection.video.max_bitrate pref, right? It seems to me that the pref, and the limits negotiated in SDP, have equal standing, so why have another variable that is used differently?
Attachment #8805462 - Flags: review?(docfaraday)
> > uint32_t mMaxBitrate; > > + uint32_t mCurrentMaxBitrate; > > As far as I can tell, |mMaxBitrate| is solely used to represent the value of > the media.peerconnection.video.max_bitrate pref, right? It seems to me that > the pref, and the limits negotiated in SDP, have equal standing, so why have > another variable that is used differently? mMaxBitrate avoids us having to access the pref again (which would also require being on MainThread). mCurrentMaxBitrate is a rollup of the logic contained in CodecConfigToWebRTCCodec() (which covers TIAS and maxbr), but only if it's the selected codec, and this simplify handling it in SelectBitrates(). I'm open to other solutions.
(In reply to Randell Jesup [:jesup] from comment #17) > > > uint32_t mMaxBitrate; > > > + uint32_t mCurrentMaxBitrate; > > > > As far as I can tell, |mMaxBitrate| is solely used to represent the value of > > the media.peerconnection.video.max_bitrate pref, right? It seems to me that > > the pref, and the limits negotiated in SDP, have equal standing, so why have > > another variable that is used differently? > > mMaxBitrate avoids us having to access the pref again (which would also > require being on MainThread). > > mCurrentMaxBitrate is a rollup of the logic contained in > CodecConfigToWebRTCCodec() (which covers TIAS and maxbr), but only if it's > the selected codec, and this simplify handling it in SelectBitrates(). I'm > open to other solutions. I think the code would be more readable if we renamed these variables then. Maybe mPrefMaxBitrate and mNegotiatedMaxBitrate.
renamed as suggested
Attachment #8805873 - Flags: review?(docfaraday)
Attachment #8805462 - Attachment is obsolete: true
Comment on attachment 8805873 [details] [diff] [review] process maxBr/TIAS setting for all codecs, not just H264 Review of attachment 8805873 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp @@ +296,5 @@ > if (!NS_WARN_IF(NS_FAILED(branch->GetIntPref("media.peerconnection.video.max_bitrate", &temp)))) > { > if (temp >= 0) { > + mPrefMaxBitrate = temp; > + mNegotiatedMaxBitrate = temp; Not what this variable is for, right? @@ +1185,5 @@ > + // Note: mNegotiatedMaxBitrate is the max transport bitrate - it applies to > + // a single codec encoding, but should also apply to the sum of all > + // simulcast layers in this encoding! So sum(layers.maxBitrate) <= > + // mNegotiatedMaxBitrate > + if (mNegotiatedMaxBitrate && mNegotiatedMaxBitrate > out_max) { We should set this based on the MinIgnoreZero of the negotiated max and the pref max, right? @@ +1973,5 @@ > 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(cinst.maxBitrate, mPrefMaxBitrate); // not mNegotiatedMaxBitrate! Ok, so we don't take the negotiated max into account here, but further down (for the simulcast streams) we call SelectBitrate, which _does_ take the negotiated max into account. Kinda weird...
Attachment #8805873 - Flags: review?(docfaraday)
I think byron is on PTO, so r?drno
Attachment #8812335 - Flags: review?(drno)
Attachment #8805873 - Attachment is obsolete: true
Comment on attachment 8812335 [details] [diff] [review] process maxBr/TIAS setting for all codecs, not just H264 Review of attachment 8812335 [details] [diff] [review]: ----------------------------------------------------------------- No r+ if it does not compile :-) ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp @@ +745,5 @@ > mSendingHeight = 0; > mSendingFramerate = video_codec.maxFramerate; > } > + // So we can comply with b=TIAS/b=AS/maxbr=X when input resolution changes > + mNegotiatedMaxBitrate = MinIgnoreZero(mPref,video_codec.maxBitrate; Did you mean |mPrefMaxBitrate| instead of |mPref|, which I believe is simply undefined? And this is also missing a closing bracket. So I doubt this compiles at all. @@ +746,5 @@ > mSendingFramerate = video_codec.maxFramerate; > } > + // So we can comply with b=TIAS/b=AS/maxbr=X when input resolution changes > + mNegotiatedMaxBitrate = MinIgnoreZero(mPref,video_codec.maxBitrate; > + WS @@ +1976,5 @@ > cinst.startBitrate = mStartBitrate ? mStartBitrate : 300; > cinst.targetBitrate = cinst.startBitrate; > + cinst.maxBitrate = MinIgnoreZero(2000U, codecInfo->mEncodingConstraints.maxBr)/1000; > + cinst.maxBitrate = MinIgnoreZero(cinst.maxBitrate, mPrefMaxBitrate); > + // not mNegotiatedMaxBitrate! cinst.maxBitrate is the max for the codec, which will be overridden I think the comment applies to the line above?! Can we please put it above the line where comments usually are.
Attachment #8812335 - Flags: review?(drno) → review-
Oops, forgot to save the editor buffer.... Compiles and nits addressed
Attachment #8812529 - Flags: review?(drno)
Attachment #8812335 - Attachment is obsolete: true
Comment on attachment 8812529 [details] [diff] [review] process maxBr/TIAS setting for all codecs, not just H264 Review of attachment 8812529 [details] [diff] [review]: ----------------------------------------------------------------- LGTM ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp @@ +752,5 @@ > mSendingFramerate = video_codec.maxFramerate; > } > + // So we can comply with b=TIAS/b=AS/maxbr=X when input resolution changes > + mNegotiatedMaxBitrate = MinIgnoreZero(mPrefMaxBitrate, video_codec.maxBitrate); > + Still WS here.
Attachment #8812529 - Flags: review?(drno) → review+
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6a57b36e6c17 process maxBr/TIAS setting for all codecs, not just H264 r=bwc
Backed out for assertions, e.g. in test_peerConnection_asymmetricIsolation.html: https://hg.mozilla.org/integration/mozilla-inbound/rev/787c66dd535fc380ced60ce4aaf68926c5c163dc Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6a57b36e6c173803831562bce0ad45ec597b8f25 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=39510582&repo=mozilla-inbound [task 2016-11-20T10:15:38.522173Z] 10:15:38 INFO - (ice/ERR) ICE(PC:1479636935541363 (id=44 url=http://mochi.test:8888/tests/dom/media/tests/mochitest/identity/test_peerConnection_asymmetricIsola): peer (PC:1479636935541363 (id=44 url=http://mochi.test:8888/tests/dom/media/tests/mochitest/identity/test_peerConnection_asymmetricIsola:default), stream(0-1479636935541363 (id=44 url=http://mochi.test:8888/tests/dom/media/tests/mochitest/identity/test_peerConnection_asymmetricIsola aLevel=0) tried to trickle ICE in inappropriate state 4 [task 2016-11-20T10:15:38.560648Z] 10:15:38 INFO - registering idp.js [task 2016-11-20T10:15:38.562024Z] 10:15:38 INFO - idp: validateAssertion({"username":"someone@test2.example.com","contents":"{\"fingerprint\":[{\"algorithm\":\"sha-256\",\"digest\":\"2C:9B:2D:5A:23:9F:49:0E:BD:64:0A:D5:51:8F:EC:1F:12:69:3B:47:F3:1A:A1:76:DC:D9:14:75:54:91:A3:3A\"}]}"}) [task 2016-11-20T10:15:38.563464Z] 10:15:38 INFO - idp: result={"identity":"someone@test2.example.com","contents":"{\"fingerprint\":[{\"algorithm\":\"sha-256\",\"digest\":\"2C:9B:2D:5A:23:9F:49:0E:BD:64:0A:D5:51:8F:EC:1F:12:69:3B:47:F3:1A:A1:76:DC:D9:14:75:54:91:A3:3A\"}]}"} [task 2016-11-20T10:15:39.661366Z] 10:15:39 INFO - [4109] WARNING: Cannot query channel count on a AudioSegment with no chunks.: '!mChunks.IsEmpty()', file /home/worker/workspace/build/src/dom/media/AudioSegment.h, line 370 [task 2016-11-20T10:15:39.668740Z] 10:15:39 INFO - [4109] WARNING: Cannot query channel count on a AudioSegment with no chunks.: '!mChunks.IsEmpty()', file /home/worker/workspace/build/src/dom/media/AudioSegment.h, line 370 [task 2016-11-20T10:15:39.777266Z] 10:15:39 INFO - Assertion failure: out_max <= mPrefMaxBitrate, at /home/worker/workspace/build/src/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:1193 [task 2016-11-20T10:16:09.351460Z] 10:16:09 INFO - #01: mozilla::WebrtcVideoConduit::ReconfigureSendCodec [media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:1420] [task 2016-11-20T10:16:09.351533Z] 10:16:09 INFO - [task 2016-11-20T10:16:09.352155Z] 10:16:09 INFO - #02: mozilla::media::LambdaRunnable<mozilla::WebrtcVideoConduit::SelectSendResolution(short unsigned int, short unsigned int, webrtc::I420VideoFrame*)::__lambda1>::Run [media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:1360] [task 2016-11-20T10:16:09.352204Z] 10:16:09 INFO - [task 2016-11-20T10:16:09.352259Z] 10:16:09 INFO - #03: nsThread::ProcessNextEvent [xpcom/threads/nsThread.cpp:1213] [task 2016-11-20T10:16:09.352291Z] 10:16:09 INFO - [task 2016-11-20T10:16:09.352339Z] 10:16:09 INFO - #04: NS_ProcessNextEvent [xpcom/glue/nsThreadUtils.cpp:361] [task 2016-11-20T10:16:09.352697Z] 10:16:09 INFO - [task 2016-11-20T10:16:09.352783Z] 10:16:09 INFO - #05: mozilla::ipc::MessagePump::Run [ipc/glue/MessagePump.cpp:97] [task 2016-11-20T10:16:09.353132Z] 10:16:09 INFO - [task 2016-11-20T10:16:09.353378Z] 10:16:09 INFO - #06: MessageLoop::RunInternal [ipc/chromium/src/base/message_loop.cc:233] [task 2016-11-20T10:16:09.354098Z] 10:16:09 INFO - [task 2016-11-20T10:16:09.354947Z] 10:16:09 INFO - #07: MessageLoop::Run [ipc/chromium/src/base/message_loop.cc:490] [task 2016-11-20T10:16:09.355648Z] 10:16:09 INFO - [task 2016-11-20T10:16:09.356418Z] 10:16:09 INFO - #08: nsBaseAppShell::Run [widget/nsBaseAppShell.cpp:158] [task 2016-11-20T10:16:09.357032Z] 10:16:09 INFO - [task 2016-11-20T10:16:09.357904Z] 10:16:09 INFO - #09: nsAppStartup::Run [toolkit/components/startup/nsAppStartup.cpp:284] [task 2016-11-20T10:16:09.358769Z] 10:16:09 INFO - [task 2016-11-20T10:16:09.359773Z] 10:16:09 INFO - #10: XREMain::XRE_mainRun [toolkit/xre/nsAppRunner.cpp:4468] [task 2016-11-20T10:16:09.360507Z] 10:16:09 INFO - [task 2016-11-20T10:16:09.361309Z] 10:16:09 INFO - #11: XREMain::XRE_main [toolkit/xre/nsAppRunner.cpp:4600] [task 2016-11-20T10:16:09.362051Z] 10:16:09 INFO - [task 2016-11-20T10:16:09.362886Z] 10:16:09 INFO - #12: XRE_main [toolkit/xre/nsAppRunner.cpp:4691] [task 2016-11-20T10:16:09.363624Z] 10:16:09 INFO - [task 2016-11-20T10:16:09.407736Z] 10:16:09 INFO - #13: do_main [browser/app/nsBrowserApp.cpp:298] [task 2016-11-20T10:16:09.407894Z] 10:16:09 INFO - [task 2016-11-20T10:16:09.407965Z] 10:16:09 INFO - #14: main [browser/app/nsBrowserApp.cpp:461] [task 2016-11-20T10:16:09.408352Z] 10:16:09 INFO - [task 2016-11-20T10:16:09.409040Z] 10:16:09 INFO - #15: libc.so.6 + 0x20830 [task 2016-11-20T10:16:09.409570Z] 10:16:09 INFO - [task 2016-11-20T10:16:09.410106Z] 10:16:09 INFO - #16: _start [task 2016-11-20T10:16:09.410620Z] 10:16:09 INFO -
Flags: needinfo?(rjesup)
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/abecacadf9d3 process maxBr/TIAS setting for all codecs, not just H264 r=bwc
https://hg.mozilla.org/mozilla-central/rev/abecacadf9d3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Flags: needinfo?(rjesup)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: