Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements default audio filters for mobile clients by introducing Google-specific audio processing constraints. These defaults can be overridden by users through custom audio constraints.
Changes:
- Added new
constraints.tsmodule with default mobile audio constraints and merge logic - Modified
FishjamProviderto automatically apply default audio constraints - Exported
DEFAULT_MOBILE_AUDIO_CONSTRAINTSfor user reference
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/mobile-client/src/constraints.ts | New file defining default Google audio processing constraints and merge function |
| packages/mobile-client/src/index.ts | Updated to import and apply default audio constraints in FishjamProvider |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ud/web-client-sdk into FCE-2591/apply-audio-filters
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const mergedConstraints = React.useMemo(() => { | ||
| return { | ||
| ...props.constraints, | ||
| audio: mergeMobileAudioConstraints(props.constraints?.audio), | ||
| }; | ||
| }, [props.constraints]); |
There was a problem hiding this comment.
The useMemo dependency array includes props.constraints which is an object reference. This will cause the memo to recompute on every render if the parent component doesn't memoize the constraints object, defeating the purpose of using useMemo. Consider using individual properties as dependencies (e.g., props.constraints?.audio, props.constraints?.video) or document that users should memoize their constraints object. Alternatively, follow the pattern used in react-client's FishjamProvider which directly passes props.constraints without wrapping in useMemo.
| const mergedConstraints = React.useMemo(() => { | |
| return { | |
| ...props.constraints, | |
| audio: mergeMobileAudioConstraints(props.constraints?.audio), | |
| }; | |
| }, [props.constraints]); | |
| const mergedConstraints = { | |
| ...props.constraints, | |
| audio: mergeMobileAudioConstraints(props.constraints?.audio), | |
| }; |
| /** | ||
| * Google-specific audio processing constraints (legacy Chrome constraints). | ||
| * These are not part of the standard MediaTrackConstraints type but are supported by WebRTC implementations. | ||
| */ | ||
| interface GoogleAudioConstraints { | ||
| googEchoCancellation?: string | boolean; | ||
| googAutoGainControl?: string | boolean; | ||
| googNoiseSuppression?: string | boolean; | ||
| googTypingNoiseDetection?: string | boolean; | ||
| googHighpassFilter?: string | boolean; | ||
| } | ||
|
|
||
| /** | ||
| * Extended MediaTrackConstraints that includes Google-specific audio processing properties. | ||
| */ | ||
| export type ExtendedMediaTrackConstraints = MediaTrackConstraints & GoogleAudioConstraints; | ||
|
|
||
| /** | ||
| * Default audio constraints for mobile-client with Google audio processing features enabled. | ||
| * These constraints can be overridden by passing custom audio constraints to FishjamProvider. | ||
| */ | ||
| export const DEFAULT_MOBILE_AUDIO_CONSTRAINTS: ExtendedMediaTrackConstraints = { | ||
| googEchoCancellation: true, | ||
| googAutoGainControl: true, | ||
| googNoiseSuppression: true, | ||
| googTypingNoiseDetection: true, | ||
| googHighpassFilter: true, | ||
| }; | ||
|
|
||
| /** | ||
| * Merges default mobile audio constraints with user-provided constraints. | ||
| * User-provided constraints take precedence over defaults. | ||
| * | ||
| * @param userConstraints - User-provided audio constraints (can be boolean or MediaTrackConstraints) | ||
| * @returns Merged audio constraints | ||
| */ | ||
| export function mergeMobileAudioConstraints( | ||
| userConstraints?: ExtendedMediaTrackConstraints | boolean, | ||
| ): ExtendedMediaTrackConstraints | boolean { | ||
| if (userConstraints === false) { | ||
| return false; | ||
| } | ||
|
|
||
| if (userConstraints === true || userConstraints === undefined) { | ||
| return DEFAULT_MOBILE_AUDIO_CONSTRAINTS; | ||
| } | ||
|
|
||
| return { | ||
| ...DEFAULT_MOBILE_AUDIO_CONSTRAINTS, | ||
| ...userConstraints, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The ExtendedMediaTrackConstraints type and DEFAULT_MOBILE_AUDIO_CONSTRAINTS constant are not exported from the module. Users who want to reference these defaults or understand the type structure for their custom constraints cannot access them. Consider exporting these to improve developer experience and API discoverability. For example, add these exports to index.ts:
export type { ExtendedMediaTrackConstraints } from './constraints';
export { DEFAULT_MOBILE_AUDIO_CONSTRAINTS } from './constraints';
| interface GoogleAudioConstraints { | ||
| googEchoCancellation?: string | boolean; | ||
| googAutoGainControl?: string | boolean; | ||
| googNoiseSuppression?: string | boolean; | ||
| googTypingNoiseDetection?: string | boolean; | ||
| googHighpassFilter?: string | boolean; | ||
| } |
There was a problem hiding this comment.
The GoogleAudioConstraints interface allows both string and boolean values (e.g., googEchoCancellation?: string | boolean), but the DEFAULT_MOBILE_AUDIO_CONSTRAINTS only uses boolean values (all set to true). If string values are not needed or supported, consider simplifying the interface to only accept boolean values for clarity. If string values are needed, the comment should explain when and why string values would be used instead of boolean.
|
Turns out these filters are enabled natively in react-native-webrtc and will be ignored anyway. |
2 participants
Description
Apply default audio filters that can be overwritten by the user.
Documentation impact
Types of changes
not work as expected)