1429219 - Enforce providing simulcast encodings with enough bits to avoid encoder Init failure

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

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

VP8 gets upset if you use temporal layers and don't specify a number of bits for each; but if the initial bitrate available is too low (such as when you switch to a screenshare when at a low bitrate), the largest simulcast screenshare layer(s) may not get allocated bits, and thus the ts_target_bitrates may be 0, leading to InitEncode() failure.

This ensures that the encoder init won't fail due to a simulcast encoding getting 0 bits.  Note that future allocations will all happen "normally"; this is only an issue with Initing the encoder.

For example, if 500kbps is available at the point of switching, there may be enough for the 320x240 and 640x480 layers to init, but not the 1280x960 layer, and stream_bitrates[] might be [200, 300, 0].  Without this patch InitEncode for the last layer will fail.

Comment 1

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

Attachment: Ensure VP8 simulcast with temporal layers won't fail if there aren't enough bits

Comment 2

[Nils Ohlmeier [:drno]]

Comment on attachment 8941204 [details] [diff] [review]
Ensure VP8 simulcast with temporal layers won't fail if there aren't enough bits

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

LGTM

Comment 3

[Nils Ohlmeier [:drno]]

Also here: candidate for upstreaming?

Comment 4

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

Definitely should open an upstream bug for this; I strongly suspect they'll want to solve it in a different way as part of the bug they're currently trying to finish that redoes a lot of the simulcast/temporal-layer code:
https://bugs.chromium.org/p/webrtc/issues/detail?id=4172

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef313ab76a82
Ensure VP8 simulcast with temporal layers won't fail if there aren't enough bits r=drno

Comment 6

[Andrei Ciure[:aciure]]

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

Comment 7

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Gerry, Ryan, this fix needs to be included in RC1. Could you please approve and make sure RC1 is built with this fix in? Thanks! Tracking as a blocker to be sure this doesn't fall off our radar.

Comment 8

[Gerry Chang [:gchang]]

Comment on attachment 8941204 [details] [diff] [review]
Ensure VP8 simulcast with temporal layers won't fail if there aren't enough bits

Take this in RC. Beta58+.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2a4cb360a9f6 (FIREFOX_58b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/d4eaca183f2f

Comment 10

[Dan Minor [:dminor]]

Filed https://bugs.chromium.org/p/webrtc/issues/detail?id=8753 upstream.