bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Temporal scalability not working in 56/57

RESOLVED FIXED in Firefox 57

Status

()

Core
WebRTC: Audio/Video
P1
normal
Rank:
15
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 unaffected, firefox56 wontfix, firefox57 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

11 months ago
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
(Assignee)

Comment 1

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=71ece14acbdab68a5f0916ea8c537b9751249ad7
Assignee: nobody → rjesup
Rank: 15
status-firefox55: --- → unaffected
status-firefox56: --- → affected
status-firefox57: --- → affected
Priority: -- → P1
(Assignee)

Comment 2

11 months ago
Created attachment 8897046 [details] [diff] [review]
Populate temporal_layer_thresholds_bps so we'll send temporal layers in webrtc
Attachment #8897046 - Flags: review?(docfaraday)
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.
Attachment #8897046 - Flags: review?(docfaraday)
(Assignee)

Comment 4

11 months ago
> @@ +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).
(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.
(Assignee)

Updated

11 months ago
Blocks: 1390215
(Assignee)

Comment 6

11 months ago
Created attachment 8897052 [details] [diff] [review]
Populate temporal_layer_thresholds_bps so we'll send temporal layers in webrtc
Attachment #8897052 - Flags: review?(docfaraday)
(Assignee)

Updated

11 months ago
Attachment #8897046 - Attachment is obsolete: true
Attachment #8897052 - Flags: review?(docfaraday) → review+

Comment 7

11 months ago
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51b5264af174
Populate temporal_layer_thresholds_bps so we'll send temporal layers in webrtc r=bwc
(Assignee)

Comment 8

11 months ago
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

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/51b5264af174
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Comment 10

10 months ago
Too late for 56. Mass won't fix for 56.
status-firefox56: affected → wontfix
Depends on: 1402834
You need to log in before you can comment on or make changes to this bug.