1390202 - Temporal scalability not working in 56/57

Status: RESOLVED FIXED

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

Description

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

Upstream webrtc.org code uses temporal_layer_thresholds_bps (in particular the array size) to decide how many temporal layers to use.  Note it ignores the values *in* the array, for some reason.

Reported by the meetecho guys on dev.media

Comment 1

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

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

Comment 2

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

Attachment: Populate temporal_layer_thresholds_bps so we'll send temporal layers in webrtc

Comment 3

[Byron Campen [:bwc]]

Comment on attachment 8897046 [details] [diff] [review]
Populate temporal_layer_thresholds_bps so we'll send temporal layers in webrtc

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

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +619,5 @@
>  
>      simulcastEncoding = mConduit->mCurSendCodecConfig->mSimulcastEncodings[idx];
>      MOZ_ASSERT(simulcastEncoding.constraints.scaleDownBy >= 1.0);
>  
>      // leave vector temporal_layer_thresholds_bps empty

Update this comment?

@@ +624,5 @@
> +    if (config.number_of_streams > 1) {
> +      // Oddly, though this is a 'bps' array, nothing really looks at the
> +      // values, just the size of the array to know the number of temporal
> +      // layers.
> +      video_stream.temporal_layer_thresholds_bps.resize(2);

How are we going to ensure we remember to update this if/when webrtc.org starts paying attention to the values? We don't want this to regress again.

@@ +628,5 @@
> +      video_stream.temporal_layer_thresholds_bps.resize(2);
> +      // XXX Note: in simulcast.cc in upstream code, the array value is
> +      // 3(-1) for all streams, though it's in an array, except for screencasts,
> +      // which use 1 (i.e 2 layers).
> +      // XXX Bug XXXXX investigate using more of

Bug number, of course.

Comment 4

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

> @@ +624,5 @@
> > +    if (config.number_of_streams > 1) {
> > +      // Oddly, though this is a 'bps' array, nothing really looks at the
> > +      // values, just the size of the array to know the number of temporal
> > +      // layers.
> > +      video_stream.temporal_layer_thresholds_bps.resize(2);
> 
> How are we going to ensure we remember to update this if/when webrtc.org
> starts paying attention to the values? We don't want this to regress again.

There's no easy way to do so, unless maybe have a bug blocking the next import (and the next if needed, etc).

Comment 5

[Byron Campen [:bwc]]

(In reply to Randell Jesup [:jesup] from comment #4)
> > @@ +624,5 @@
> > > +    if (config.number_of_streams > 1) {
> > > +      // Oddly, though this is a 'bps' array, nothing really looks at the
> > > +      // values, just the size of the array to know the number of temporal
> > > +      // layers.
> > > +      video_stream.temporal_layer_thresholds_bps.resize(2);
> > 
> > How are we going to ensure we remember to update this if/when webrtc.org
> > starts paying attention to the values? We don't want this to regress again.
> 
> There's no easy way to do so, unless maybe have a bug blocking the next
> import (and the next if needed, etc).

Let's do that. It would be _really_ nice to be able to test this, though.

Comment 6

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

Attachment: Populate temporal_layer_thresholds_bps so we'll send temporal layers in webrtc

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51b5264af174
Populate temporal_layer_thresholds_bps so we'll send temporal layers in webrtc r=bwc

Comment 8

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

Tested with https://janus.conf.meetecho.com/echotest?simulcast=true - works great (select tl1 or tl0 to verify - video should be slower/choppier)

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/51b5264af174

Comment 10

[Gerry Chang [:gchang]]

Too late for 56. Mass won't fix for 56.