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

Modifications to [[SendEncodings]] from setParameters and sRD can be racy

Open docfaraday opened this issue 3 years ago • 4 comments

setParameters has the following flow in brief:

  1. Synchronously executed state and validity checking
  2. In parallel, apply the new parameters to the RTP stream being transmitted (might involve queuing, might not)
  3. A queued task (that is enqueued during step 2) that updates [[SendEncodings]], and then resolves the promise

sRD(answer) can reject an offered simulcast, which also results in a modification to [[SendEncodings]] (by truncating to size 1). It seems like it would be fairly easy for a message handler to call sRD(answer) so that the truncation happens between steps 1 and 2, or steps 2 and 3.

docfaraday avatar May 16 '22 19:05 docfaraday

@docfaraday mentioned offline a similar issue with addTrack and SRD(simulcastOffer).

jan-ivar avatar Aug 16 '22 02:08 jan-ivar

My sense is there are two aspects to this problem:

  1. The setParameters algorithm's approach of updating entire dictionaries instead of individual members overwrites unrelated data at times
  2. The inherent race of two sides competing to modify the scaleResolutionDownBy property

1 seems more problematic to me than 2.

IOW, if setParameters were a three-way merge (soving 1) then the only remaining problem would seem to be 2 which seems fine since it's an inherent race between parties (a few milliseconds earlier and you'd get one outcome, vs. a few milliseconds later and you'd get the other).

jan-ivar avatar Aug 16 '22 02:08 jan-ivar

@docfaraday mentioned offline a similar issue with addTrack and SRD(simulcastOffer).

After more offline discussion, it seems like this particular question (what to do when setParameters races against sRD(simulcast offer)) depends on what the expected behavior is in the non-racy case. So if JS calls addTrack, then performs a get/setParameters cycle (which will have one encoding), what should be done with a subsequent sRD(simulcast offer)? Do we interpret the unicast get/setParameters cycle as establishing unicast, and reject the simulcast in our answer? Do we have the sRD(simulcast offer) stomp the encodings entirely? Or do we try to merge the two parameter sets somehow?

docfaraday avatar Aug 16 '22 15:08 docfaraday

Also, we need to deal with setParameters racing against rollback.

docfaraday avatar Sep 13 '22 21:09 docfaraday

There are a handful of types of race here, on further thought.

addTrack, then a race between sRD(simulcast recv offer) and setParameters(unicast)

  • There are multiple questions here. First we need to decide what addTrack followed by unicast setParameters means (that's #2777). Whatever our decision is there, I think the same result should happen in the racy case.

a race between sRD(unicast reoffer/answer/reanswer) and setParameters(simulcast)

  • I think we can agree that we do not want to end up settling on simulcast when this happens. We probably want the same result that we would have gotten if we ran the setParameters to completion, and then the sRD. (Doing the opposite order would result in the setParameters failing; not as clean, but possibly viable. I prefer not to do this.)

a race between sRD(rollback of a unicast reoffer) and setParameters(unicast)

  • We want to end up back in simulcast here, and I guess we want to allow the changes from the setParameters to persist; that encoding will be there either way.

a race between sRD(rollback of initial simulcast offer) and setParameters(simulcast)

  • We definitely want there to be just one encoding when this finishes. Do we want that encoding to have the changes from the setParameters call?

I think the simplest way to handle all of these is to mimic what we do when addTrack races against sRD. Let the setParameters finish, and then re-do the sRD(whatever).

docfaraday avatar Oct 05 '22 22:10 docfaraday