1746477 - We aren't using VideoStreamFactoryInterface in quite the intended way

Status: NEW

(Core :: WebRTC: Audio/Video, task, P3)

Description

[Byron Campen [:bwc]]

VideoStreamFactoryInterface is intended to be used less like a factory, and more like a mutator. libwebrtc expects VideoEncoderConfig::simulcast_layers to be populated before the "factory" is invoked (https://bugzilla.mozilla.org/show_bug.cgi?id=1706917#c23), and expects this "factory" to iterate over this array and make modifications after some simulcast alignment fixup is applied. If this array is not pre-populated, the simulcast alignment fixup never occurs, which means that it would have to be solely the responsibility of the factory, which I'm not sure we do.

We would probably need to do more of the VideoStream creation work up front in the VideoConduit to take advantage of the simulcast alignment fixup, simplify our "factory" code significantly (maybe even to the point of making it a passthrough), and give this not-really-a-factory a more accurate name.

Comment 1

[Byron Campen [:bwc]]

Thoughts?

Comment 2

[Andreas Pehrson [:pehrsons]]

Our code around this is not the prettiest, so if it can be simplified, great.

It seems like upstream looks at three attributes on those simulcast_layers:

• size (number of layers)
• active (per layer)
• scaleDownBy (per layer)

These are all negotiation details so they seem like something we could easily write.

But I don't think we can make our factory a pass-through thing because an output of the factory is the resolution of each layer, and that depends on the input resolution, which is not known at the time of negotiation.

The alignment around scaleDownBy is new with the update. We have been rolling our own thing as a workaround in the past. We have decent tests here so I don't think the priority to switch over to upstream's logic is awfully high.

The number of layers here seem to be used to ensure the number of spatial layers don't exceed the number of simulcast layers. But we don't support SVC and don't set anything other than 1 spatial layer. So might be a blocker for that work but probably not of much impact now.

The active attribute we never set so it's always true. I guess this will come from the setParameters work.

So to summarize I think we are good at the moment -- this is not causing any breakages. Just adding what is known at negotiation time should be low-hanging fruit and will unblock some feature work (svc, setParameters). The layer scaling stuff, i.e. our factory impl I consider technical debt. It could perhaps be made simpler now. Reworking it may also be justified by a perf effort, because there's a potential video copy (scaling down) in this path.