1159489 - Make WebRTC video min/max bitrates dynamic and dependent on resolution and framerate

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

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

We shouldn't use 200Kbps to 2Mbps as the limits for all video; for low-res inputs (or even really VGA) this is overkill; and the lower-limit is at the near-perfect-quality level for QCIF/QQVGA video.  Similarly, you don't want to send 1920x1080 30fps video at a mere 2Mbps, let alone anything far below it.

Bandwidth adaptation limits should primarily depend on resolution, and secondarily depend on framerate, and also depend (in the future, for things like VP9) on codec.

These should be adaptive to changing input resolution and framerate.

Comment 1

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

Attachment: WIP patch to make bitrate limits for webrtc video depend on input resolution and framerate

WIP; however it seems to not detect framerate changes because of a problem in the stats code.  Should move the trigger out of the stats callback anyways

Comment 2

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

Attachment: Make bitrate limits for webrtc video depend on input resolution and framerate

trivial changes - added debug

Comment 3

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

Attachment: Fix VideoCodecStats to allow for collecting encoder and decoder stats

I found why the FPS wasn't getting updated for the encoder - the VideoCodecStats object assumed it could only be used from either an Encode side or a Decode side, but now VideoConduits are encode/decode, so it never registered for EncoderStats callbacks

Comment 4

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

Comment on attachment 8599104 [details] [diff] [review]
Make bitrate limits for webrtc video depend on input resolution and framerate

Review of attachment 8599104 [details] [diff] [review]:
-----------------------------------------------------------------

I'll remove the trailing spaces, and the XXX comment about locking

f? to mreavy & derf, especially to review the algorithmic choices around selection of rates and the hard-coded values

I should consider adding a followup bug to move the triggering code for FPS changes off of the stats callbacks.

Comment 5

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

Comment on attachment 8599107 [details] [diff] [review]
Fix VideoCodecStats to allow for collecting encoder and decoder stats

Review of attachment 8599107 [details] [diff] [review]:
-----------------------------------------------------------------

Need to ensure that these changes are safe and don't mess anything up.

Comment 6

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

Comment on attachment 8599107 [details] [diff] [review]
Fix VideoCodecStats to allow for collecting encoder and decoder stats

Moved to a separate bug

Comment 7

[Maire Reavy [:mreavy]]

Comment on attachment 8599104 [details] [diff] [review]
Make bitrate limits for webrtc video depend on input resolution and framerate

Review of attachment 8599104 [details] [diff] [review]:
-----------------------------------------------------------------

I think the algorithm you're using is fine.  I'm less confident in the numbers. That is to say, they look fine (based on my experience) but I have no proof they are fine. We could spend a lot of time trying to work through the math using various inputs with not much bang for our buck. I am confident that landing this patch is a move in the right direction.  So, I'd like to land this in Nightly and ask for feedback from our QA folks and from Fred Dixon and his team -- and show them how to play with the numbers to affect the experience.  I'd like to get this testing and feedback done by or before the first week in June.  Fx 41 uplifts from Nightly to Aurora on June 29th.

Comment 8

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

Attachment: Atomic<double/float> isn't supported.. (interdiff)

Comment 9

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

Attachment: WebRTC bitrate limits for video depend on input resolution and framerate

full patch

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/340c59ea962d

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/340c59ea962d