Fix ignored constraints in LocalTrack.restart #1435
Conversation
🦋 Changeset detectedLatest commit: e10afd6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
src/room/track/LocalTrack.ts
Outdated
| constraints = this._constraints; | ||
| } | ||
| const { deviceId, ...otherConstraints } = this._constraints; | ||
| constraints = { ...this._constraints, ...(constraints ?? {}) }; |
There was a problem hiding this comment.
this mutates the passed constraints, we'd want to create a separate local variable for this
src/room/track/LocalTrack.ts
Outdated
| this._constraints = trackConstraints; | ||
|
|
There was a problem hiding this comment.
I am modifying the order of operations in the restart method. I moved this._constraints = trackConstraints; before the setMediaStream call. Please review these changes!
The reason for this change is to prevent potential loss of deviceId information. Here's the current issue:
- If we get a new track without a specified deviceId
- Then in setMediaStreamTrack, we do
this._constraints = newTrack.getConstraints();(which might obtain a new deviceId) - And then back in restart, we do
this._constraints = trackConstraints; - This could result in losing the deviceId information
There was a problem hiding this comment.
we do this._constraints = newTrack.getConstraints(); (which might obtain a new deviceId)
I don't believe this is true, getConstraints should always return the constraints that have been used to construct the mediastreamtrack, see https://w3c.github.io/mediacapture-main/#dom-constrainablepattern-getconstraints
There was a problem hiding this comment.
oh, maybe you're right and this
that were the argument to the most recent successful invocation
means that it's exclusively the last call to applyconstraints which would always be missing the device Id in our case.
There was a problem hiding this comment.
I agree with you, and I think my explanation was incomplete.
I was concerned about cases where this._constraints's deviceId becomes undefined before creating newTrack, like this:
const trackConstraints = { ...this._constraints, ...constraints?? {} }
const { deviceId } = trackConstraints // deviceId is undefined
// omitted
const newTrack = await navigator.mediaDevices.getUserMedia({ audio: true }).getTracks()[0]
await this.setMediaStreamTrack(newTrack); // this._constraints = { deviceId: "anything" }
this._constraints = trackConstraints; // this._constraints = { deviceId: undefined }Even if such situations don't occur, I think there's no guarantee that trackConstraints and newTrack.getConstraints() will always be identical in the current code. Therefore, I believed it would be safer to assign newTrack.getConstraints() to this.constraints at the end.
If anything is unclear or if you disagree with my explanation, please let me know🙏
There was a problem hiding this comment.
@lukasIO Do you agree with this change? While this is a different context from the original issue we were trying to solve, I had provided an additional suggestion. If it's not valid, we can rollback this :)
There was a problem hiding this comment.
yeah, please revert this. If you have a reproduction of a case where this causes an actual problem feel free to open a separate issue/PR and I'll take a look :)
| if (!constraints) { | ||
| constraints = this._constraints; | ||
| } | ||
| const { deviceId, ...otherConstraints } = this._constraints; |
There was a problem hiding this comment.
@rktguswjd thinking more about this I think the correct fix is simply to change this line to
const { deviceId, ...otherConstraints } = constraints;
There was a problem hiding this comment.
Aha, I thought we needed to merge this._constraints with the given constraints. I see that wasn't the intention! I'll fix it.
There was a problem hiding this comment.
thanks, if you can just revert that other change now, then we're good to go!
The reason the constraints are overridden after setMediaStreamTrack is because the constraints passed in by the user are the desired constraints. We want to reuse the desired constraints after, too. And as we don't merge any constraints together this should be sufficient
There was a problem hiding this comment.
the constraints passed in by the user are the desired constraints.
That makes sense! Thank you for clarifying the intention!
rktguswjd
force-pushed
the
fix/local-track-restart
branch
from
b93f0a7 to
eb787d7
Compare
|
@lukasIO Oh, sorry. I did a force push and the changeset commit disappeared🥲 |
|
no worries, I added it back in! |
Co-authored-by: lukasIO <mail@lukasseiler.de>
resolve #1434