-
Notifications
You must be signed in to change notification settings - Fork 17
Allow developers to control the audio codecs #1240
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Ammar Ansari <[email protected]>
Co-authored-by: Ammar Ansari <[email protected]>
Co-authored-by: Ammar Ansari <[email protected]>
Co-authored-by: Ammar Ansari <[email protected]>
Co-authored-by: Ammar Ansari <[email protected]>
Co-authored-by: Ammar Ansari <[email protected]>
Co-authored-by: Ammar Ansari <[email protected]>
Co-authored-by: Ammar Ansari <[email protected]>
…s into joao/opus_config
- Resolved conflict in packages/core/src/utils/index.ts: Added new helper functions from main - Resolved conflict in packages/js/src/fabric/WSClient.ts: Updated buildOutboundCall to use ReattachParams - Resolved conflict in packages/webrtc/package.json: Updated @signalwire/core version but kept sdp-transform dependency - Updated package-lock.json
…ec_configs_with_test_refactor
🦋 Changeset detectedLatest commit: dd63db6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new audioCodecs
dial parameter and refactors SDP handling for more semantic manipulation.
- Added
audioCodecs
option so developers can specify audio codec configurations when callingclient.dial()
. - Swapped out the old
sdp
helper with thesdpTransform
library and refactored related functions. - Updated end-to-end tests to use the new
expectRoomJoinWithDefaults
helper and adjusted CI config to include the feature branch.
Reviewed Changes
Copilot reviewed 54 out of 54 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
internal/e2e-js/tests/roomSessionMethodsOnNonExistingMembers.spec.ts | Replaced expectRoomJoined with expectRoomJoinWithDefaults in room-join assertions. |
internal/e2e-js/tests/roomSessionMaxMembers.spec.ts | Updated tests to call expectRoomJoinWithDefaults for consistent join defaults. |
internal/e2e-js/tests/roomSessionLockUnlock.spec.ts | Swapped to expectRoomJoinWithDefaults when verifying room join. |
internal/e2e-js/tests/roomSessionLocalStream.spec.ts | Changed join assertion to expectRoomJoinWithDefaults for custom local stream tests. |
internal/e2e-js/tests/roomSessionJoinUntil.spec.ts | Removed expectToJoin flag in join-until tests to consolidate on default join helper. |
internal/e2e-js/tests/roomSessionJoinFrom.spec.ts | Now using expectRoomJoinWithDefaults to validate join behavior before/after join_from . |
internal/e2e-js/tests/roomSessionFollowLeader.spec.ts | Updated multi-page join logic to use expectRoomJoinWithDefaults with join roles. |
internal/e2e-js/tests/roomSessionDevices.spec.ts | Migrated room-join checks to expectRoomJoinWithDefaults . |
internal/e2e-js/tests/roomSessionDemoteReattachPromote.spec.ts | Refactored demote/reattach workflows to rely on expectRoomJoinWithDefaults . |
internal/e2e-js/tests/roomSessionDemotePromote.spec.ts | Replaced direct expectRoomJoined calls with expectRoomJoinWithDefaults . |
internal/e2e-js/tests/roomSessionDemoteAudience.spec.ts | Swapped audience demotion joins to use expectRoomJoinWithDefaults . |
internal/e2e-js/tests/roomSessionDemote.spec.ts | Updated demotion flows to validate rejoin via expectRoomJoinWithDefaults . |
internal/e2e-js/tests/roomSessionCleanup.spec.ts | Changed cleanup test to assert join via expectRoomJoinWithDefaults . |
internal/e2e-js/tests/roomSessionBadNetwork.spec.ts | Now passes joinAs into expectRoomJoinWithDefaults for network tests. |
internal/e2e-js/tests/roomSessionAutomaticStream.spec.ts | Updated auto-stream tests to call expectRoomJoinWithDefaults . |
internal/e2e-js/tests/roomSessionAudienceCount.spec.ts | Replaced audience-count join validations with expectRoomJoinWithDefaults . |
internal/e2e-js/tests/roomSession.spec.ts | Unified base-room join steps under expectRoomJoinWithDefaults . |
internal/e2e-js/tests/buildVideoWithVideoSDK.spec.ts | Switched video SDK tests to use expectRoomJoinWithDefaults for join events. |
.github/workflows/unit-tests.yml | Added feature branch to PR triggers and temporarily commented out legacy test jobs. |
.changeset/cute-zebras-grin.md | Documented the new audioCodecs parameter in the changelog. |
Comments suppressed due to low confidence (4)
internal/e2e-js/tests/roomSessionJoinUntil.spec.ts:65
- [nitpick] Removing the
expectToJoin: false
flag may skip validation of the intended non-join behavior; ensure the test still verifies join failure as expected.
expectToJoin: false,
.github/workflows/unit-tests.yml:15
- The
tests
job is commented out, which may prevent unit and stack tests from running; re-enable or update this section to restore CI coverage.
# tests:
.github/workflows/unit-tests.yml:7
- [nitpick] Including a specific user branch in the workflow triggers may not scale; consider using a more generic pattern or relying on branch naming conventions.
branches: [main, dev, joao/opus_config]
.github/workflows/unit-tests.yml:46
- The
needs: tests
dependency is commented out for the production staging job, which could allow it to run before tests complete; restore the dependency to enforce CI order.
# needs: tests
Since you have reopened the PR, adding commits from my WIP PR #1233. I will repeat what I mentioned on Slack. Please try to understand that the test refactor I made has nothing to do with your PR changes. My refactor PR is targeting the main branch since I found a possible race condition on the main branch, not in your PR. That refactor touches almost all the e2e tests. Please check the CI, it's red, which shows the solution is half-cooked and can't be merged. It was not so easy to debug and understand the race condition. Merging this PR in the current state may lead to God knows how many weird race conditions. It will be even harder to debug. That's why I request you not to merge the tests in the current state; it's NOT ready. Your PR for the audio codec has been open for the past month. If it is still a top priority, I would suggest merging the PR as it is without touching the E2E tests. The SDK is already in dev version, as you said before, so I don't think it matters. |
Both options make the SDK unreliable. Considering the SDK is already in dev mode, it seems unreliable as it is. Also, I believe you already skipped some tests in one of the recent PRs, so I don’t think skipping one more test will make much of a difference. |
Cool. I'll revert and skip then. |
Description
Added dial params:
The new parameter allows developers to select the audio codec to use with the desired configuration
Replaced the
sdp
helper lib withsdpTransform
and refactored some of the sdpHelpers code to benefit fromsdpTransform
This allows a more semantic SDP manipulation now that we're exposing to the developer parameters to make changes on the SDP offers.
Type of change
Code snippets