Skip to content

Allow developers to control the audio codecs - #1240

Open
jpsantosbh wants to merge 69 commits into
mainfrom
joao/codec_configs_with_test_refactor
Open

Allow developers to control the audio codecs#1240
jpsantosbh wants to merge 69 commits into
mainfrom
joao/codec_configs_with_test_refactor

Conversation

@jpsantosbh

Copy link
Copy Markdown
Collaborator

Description

Added dial params:

  • audioCodecs

The new parameter allows developers to select the audio codec to use with the desired configuration

Replaced the sdp helper lib with sdpTransform and refactored some of the sdpHelpers code to benefit from sdpTransform

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

  • Internal refactoring
  • Bug fix (bugfix - non-breaking)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Code snippets

client.dial({
    to: document.getElementById('destination').value,
    rootElement: document.getElementById('rootElement'),
    video: true,
    audio: true,
    audioCodecs:[
     'opus:maxplaybackrate=8000;maxaveragebitrate=4000',
     'telephone-event:',
     'PCMU:'.
     'PCMA:'
    ]
  }})

jpsantosbh and others added 30 commits March 7, 2025 22:18
Co-authored-by: Ammar Ansari <iammaransari@gmail.com>
Co-authored-by: Ammar Ansari <iammaransari@gmail.com>
Co-authored-by: Ammar Ansari <iammaransari@gmail.com>
Co-authored-by: Ammar Ansari <iammaransari@gmail.com>
Co-authored-by: Ammar Ansari <iammaransari@gmail.com>
Co-authored-by: Ammar Ansari <iammaransari@gmail.com>
Co-authored-by: Ammar Ansari <iammaransari@gmail.com>
Co-authored-by: Ammar Ansari <iammaransari@gmail.com>
@changeset-bot

changeset-bot Bot commented Jun 12, 2025

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: dd63db6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@signalwire/webrtc Minor
@signalwire/js Minor

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

@jpsantosbh
jpsantosbh requested review from Copilot and iAmmar7 June 13, 2025 12:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 calling client.dial().
  • Swapped out the old sdp helper with the sdpTransform 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

Comment thread internal/e2e-js/tests/buildVideoWithVideoSDK.spec.ts
Comment thread internal/e2e-js/tests/roomSessionLocalStream.spec.ts
@iAmmar7

iAmmar7 commented Jun 16, 2025

Copy link
Copy Markdown
Member

Since you have reopened the PR, adding commits from my WIP PR https://github.com/signalwire/signalwire-js/pull/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.
Here is my refactor PR (39 file changes): https://github.com/signalwire/signalwire-js/pull/1233

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.

@jpsantosbh

Copy link
Copy Markdown
Collaborator Author

@iAmmar7

I'm fine with reverting https://github.com/signalwire/signalwire-js/pull/1233 changes from this PR.
We could:

a) Fully revert it, and skip the streaming test.
b) Keep the changes only for the Streaming test. Even if still flaky, they improved the tests, making them pass.

What do you prefer?

@iAmmar7

iAmmar7 commented Jun 24, 2025

Copy link
Copy Markdown
Member

@iAmmar7

I'm fine with reverting #1233 changes from this PR. We could:

a) Fully revert it, and skip the streaming test. b) Keep the changes only for the Streaming test. Even if still flaky, they improved the tests, making them pass.

What do you prefer?

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.

@jpsantosbh

Copy link
Copy Markdown
Collaborator Author

Cool. I'll revert and skip then.

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 this pull request may close these issues.

3 participants