Closed Bug 1254187 Opened 5 years ago Closed 5 years ago

maxBitrate not working.

Categories

(Core :: WebRTC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: jib, Assigned: bwc)

References

Details

Attachments

(1 file, 3 obsolete files)

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 on attachment 8727489 [details]
MozReview Request: Bug 1254187 - fix sender.setParameters maxBitrate.

https://reviewboard.mozilla.org/r/38493/#review35081
Attachment #8727489 - Flags: review?(rjesup) → review+
(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...
Assignee: nobody → jib
Rank: 10
Priority: -- → P1
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/
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
Assignee: jib → docfaraday
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
webrtc.org is sad because vie_codec.maxBitrate is less than the sum of the max bitrates of the simulcast streams.
Check back on try.
Flags: needinfo?(docfaraday)
Maybe needs a rebase to run successfully on android?
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 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/
Attachment #8727489 - Attachment is obsolete: true
Flags: needinfo?(docfaraday)
Attachment #8730938 - Attachment is obsolete: true
Flags: needinfo?(docfaraday)
Attachment #8730938 - Flags: review?(docfaraday)
Flags: needinfo?(docfaraday)
Attachment #8731337 - Flags: review?(rjesup)
Attachment #8731338 - Flags: review?(rjesup)
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.
Attachment #8731338 - Flags: review?(rjesup) → review+
Comment on attachment 8731337 [details]
MozReview Request: Bug 1254187: Fix maxBitrate to respect simulcast. r=jesup

https://reviewboard.mozilla.org/r/40511/#review38123
Attachment #8731337 - Flags: review?(rjesup) → review+
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.
^
Flags: needinfo?(rjesup)
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.
Flags: needinfo?(rjesup)
Ok, got it. Let me see if I can do this without breaking the test.
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/
Attachment #8731337 - Attachment description: MozReview Request: Bug 1254187 - Part 1: fix sender.setParameters maxBitrate. → MozReview Request: Bug 1254187: Fix maxBitrate to respect simulcast. r=jesup
Attachment #8731338 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/e51b27b501f4
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Duplicate of this bug: 1280567
You need to log in before you can comment on or make changes to this bug.