w3c / webrtc-extensions

A repository for experimental additions to the WebRTC API
https://w3c.github.io/webrtc-extensions/
Other
58 stars 19 forks source link

What happens if scaleTo: {360p, 720p} and frame is 360 #222

Closed henbos closed 1 month ago

henbos commented 1 month ago

The restrictions applied on the frame would result in {360p, 360p}.

The question is, should we not send the "top" layer or should we send 360p twice?

henbos commented 1 month ago

For reference, the part that got removed from #221 is:

In simulcast, not upscaling can result in multiple same-sized encodings if the size of the frame is smaller than {{scaleResolutionDownTo}}; an encoding MUST NOT be sent if {{scaleResolutionDownTo}} is larger than the frame and there exists another {{RTCRtpEncodingParameters/active}} encoding with a smaller {{scaleResolutionDownTo}} density resulting in the same size.

henbos commented 1 month ago

Here's an example of using this API:

1. Initially configure {360p, 720p} with scaleTo and a track that is 720p.
2. Make 720p layer active=false. {360p, off} is achieved.
3. No need to do 720p effects processing so the track is changed to 360p.

So far so good, no race and we save CPU. But now we want to enable 720p again:

1. Increase size of track to 720p.
2. Re-enable the 720p encoding: {360p, 720p}.

Both step 1) and step 2) here happen in-parallel.

If it is the case that any 720p frame emitted by step 1) always reaches the encoder before any activation instruction from step 2) reaches the encoder then there is no race here. But if media passes by some thread that the setParameters call is not passing through then there could be a race here where "active=true" happens before "720p reaches encoder".

If this race is a non-issue then I agree it's better not to inactivate encodings on behalf of the app since the app should know what it's doing. @jan-ivar

henbos commented 1 month ago

BreakoutBox would be running in a different thread than the main thread doing setParameters() too. If that changes things?

jan-ivar commented 1 month ago

(renumbering the second 1,2 steps to 4,5)

4. Increase size of track to 720p.

{360p, off} is achieved. No race so far

5. Re-enable the 720p encoding: {360p, 720p}.

Is the problem not knowing how long to wait to do this step to ensure a clean {360p, off} → {360p, 720p} instead of {360p, off} → {360p, 360p} → {360p, 720p}?

Is shaving off a few 360p frames a significant saving during this transition to using more bandwidth?

henbos commented 1 month ago

Is the problem not knowing how long to wait to do this step to ensure a clean {360p, off} → {360p, 720p} instead of {360p, off} → {360p, 360p} → {360p, 720p}?

Not knowing how long to wait, but also we don't want to wait as that would be a user visible glitch. But yeah the race is {360p, off} → {360p, 360p} → {360p, 720p} instead of {360p, off} -> {360p, 720p}. If it happened it would only be a very brief 360p.

Is shaving off a few 360p frames a significant saving during this transition to using more bandwidth?

When you phrase it like that, I suppose the momentarily going from 360p -> 720p is similar to normal ramp up. I think the bigger concern is that it may be confusing for the receiver to get {360p, 360p}, but perhaps that's ok.

Do you prefer to allow the 360p in order to keep spec + implementation simpler?

henbos commented 1 month ago

@jan-ivar I propose we close this issue in favor of keeping things simple since this edge case is equivalent to a ramp-up. Agreed?

jan-ivar commented 1 month ago

Works for me.