webrtc-pc icon indicating copy to clipboard operation
webrtc-pc copied to clipboard

Inconsistent initialization of scaleResolutionDownBy

Open jan-ivar opened this issue 3 years ago • 3 comments

addTransceiver says: "7. If the scaleResolutionDownBy attributes of sendEncodings are still undefined, initialize [them] ... 9. Create an RTCRtpSender with track, kind, streams and sendEncodings ..."

Create an RTCRtpSender says: "If sendEncodings is given as input to this algorithm, and is non-empty, set the [[SendEncodings]] slot to sendEncodings. Otherwise, set it to a list containing a single RTCRtpEncodingParameters with active set to true."

This means that (according to spec) the output of sender.getParameters().encodings[0] will differ in the following cases:

pc.addTrack(track)                                    // {active: true}
pc.addTransceiver(track)                              // {active: true}
pc.addTransceiver(track, {sendEncodings: []})         // {active: true}
pc.addTransceiver(track, {sendEncodings: [{}]})       // {active: true, scaleResolutionDownBy: 1}
pc.addTransceiver(track, {sendEncodings: [{rid: 1}]}) // {active: true, scaleResolutionDownBy: 1}

A quick test (working around #2729) shows Chrome never initializes scaleResolutionDownBy, while Safari always does.

I think we're better off always initializing scaleResolutionDownBy, or we need to fix setParameters: "Verify that each encoding in encodings has a scaleResolutionDownBy member whose value is greater than or equal to 1.0. If one of the scaleResolutionDownBy values does not meet this requirement, return a promise rejected with a newly created RangeError."

jan-ivar avatar May 06 '22 23:05 jan-ivar

It would be tempting to default scaleResolutionDownBy to 1 in the IDL, but that can't be done because the sendEncodings field is defined for both audio and video (although I'm unaware of anyone using it for audio).

The initialization rule given in https://w3c.github.io/webrtc-pc/#dom-rtcrtpencodingparameters-scaleresolutiondownby is consistent with always initializing it to 1 when there is a single layer.

alvestrand avatar May 08 '22 18:05 alvestrand

It would be tempting to default scaleResolutionDownBy to 1 in the IDL, but that can't be done because the sendEncodings field is defined for both audio and video (although I'm unaware of anyone using it for audio).

The initialization rule given in https://w3c.github.io/webrtc-pc/#dom-rtcrtpencodingparameters-scaleresolutiondownby is consistent with always initializing it to 1 when there is a single layer.

On the subject of audio; what should we be doing when JS passes scaleResolutionDownBy to addTransceiver('audio')? TypeError? The spec clearly says that it is invalid to use scaleResolutionDownBy in the audio case.

Also, the steps in addTransceiver require auto-filling scaleResolutionDownBy regardless of the media kind (either the "fill with 1" case, or the "powers of 2" case). This is almost certainly not what we want. That might also be a TypeError?

docfaraday avatar May 10 '22 22:05 docfaraday

Another thing; it probably doesn't make much sense to throw RangeError when scaleResolutionDownBy is undefined in setParameters. If we want to throw something in this case, I think InvalidModificationError is appropriate.

docfaraday avatar May 11 '22 14:05 docfaraday

Another thing; it probably doesn't make much sense to throw RangeError when scaleResolutionDownBy is undefined in setParameters. If we want to throw something in this case, I think InvalidModificationError is appropriate.

I don't think we should throw. Safari (the only browser that defaults to scaleResolutionDownBy: 1 correctly) treats absence of scaleResolutionDownBy in setParameters as scaleResolutionDownBy: 1.

This seems correct as it's what would have happened had we been able to specify a default value for scaleResolutionDownBy in WebIDL like we do for the other parameters, as @alvestrand mentions in https://github.com/w3c/webrtc-pc/issues/2730#issuecomment-1120469775.

jan-ivar avatar Sep 23 '22 21:09 jan-ivar

... had we been able to specify a default value for scaleResolutionDownBy in WebIDL ...

Hmm, except the overall default in addTransceiver isn't all 1s, but a "smart" 2^(length of sendEncodings - encoding index - 1) if all are untouched, but nobody implements that yet it seems?

So should the following produce 1,1,1 or 4,2,1?

const pc = new RTCPeerConnection();
const tc = pc.addTransceiver("video", {sendEncodings: [{rid: 1}, {rid: 2}, {rid: 3}]);

const params = tc.sender.getParameters();
delete params.encodings[0].scaleResolutionDownBy;
delete params.encodings[1].scaleResolutionDownBy;
delete params.encodings[2].scaleResolutionDownBy;
await tc.sender.setParameters(params);

jan-ivar avatar Sep 23 '22 21:09 jan-ivar

So should the following produce 1,1,1 or 4,2,1?

That's an interesting question. It gets more interesting when you ask what happens when that setParameters occurs right after a sRD(initial simulcast offer), which is then rolled back.

docfaraday avatar Sep 23 '22 21:09 docfaraday

Or, what happens if only some of the scaleResolutionDownBy are unset?

docfaraday avatar Sep 23 '22 21:09 docfaraday

I think my inclination is to just set to 1 regardless. Too many wonky corner cases pop out if you do anything but that. Or, I guess we could throw an error. Seems weird that anyone would want to do something like that.

docfaraday avatar Sep 23 '22 21:09 docfaraday

I got this comment out-of-band:

have you considered changing scaleResolutionDownBy to a field that is ignored for audio (instead of being an error)? That would mean you 1) could set a default in WebIDL, and 2) wouldn't need to come up with an error for passing it to audio.

This seems reasonable to me, as dictionaries are generally only examined for expected inputs. We'd have to not just ignore it but remove it, or it'd show up in getParameters() which would be undesirable, but I think this is a winner. Updating PR.

jan-ivar avatar Sep 26 '22 22:09 jan-ivar

It gets more interesting when you ask what happens when that setParameters occurs right after a sRD(initial simulcast offer), which is then rolled back.

I think that's https://github.com/w3c/webrtc-pc/issues/2764 which seems orthogonal to defaults.

jan-ivar avatar Sep 26 '22 22:09 jan-ivar

I think deleting parameters is odd enough that 1,1,1 seems fine.

jan-ivar avatar Sep 26 '22 22:09 jan-ivar

Note we still can't set a default in WebIDL, because we need to detect absence to apply the smart (4,2,1) default.

jan-ivar avatar Sep 26 '22 23:09 jan-ivar