Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scaleResolutionDownBy validation steps are inconsistent, browser interop issues #2873

Open
henbos opened this issue May 15, 2023 · 5 comments

Comments

@henbos
Copy link
Contributor

henbos commented May 15, 2023

The validation steps are different for which scaling factors to use if scaleResolutionDownBy is not specified. When creating a transceiver we do 4:2:1 but when setting parameters we do 1:1:1.

  • In addTransceiver sendEncodings validation steps we say: If kind is "video" and none of the encodings contain a scaleResolutionDownBy member, then for each encoding, add a scaleResolutionDownBy member with the value 2^(length of sendEncodings - encoding index - 1). This results in smaller-to-larger resolutions where the last encoding has no scaling applied to it, e.g. 4:2:1 if the length is 3.
  • In Set the session description, where remote SDP is processed and encodings are being configured for a "suitable transceiver", we say: If proposedSendEncodings is non-empty, set each encoding's scaleResolutionDownBy to 2^(length of proposedSendEncodings - encoding index - 1).
  • In setParameters we say: If transceiver kind is "video", then for each encoding in encodings that doesn't contain a scaleResolutionDownBy member, add a scaleResolutionDownBy member with the value 1.0.

What the spec describes in the setParameters case (1:1:1), but not in the other cases (4:2:1) seems undesirable, can we be consistent and always do 4:2:1?

This inconsistency is causing different issues on different browsers, we need to align and I would prefer if we all align on 4:2:1 to avoid weird edge cases.

What happens today:

  • In Chrome, clearing scaleResolutionDownBy in setParameters() triggers 4:2:1 but on Safari and Firefox it triggers 1:1:1. Repro that starts in 4:2:1 but you can trigger this by manually clearing scaleResolutionDownBy and pressing "Set Parameters" button. Chrome's behavior goes against the spec but I believe makes more sense.
  • In Safari, creating a transceiver without specifying scaleResolutionDownBy triggers 1:1:1 (repro). This goes against the spec (at creation time spec says to do 4:2:1).
  • In Firefox, creating a transceiver without specifying scaleResolutionDownBy triggers 4:2:1 (repro). This is what the spec says, but if you press "Set Parameters" without touching scaleResolutionDownBy (meaning it is still undefined) you get 1:1:1. You could argue this is an issue with the test page rather than the browser since getParameters() does return 4:2:1 and a "get+setParameters" dance would have applied 4:2:1 correctly, but the issue remains that the parameters changed behind our backs between addTransciever and getParameters. The last thing that was set was not what you obtained in getParameters.
@henbos
Copy link
Contributor Author

henbos commented May 15, 2023

In Chrome, creating encodings with undefined scaleResolutionDownBy (repro) results in 4:2:1 which is the same as in Firefox, but if you do sender.getParameters() then Chrome does not return any value for scaleResolutionDownBy while Firefox does return 4, 2 and 1.

In Chrome, whether or not you specify scaleResolutionDownBy affects whether you get VP9 simulcast or backwards-compat VP9 legacy SVC mode (PSA), which may skew apps' preferences towards not specifying scaleResolutionDownBy in the medium term

@jan-ivar
Copy link
Member

When creating a transceiver we do 4:2:1 but when setting parameters we do 1:1:1.

If I addTransceiver, default is 4:2:1 → getParameters is 4:2:1 → setParameters will set 4:2:1 unless I change it.
If I setRemoteDescription, default is 4:2:1 → getParameters is 4:2:1 → setParameters will set 4:2:1 unless I change it.

I would characterize that as consistent.

Are we only talking about an app that manipulates all layers to undefined on purpose? E.g.

const params = sender.getParameters();
for (encoding of params.encodings) {
  delete encoding.scaleResolutionDownBy;
}
await sender.setParameters(params)

Why would anyone...

In Chrome, whether or not you specify scaleResolutionDownBy affects whether you get VP9 simulcast or backwards-compat VP9 legacy SVC mode (PSA), which may skew apps' preferences towards not specifying scaleResolutionDownBy in the medium term

I see.

What behavior are you proposing? That the code I show above produce 4:2:1 for three layers, and 2:1 for two?

If so, what should the following produce?

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

Would it depend on how many encodings there are or what encodings[2] is? E.g. 1:1:1 if three, or 2:1 for two?

From my recollection of discussions on the existing behavior, we stopped short of applying magic defaults once parameters have been fiddled with, since they arguably stop being POLA at that point.

@henbos
Copy link
Contributor Author

henbos commented May 22, 2023

If so, what should the following produce? [deleting some but not all encodings' scaleResolutionDownBy]

In the addTransceiver case, 4:2:1 is only applied if none of the encodings have a scaleResolutionDownBy value. So I don't think we should apply 4:2:1 logic if there still exists an encoding (encodings[2]) that have a scaleResolutionDownBy value.

We'd still need to agree on what to do if we're trying to apply an undefined scale... not sure if this should throw, apply 1, or apply whatever was the scale previously. In practise I think apps either apply all or none of the scales, but throwing an exception has backwards compat risks that we may want to avoid. I'd check what browsers do today if we can find a pattern.

const params = sender.getParameters();
for (encoding of params.encodings) {
  delete encoding.scaleResolutionDownBy;
}
await sender.setParameters(params)

Why would anyone...

The pattern I see in practise is getting the parameters, then for a subset of the parameters (e.g. scaleResolutionDownBy) overwrite it to be what the app's current settings are and then setting the resulting parameters.

The issue for the app doing this pattern boils down to the app thinking "the current settings are scaleResolutionDownBy is undefined". So it setParameters() with scaleResolutionDownBy not having a value, as per current setting.

  • In Chrome, this works because getParameters returns undefined and setParameters with undefined results in 4:2:1.
  • In Firefox, this does not work because getParameters returns 4:2:1 and setParameters with undefined results in 1:1:1.
  • In Safari, this doubly does not work because getParameters is 1:1:1 by default, but setParameters with undefined is also 1:1:1.

You could argue that the app should update its current settings to 4:2:1 and never set undefined scaleResolutionDownBy in setParameters, but today there is a preference towards not specifying scaleResolutionDownBy so we end up having to use "if browser..." gates to work around this.

From my recollection of discussions on the existing behavior, we stopped short of applying magic defaults once parameters have been fiddled with, since they arguably stop being POLA at that point.

I recall the discussion being "after initial setup, scaleResolutionDownBy will have a value, so there's no need for the 4:2:1 algorithm anymore". This discussion did not anticipate the fact that:

  • Apps having a current setting which distinguishes between undefined and 4:2:1.
  • Some browsers return undefined in getParamers rather than 4:2:1 if the last value set was undefined.

@henbos
Copy link
Contributor Author

henbos commented May 22, 2023

today there is a preference towards not specifying scaleResolutionDownBy

This preference has to do with a bug in adaptation logic when scaleResolutionDownBy being specified or with specifying it being an opt-out of legacy VP9 kSVC mode.

So to be fair, the preference basically has to do with browser bugs and historical reasons rather than sensible things. But I still think there is value in allowing the app to not specifying scale and getting 4:2:1 so that applying the same parameters at init and at setParameters have the same effect.

@henbos
Copy link
Contributor Author

henbos commented May 22, 2023

Once we have fixed all bugs and unshipped kSVC, there will no longer be a need for the app to apply undefined, so insisting that the app explicitly specified 4:2:1 if it wants 4:2:1 makes sense 1-2 years from now.

But interpreting undefined as 1:1:1 also makes little sense in the meantime.

Whatever we decide, we need to make sure all browsers does the same thing, so what should the spec say?

  1. setParameters with undefined scale = 4:2:1
  2. setParameters with undefined scale = 1:1:1
  3. setParameters with undefined scale = throw an exception

My take is that 1) has the least amount of backwards compat risk, 3) has the most backwards-compat risk, and 2) is somewhere in the middle where breakage is likely but less severe. Changing from 1:1:1 to 4:2:1 is a change in behavior on some browsers, but it is also unlikely that anyone depended on this behavior since nobody should want to send 1:1:1. Changing from 4:2:1 to 1:1:1 on the other hand is risky since apps use this code path today on Chromium.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants