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

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

Closed
docfaraday opened this issue May 16, 2022 · 5 comments · Fixed by #2788
Closed

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

docfaraday opened this issue May 16, 2022 · 5 comments · Fixed by #2788
Assignees

Comments

@docfaraday
Copy link
Contributor

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.

@jan-ivar
Copy link
Member

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

@jan-ivar
Copy link
Member

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).

@docfaraday
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@docfaraday
Copy link
Contributor Author

docfaraday commented Oct 5, 2022

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

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

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).

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

Successfully merging a pull request may close this issue.

3 participants