1308481 - TIAS bitrate limitation does not work when resolution changes

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Alexander Abagian]

Attachment: 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).

Comment 1

[Alexander Abagian]

The picture attached has been taken from a proprietary client, working with MacOS Firefox 49.01 WebRTC client via SFU.

Comment 2

[Alexander Abagian]

Attachment: ff-wrtc-tias-2.png

Comment 3

[Alexander Abagian]

If resolution changes to 640x480, everything is OK. The problems happen only at 352x288 resolution.

Comment 4

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

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.

Comment 5

[Byron Campen [:bwc]]

When renegotiation happens, we just use whatever was negotiated.

Comment 6

[Alexander Abagian]

Did you reproduce it ?

Comment 7

[Byron Campen [: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?

Comment 8

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

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

Comment 9

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

Attachment: process maxBr/TIAS setting for all codecs, not just H264

How about this?

Comment 10

[Nils Ohlmeier [:drno]]

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

[Byron Campen [:bwc]]

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);

Comment 12

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

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

Comment 13

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

Attachment: 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

Comment 14

[Alexander Abagian]

VP8 was used when I was reproducing it.

Comment 15

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

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

Comment 16

[Byron Campen [:bwc]]

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?

Comment 17

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

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

[Byron Campen [:bwc]]

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

Comment 19

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

Attachment: process maxBr/TIAS setting for all codecs, not just H264

renamed as suggested

Comment 20

[Byron Campen [:bwc]]

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

Comment 21

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

Attachment: process maxBr/TIAS setting for all codecs, not just H264

I think byron is on PTO, so r?drno

Comment 22

[Nils Ohlmeier [:drno]]

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.

Comment 23

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

Attachment: process maxBr/TIAS setting for all codecs, not just H264

Oops, forgot to save the editor buffer.... Compiles and nits addressed

Comment 24

[Nils Ohlmeier [:drno]]

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.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a57b36e6c17
process maxBr/TIAS setting for all codecs, not just H264 r=bwc

Comment 26

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

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":"[e-mail]","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":"[e-mail]","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 -

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/abecacadf9d3
process maxBr/TIAS setting for all codecs, not just H264 r=bwc

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/abecacadf9d3