Add Noise Suppression (#1264) #1287

ChabanovX · 2025-06-20T11:23:01Z

Resolves #1264

Synopsis

Add user-configurable noise suppression now that medea_flutter_webrtc 0.14.0
exposes the feature (Desktop). Users need to toggle it in My Profile ▸ Media.

Solution

Upgrade deps. Extend MediaSettings. Migrate DB. Add API in repository. Paint UI. Write tests

Checklist

SleepySquash · 2025-06-25T12:32:16Z

lib/domain/model/ongoing_call.dart

+            await track.getTrack().setNoiseSuppressionEnabled(
+              _noiseSuppressionEnabled!,
+            );


@ChabanovX, I've noticed that CI fails to build Web platform due to NoiseSuppressionLevel enum not being available there. And that seems to be true, if we talk about NoiseSuppressionLevel from the medea_flutter_webrtc. I've discussed this matter with guys from medea_flutter_webrtc and they told me that we shouldn't use this enum from that package - instead the NoiseSuppressionLevel from medea_jason should be used. Can you please try that?

And in that case the proper method to change the noise suppression settings would be to call this method over track directly:

Suggested change

await track.getTrack().setNoiseSuppressionEnabled(

_noiseSuppressionEnabled!,

);

await track.setNoiseSuppressionEnabled(

_noiseSuppressionEnabled!,

);

Okay, got it!

ChabanovX · 2025-06-27T11:22:23Z

FCM

Add noise suppression settings to MyProfile page (#1287, #1264)

SleepySquash · 2025-06-27T11:55:21Z

assets/l10n/en-US.ftl

 label_media_settings = Media settings
+label_voice_processing = Voice processing
+label_noise_suppression = Noise suppression
+label_low = Low
+label_medium = Moderate
+label_high = High
+label_very_high = Very high
 label_message = Message


Can you please sort these l10n keys alphabetically? You may install a sort extension to your IDE so that it can be easier.

SleepySquash · 2025-06-27T12:14:14Z

lib/domain/model/ongoing_call.dart

-import 'package:medea_flutter_webrtc/medea_flutter_webrtc.dart' as webrtc;
+import 'package:medea_flutter_webrtc/medea_flutter_webrtc.dart'
+    as webrtc
+    hide NoiseSuppressionLevel;


If this import is prefixed with webrtc, then Dart shouldn't confuse webrtc.NoiseSuppressionLevel from medea_flutter_webrtc with NoiseSuppressionLevel from medea_jason, since the first one requires a prefix to be specified. Thus this hide NoiseSuppressionLevel may be removed? Or not?

Yes, it might be removed, but if we leave it as it is, then other developers might be hinted, that webrtc version of NoiseSuppressionLevel should be avoided. Might not be important though. Should I remove it?

@ChabanovX, this approach seems to be true for every enum present both in medea_jason and medea_flutter_webrtc. Not sure that we will be able to address all of those in such way with hide.

SleepySquash · 2025-06-27T14:35:20Z

lib/domain/model/ongoing_call.dart

@@ -2154,7 +2164,56 @@ class OngoingCall {
            devices.firstWhereOrNull(
              (e) => e.deviceId() == track.getTrack().deviceId(),
            );
+
+        _addNoiseSuppression(track);


Can we await this call? Or shouldn't we?

I think - yes, there is no reason for not awaiting this call

SleepySquash · 2025-06-27T14:46:29Z

test/widget/user_profile_suppression_test.dart

+/// Simulate [ProfileController].
+class _NoiseController extends GetxController {
+  final media = Rx(MediaSettings(noiseSuppressionEnabled: false));
+
+  void setNoiseSuppressionEnabled(bool enabled) {
+    media.update((m) {
+      m ??= MediaSettings();
+      m.noiseSuppressionEnabled = enabled;
+    });
+  }
+
+  void setNoiseSuppressionLevelValue(NoiseSuppressionLevel level) {
+    media.update((m) {
+      m ??= MediaSettings();
+      m.noiseSuppressionLevel = level;
+    });
+  }
+}
+
 class _NoiseSuppressionView extends StatelessWidget {
+  const _NoiseSuppressionView(this.c);
+
+  final _NoiseController c;
+
+  @override
+  Widget build(BuildContext context) {
+    final style = Theme.of(context).style;


It seems like this test only tests the widgets defined and implemented in the test itself and not the application. Developers may forget the fact that whenever MyProfile pages is changed this code should be changed as well. It's easy to lose track of. Can't be just test the MyProfile page itself?

SleepySquash · 2025-06-27T14:52:27Z

CHANGELOG.md

 [#1263]: /../../issue/1263
+[#1264]: /../../issue/1264
 [#1268]: /../../issue/1268
 [#1280]: /../../pull/1280
 [#1285]: /../../pull/1285
+[#1285]: /../../pull/1287


I can see the links being added, but no actual change entry?

SleepySquash · 2025-06-27T15:00:02Z

lib/ui/page/home/widget/block.dart

+                  child: ClipPath(
+                    clipper: _BottomEdgeClipper(),
+                    child: AnimatedSize(


Won't this clipping applied to every single Block affect performance? https://docs.flutter.dev/release/breaking-changes/clip-behavior#summary

Yes, very good notice, I will add optional parameter for applying the clipping

SleepySquash · 2025-06-27T15:00:40Z

lib/ui/page/home/widget/block.dart

+class _BottomEdgeClipper extends CustomClipper<Path> {
+  @override
+  Path getClip(Size size) {
+    const double kBig = 1e6;


https://dart.dev/effective-dart/style#dont-use-prefix-letters

SleepySquash · 2025-06-27T15:01:47Z

lib/ui/page/home/widget/block.dart

+  @override
+  Path getClip(Size size) {
+    const double kBig = 1e6;
+    return Path()..addRect(Rect.fromLTRB(-kBig, 0, kBig, size.height));


Can't we use double.maxFinite here in this case?

I've been debugging and noticed a problem using infinite values. I checked how rect is added to path and noticed this check:

bool _rectIsValid(Rect rect) { assert(!rect.hasNaN, 'Rect argument contained a NaN value.'); return true; }

double.infinity converts to NotANumber value and assertion happens. Hence, we cannot deal with infinite values and should leave finite number

@ChabanovX, why would you use a double.infinity then?

My mistake, read as infinity, maxFinite is working just fine and should be used here

ChabanovX · 2025-07-01T14:02:53Z

Should we remove widget test since we in the future we are going to introduce e2e test?
Also, I cut commits related to e2e enhancing @SleepySquash

SleepySquash · 2025-07-01T15:31:41Z

@ChabanovX,

Should we remove widget test since we in the future we are going to introduce e2e test?

Ok, let's remove it

SleepySquash

@ChabanovX, be sure to read thoughtfully your code before sending it for a review, please.

SleepySquash · 2025-07-02T10:50:54Z

assets/l10n/en-US.ftl

-label_user_removed_user3 = {" "}{$user}
+label_user_removed_user3 = { " "}{$user}


Will be fixed, I will never trust AI again to do the sorting and will write my own script to do it. So far I did not find a sorting extension, that would manage file with multicolumn labels.

SleepySquash · 2025-07-02T10:52:24Z

assets/l10n/en-US.ftl

@@ -1048,4 +1054,4 @@ question_mark = ?
 space = {" "}
 space_or_space = {" "}or{" "}
 space_plus_space = {" "}+{" "}
-space_vertical_space = {" "}|{" "}
+space_vertical_space = {" "}|{" "}


This file has no newline character at the end despite .editorconfig specified to have one:

[*] ... end_of_line = lf ...

You should install an extension that would respect .editorconfig files and apply the settings specified in there.

SleepySquash · 2025-07-02T11:09:53Z

assets/l10n/en-US.ftl

 label_no_limit = ∞
 label_no_messages = No messages
+label_noise_suppression = Noise suppression
 label_no_updates_are_available_subtitle = You have the latest version installed.
 label_no_updates_are_available_title = No updates are available


Doesn't seem sorted. #1287 (comment)
Why did you place this label between two label_no_* labels? What is the reasoning behind this?

That is a poor sorting algorithm and will be fixed.

SleepySquash · 2025-07-02T11:18:55Z

lib/domain/model/ongoing_call.dart

+    // Currently works only on Desktop
+    if (PlatformUtils.isWeb || !PlatformUtils.isDesktop) {
+      Log.debug(
+        '❌ Noise suppression is not supported on this platform',
+        '$runtimeType',
+      );
+      return;
+    }
+
+    final enabled = _noiseSuppressionEnabled;
+    if (enabled == null) {
+      Log.debug('❌ _noiseSuppressionEnabled is null', '$runtimeType');
+      return;
+    }


I don't think any logs should be printed if noise suppression is disabled or are unavailable for the current platform.

SleepySquash · 2025-07-02T11:19:36Z

lib/domain/model/ongoing_call.dart

+    if (!track.isAudioProcessingAvailable()) {
+      Log.debug(
+        '❌ Audio processing not available for this track $track',
+        '$runtimeType',
+      );
+      return;
+    }


I guess this should be checked only if noise suppression is enabled and the current platform is supported. Otherwise I don't see any reason to check it.

SleepySquash · 2025-07-02T11:20:30Z

lib/domain/model/ongoing_call.dart

+
+      final level = _noiseSuppressionLevel;
+      if (level == null) {
+        Log.debug('❌ _noiseSuppressionLevel is null', '$runtimeType');


This emoji makes it look like an error. Is it an error?

That is not an error, should be removed.

SleepySquash · 2025-07-02T11:20:58Z

lib/domain/model/ongoing_call.dart

+      Log.debug('✅ set level to $level', '$runtimeType');
+      await track.setNoiseSuppressionLevel(level);
+    } catch (e, _) {
+      Log.error('💥 Failed to configure noise suppression: $e', '$runtimeType');


This log should contain the track that failed to set the suppression for.

SleepySquash · 2025-07-02T11:23:29Z

lib/ui/page/home/widget/block.dart

+  /// Whether to clip overflowing content in height, but not width.
+  /// Defaults to `false` to avoid extra GPU work.
+  final bool clipHeight;


This documentation doesn't follow Effective Dart Documentation guidelines. https://dart.dev/effective-dart/documentation#do-separate-the-first-sentence-of-a-doc-comment-into-its-own-paragraph

Once again I carefully read the following paragraph; will be fixed.

SleepySquash · 2025-07-02T11:24:48Z

lib/ui/page/home/page/my_profile/view.dart

+            LineDivider('label_voice_processing'.l10n),
+            const SizedBox(height: 16),
+            Obx(() {
+              // False by default


Comments should have trailing dot. https://dart.dev/effective-dart/documentation#do-format-comments-like-sentences

SleepySquash · 2025-07-02T11:25:41Z

lib/ui/page/home/page/my_profile/view.dart

+              // Values of NoiseSuppressionLevel
+              final values = NoiseSuppressionLevel.values;


Comments should cover only parts that aren't obvious, I suppose. Otherwise those can make code less readable despite its purpose to make it more readable. Don't you think?

I agree, will be removed.

ChabanovX added this to the 0.6.0 milestone Jun 20, 2025

ChabanovX self-assigned this Jun 20, 2025

ChabanovX added enhancement Improvement of existing features or bugfix k::UI/UX UI (user interface) and UX (user experience) changes k::refactor Refactor changes of existing code labels Jun 20, 2025

ChabanovX force-pushed the 1264-add-noise-suppression branch from d70fb99 to dde9297 Compare June 24, 2025 10:16

SleepySquash reviewed Jun 25, 2025

View reviewed changes

ChabanovX force-pushed the 1264-add-noise-suppression branch from 1610adf to 05968e1 Compare June 26, 2025 13:58

ChabanovX added 19 commits June 27, 2025 13:50

chore: update rtc version

5959422

Add new fields in MediaSettings

a76abd2

Enhanced Settings Repo

ebd6a23

Made a migration of Settings Schema

fd157b1

enhance ongoing_call with noise settings

3b96c61

add new methods to MyProfileController

353a753

Implement raw view

284964a

Made migration of db

0011fa9

Fix UI inconsistencies and poor clipping

862c084

Finish UI of feature

ed8492e

nit: package related

6f67ddc

Migrate rtc -> jason

40330c2

Fix linting issues

bf9211e

Update version on iOS

13c9f11

Set level on enabled automatically

0b16dc3

Add localizations

e9fd8a0

Refactor addSuppression chunk of code

6fe1513

Centered labels in case of long text

c4b07b9

Wrote tests for feature

5a0c024

ChabanovX force-pushed the 1264-add-noise-suppression branch from dc4ec3a to 5a0c024 Compare June 27, 2025 10:51

ChabanovX added 2 commits June 27, 2025 13:55

Merge branch 'main' into 1264-add-noise-suppression

79911a0

Fix timers problems in test

c361836

ChabanovX changed the title ~~Draft: Add Noise Suppression (#1264)~~ Add Noise Suppression (#1264) Jun 27, 2025

Prepare for review

0a06af2

ChabanovX requested a review from SleepySquash June 27, 2025 11:30

SleepySquash requested changes Jun 27, 2025

View reviewed changes

ChabanovX added 6 commits June 30, 2025 14:22

Update CHANGELOG

d530401

Made clipping optional

1d210fb

Refactor ongoing_call

0551b56

Merge branch 'main' into 1264-add-noise-suppression

de2b5c6

Resolved fmt issues

be5af40

Sorted ftl files

806543e

ChabanovX force-pushed the 1264-add-noise-suppression branch from aa02b35 to 806543e Compare June 30, 2025 14:32

SleepySquash modified the milestones: 0.5.2, 0.6.0 Jul 1, 2025

ChabanovX mentioned this pull request Jul 1, 2025

Add platform mocking during e2e tests #1312

Open

ChabanovX force-pushed the 1264-add-noise-suppression branch from 4d20d13 to 806543e Compare July 1, 2025 13:39

Merge branch 'main' into 1264-add-noise-suppression

289d273

ChabanovX added 2 commits July 1, 2025 19:01

Remove widget test

de50162

Merge branch 'main' into 1264-add-noise-suppression

b93cb60

ChabanovX requested a review from SleepySquash July 2, 2025 08:46

SleepySquash requested changes Jul 2, 2025

View reviewed changes

		label_user_removed_user3 = {" "}{$user}
		label_user_removed_user3 = { " "}{$user}

		// Values of NoiseSuppressionLevel
		final values = NoiseSuppressionLevel.values;

Add Noise Suppression (#1264) #1287

Are you sure you want to change the base?

Add Noise Suppression (#1264) #1287

Conversation

Uh oh!

Synopsis

Solution

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

FCM

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment