1254187 - maxBitrate not working.

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

When I test this in https://jsfiddle.net/kv14z7r6/ I'm seeing maxBitrate being adhered to in practice (e.g. set { maxBitRate: 200000, scaleResolutionDownBy: 1 }

I think the functionality slipped away between some revisions during review in bug 1244913.

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: MozReview Request: Bug 1254187 - fix sender.setParameters maxBitrate.

Review commit: https://reviewboard.mozilla.org/r/38493/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38493/

Comment 2

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

Comment on attachment 8727489 [details]
MozReview Request: Bug 1254187 - fix sender.setParameters maxBitrate.

https://reviewboard.mozilla.org/r/38493/#review35081

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #0)
> I'm seeing maxBitrate being adhered to in practice

I meant of course NOT being adhered to...

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8727489 [details]
MozReview Request: Bug 1254187 - fix sender.setParameters maxBitrate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38493/diff/1-2/

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: MozReview Request: Bug 1254187 - fix maxBitrate to respect simulcast.

Review commit: https://reviewboard.mozilla.org/r/40241/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40241/

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I've uploaded a new WIP patch, since I fear the previous patch did not do the correct thing in the case of simulcast. Unfortunately, it causes test_peerConnection_simulcastOffer.html to perma-fail.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4da4c48737dd&selectedJob=17895142

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I also wrote this fiddle https://jsfiddle.net/ejbb0egu/ to debug simulcast.

Fiddle requires locally removing [ChromeOnly] from
http://mxr.mozilla.org/mozilla-central/source/dom/webidl/RTCPeerConnection.webidl?rev=d034fcc08831#140

Comment 8

[Byron Campen [:bwc]]

webrtc.org is sad because vie_codec.maxBitrate is less than the sum of the max bitrates of the simulcast streams.

Comment 9

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1254187: Fix maxBitrate to respect simulcast. r=jesup

Review commit: https://reviewboard.mozilla.org/r/40511/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40511/

Comment 10

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1254187 - Part 2: fix maxBitrate to respect simulcast.

Review commit: https://reviewboard.mozilla.org/r/40513/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40513/

Comment 11

[Byron Campen [:bwc]]

Check back on try.

Comment 12

[Byron Campen [:bwc]]

Maybe needs a rebase to run successfully on android?

Comment 13

[Byron Campen [:bwc]]

Comment on attachment 8731337 [details]
MozReview Request: Bug 1254187: Fix maxBitrate to respect simulcast. r=jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40511/diff/1-2/

Comment 14

[Byron Campen [:bwc]]

Comment on attachment 8731338 [details]
MozReview Request: Bug 1254187 - Part 2: fix maxBitrate to respect simulcast.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40513/diff/1-2/

Comment 15

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

Comment on attachment 8731338 [details]
MozReview Request: Bug 1254187 - Part 2: fix maxBitrate to respect simulcast.

https://reviewboard.mozilla.org/r/40513/#review38121

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:1340
(Diff revision 2)
>    if (vie_codec.numberOfSimulcastStreams != 0) {
> -    vie_codec.startBitrate /= vie_codec.numberOfSimulcastStreams;
> +    vie_codec.minBitrate = std::max(totalMinBitrate, vie_codec.minBitrate);
> +    vie_codec.maxBitrate = std::min(totalMaxBitrate, vie_codec.maxBitrate);
> +    vie_codec.startBitrate = std::max(vie_codec.minBitrate,
> +                                      std::min(totalStartBitrate,
> +                                               vie_codec.maxBitrate));

The startBitRate is hard to define for simulcast since it uses a single codec structure for all of them.  Each stream would want a start bitrate appropriate to that stream.  The start rate should not be the addition of them; it should be the min of all the startrates (and the codec startrate).  That's not ideal, but perhaps it's better than having small streams starting at max-bitrate.

Please file a followup to resolve this.

Comment 16

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

Comment on attachment 8731337 [details]
MozReview Request: Bug 1254187: Fix maxBitrate to respect simulcast. r=jesup

https://reviewboard.mozilla.org/r/40511/#review38123

Comment 17

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/40513/#review38121

> The startBitRate is hard to define for simulcast since it uses a single codec structure for all of them.  Each stream would want a start bitrate appropriate to that stream.  The start rate should not be the addition of them; it should be the min of all the startrates (and the codec startrate).  That's not ideal, but perhaps it's better than having small streams starting at max-bitrate.
> 
> Please file a followup to resolve this.

I don't understand why the start bitrate for the codec as a whole should be the minimum of all. I don't even understand why these parameters on the vie_codec should matter at all if the params are specified on each simulcast stream. I really don't get what the meaning of the vie_codec is when we have simulcast configured. I can change the logic here around the start bitrate and see if webrtc.org complains, but from what I can tell, webrtc.org interprets it as totals of all the simulcast streams, which makes taking the minimum strange to me.

Comment 18

[Byron Campen [:bwc]]

^

Comment 19

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

Well, the start bitrate really should be dependent on the resolution.  Generally, the start bitrate should be 1.5-2x the minimum rate, and the minimum rate should depend on the resolution.  The problem here is that for simulcast, you want to provide bits to the smallest layers first, and then only turn on higher layers when there are "enough" bits to make use of them.  Also, you don't want to turn it on, and then immediately have to turn it off the the available bits drops, so again you don't want it *too* low.

So: minbitrate *of a stream* should be min*1.x (whatever we pull from the resolution-based table).  We should only enable a higher-resolution stream when available bits not used by smaller streams >= start of larger stream, and turn it off if it's stuck at it's min too long or the estimated bits goes too far below the min.  (I'm going to assume for the minute we don't play with framerate here; when we hit min (or get near it) we probably want to cut the framerate to keep within available bits for a while, and then turn it off if we have to cut framerate too much/too long, instead of violating the estimate.)

Now, that's *should*.  The vie_codec structure may not make this possible.  If it doesn't... what is Chrome doing with it?  Or should we tweak things?  Ideally, we can control the codec params of each stream independently at some level; perhaps we leave vie_codec as a "master" all-streams setting, and everything else is handled dynamically inside the code - the problem may be that this interacts poorly with how we shoehorned in the resolution-bandwidth adaptation.  We might need to move that down into the webrtc.org code so we can avoid messing with vie_codec settings.

In that case, vie_codec min would be the minimum we would *ever* use for *any* stream using that codec, or (more dynamically) the minimum we'd use for the smallest simulcast stream configured (at min framerate).

I may have led you down the wrong path on setting these, depending on some of these interactions (including with our resolution-checking code).  It is a bit brain-twisting to think this through in all scenarios for simulcast.  layered encodings are actually much simpler here.

Comment 20

[Byron Campen [:bwc]]

Ok, got it. Let me see if I can do this without breaking the test.

Comment 21

[Byron Campen [:bwc]]

Comment on attachment 8731337 [details]
MozReview Request: Bug 1254187: Fix maxBitrate to respect simulcast. r=jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40511/diff/2-3/

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e51b27b501f4

Comment 23

[Carsten Book [:Tomcat]]

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