TIAS bitrate limitation does not work when resolution changes

RESOLVED FIXED in Firefox 53

Status

()

Core
WebRTC
P1
normal
Rank:
15
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Alexander Abagian, Assigned: jesup, NeedInfo)

Tracking

49 Branch
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8798850 [details]
ff-wrtc-tias.jpg

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

Comment 1

2 years ago
The picture attached has been taken from a proprietary client, working with MacOS Firefox 49.01 WebRTC client via SFU.
(Reporter)

Comment 2

2 years ago
Created attachment 8798860 [details]
ff-wrtc-tias-2.png
(Reporter)

Comment 3

2 years ago
If resolution changes to 640x480, everything is OK. The problems happen only at 352x288 resolution.
Component: Untriaged → WebRTC
Product: Firefox → Core
(Assignee)

Comment 4

2 years ago
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)

Comment 5

2 years ago
When renegotiation happens, we just use whatever was negotiated.
Flags: needinfo?(docfaraday)
(Reporter)

Comment 6

2 years ago
Did you reproduce it ?
Whiteboard: [question 2016-10-24 to bwc]

Comment 7

2 years ago
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)

Updated

2 years ago
Whiteboard: [question 2016-10-24 to bwc] → [question 2016-10-27 to jesup]
(Assignee)

Comment 8

2 years ago
(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).
(Assignee)

Updated

2 years ago
Flags: needinfo?(rjesup)
(Assignee)

Updated

2 years ago
Whiteboard: [question 2016-10-27 to jesup]
(Assignee)

Comment 9

2 years ago
Created attachment 8805167 [details] [diff] [review]
process maxBr/TIAS setting for all codecs, not just H264

How about this?
Attachment #8805167 - Flags: review?(docfaraday)
(Assignee)

Updated

2 years ago
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 11

2 years ago
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)
(Assignee)

Comment 12

2 years ago
> 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).
(Assignee)

Comment 13

2 years ago
Created attachment 8805462 [details] [diff] [review]
process maxBr/TIAS setting for all codecs, not just H264

More complete fix; maintains bitrate from maxBr/TIAS across input resolution changes and renegotiations/etc
Attachment #8805462 - Flags: review?(docfaraday)
(Assignee)

Updated

2 years ago
Attachment #8805167 - Attachment is obsolete: true
(Reporter)

Comment 14

2 years ago
VP8 was used when I was reproducing it.

Updated

2 years ago
Rank: 15
Priority: -- → P1

Comment 16

2 years ago
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)
(Assignee)

Comment 17

2 years ago
> >    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.

Comment 18

2 years ago
(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.
(Assignee)

Comment 19

2 years ago
Created attachment 8805873 [details] [diff] [review]
process maxBr/TIAS setting for all codecs, not just H264

renamed as suggested
Attachment #8805873 - Flags: review?(docfaraday)
(Assignee)

Updated

2 years ago
Attachment #8805462 - Attachment is obsolete: true

Comment 20

2 years ago
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)
(Assignee)

Comment 21

2 years ago
Created attachment 8812335 [details] [diff] [review]
process maxBr/TIAS setting for all codecs, not just H264

I think byron is on PTO, so r?drno
Attachment #8812335 - Flags: review?(drno)
(Assignee)

Updated

2 years ago
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-
(Assignee)

Comment 23

2 years ago
Created attachment 8812529 [details] [diff] [review]
process maxBr/TIAS setting for all codecs, not just H264

Oops, forgot to save the editor buffer.... Compiles and nits addressed
Attachment #8812529 - Flags: review?(drno)
(Assignee)

Updated

2 years ago
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+

Comment 25

2 years ago
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)

Comment 27

2 years ago
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

Comment 28

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/abecacadf9d3
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.