Skip to content
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.

Sign up for GitHub

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

Zero memory loss by syncing missing audio frames to server backward #980

Merged
merged 4 commits into from
Oct 6, 2024
Merged

Zero memory loss by syncing missing audio frames to server backward #980

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a04b2c6
First draft
beastoin Oct 5, 2024
9bd32b8
Add chunking, flushing wals
beastoin Oct 6, 2024
af41ae8
Sync wals to listen/backward #1
beastoin Oct 6, 2024
0a009b6
Clarify timestamp, clean log
beastoin Oct 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions app/lib/backend/preferences.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
import 'dart:convert';

import 'package:collection/collection.dart';
import 'package:flutter/foundation.dart';
import 'package:friend_private/backend/schema/bt_device/bt_device.dart';
import 'package:friend_private/backend/schema/memory.dart';
import 'package:friend_private/backend/schema/message.dart';
import 'package:friend_private/backend/schema/person.dart';
import 'package:friend_private/backend/schema/plugin.dart';
import 'package:friend_private/backend/schema/transcript_segment.dart';
import 'package:friend_private/services/wals.dart';
import 'package:shared_preferences/shared_preferences.dart';

class SharedPreferencesUtil {
Expand Down Expand Up @@ -418,4 +420,14 @@ class SharedPreferencesUtil {
set locationPermissionRequested(bool value) => saveBool('locationPermissionRequested', value);

bool get locationPermissionRequested => getBool('locationPermissionRequested') ?? false;

set wals(List<Wal> wals) {
final List<String> value = wals.map((e) => jsonEncode(e.toJson())).toList();
saveStringList('wals', value);
}

List<Wal> get wals {
final List<String> value = getStringList('wals') ?? [];
return Wal.fromJsonList(value.map((e) => jsonDecode(e)).toList());
}
Comment on lines +423 to +432
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The new methods set wals and get wals are handling JSON encoding/decoding directly. This could lead to potential issues if the Wal object's structure changes in the future, as it would require updates in multiple places. It would be better to encapsulate this logic within the Wal class itself, providing a method to convert a list of Wal objects to a list of strings and vice versa.

  set wals(List<Wal> wals) {
-   final List<String> value = wals.map((e) => jsonEncode(e.toJson())).toList();
+   final List<String> value = Wal.encodeList(wals);
    saveStringList('wals', value);
  }

  List<Wal> get wals {
-   final List<String> value = getStringList('wals') ?? [];
-   return Wal.fromJsonList(value.map((e) => jsonDecode(e)).toList());
+   return Wal.decodeList(getStringList('wals') ?? []);
  }

In the Wal class, you can add these static methods:

class Wal {
  // Other existing code...

  static List<String> encodeList(List<Wal> wals) {
    return wals.map((wal) => jsonEncode(wal.toJson())).toList();
  }

  static List<Wal> decodeList(List<String> walStrings) {
    return walStrings.map((walString) => Wal.fromJson(jsonDecode(walString))).toList();
  }
}

This way, the responsibility of encoding/decoding Wal objects is kept within the Wal class, improving maintainability and reducing the risk of errors.

}
5 changes: 5 additions & 0 deletions app/lib/backend/schema/message_event.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ enum MessageEventType {
memoryProcessingStarted('memory_processing_started'),
processingMemoryStatusChanged('processing_memory_status_changed'),
ping('ping'),
memoyBackwardSynced('memory_backward_synced'),
unknown('unknown'),
;

Expand All @@ -28,6 +29,7 @@ class ServerMessageEvent {
ServerMemory? memory;
List<ServerMessage>? messages;
// ServerProcessingMemoryStatus? processingMemoryStatus;
String? name;

ServerMessageEvent(
this.type,
Expand All @@ -36,6 +38,7 @@ class ServerMessageEvent {
this.memory,
this.messages,
// this.processingMemoryStatus,
this.name,
);

static ServerMessageEvent fromJson(Map<String, dynamic> json) {
Expand All @@ -48,6 +51,8 @@ class ServerMessageEvent {
// json['processing_memory_status'] != null
// ? ServerProcessingMemoryStatus.valuesFromString(json['processing_memory_status'])
// : null,

json['name'],
);
}
}
7 changes: 4 additions & 3 deletions app/lib/main.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,6 @@ Future<bool> _init() async {
// Service manager
ServiceManager.init();

// TODO: thinh, move to app start
await ServiceManager.instance().start();

// Firebase
if (F.env == Environment.prod) {
await Firebase.initializeApp(options: prod.DefaultFirebaseOptions.currentPlatform, name: 'prod');
Expand All @@ -60,6 +57,10 @@ Future<bool> _init() async {
await NotificationService.instance.initialize();
await SharedPreferencesUtil.init();
await MixpanelManager.init();

// TODO: thinh, move to app start
await ServiceManager.instance().start();

bool isAuth = false;
try {
isAuth = (await getIdToken()) != null;
Expand Down
16 changes: 16 additions & 0 deletions app/lib/pages/memories/page.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,22 @@ class _MemoriesPageState extends State<MemoriesPage> with AutomaticKeepAliveClie
},
child: CustomScrollView(
slivers: [
// TODO: FIXME
SliverToBoxAdapter(
child: memoryProvider.missingWals.isNotEmpty
? Padding(
padding: const EdgeInsets.all(32.0),
child: Center(
child: GestureDetector(
onTap: () {
memoryProvider.syncWals();
},
child: Text(
"You have ${memoryProvider.missingWals.map((val) => val.seconds).reduce((a, b) => a + b)}s stereo localy, sync now?"),
),
),
)
: SizedBox.shrink()),
Comment on lines +49 to +64
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The code block here is responsible for displaying a prompt to the user when there are missing audio frames that need to be synced. However, it's not handling the case where memoryProvider.missingWals is null, which could lead to a null pointer exception if missingWals is not initialized. You should add a null check before accessing its isNotEmpty property.

Also, the calculation of total seconds of missing audio frames is done directly in the UI layer. This logic should be moved to the provider or model layer to keep the UI layer clean and focused on rendering.

- child: memoryProvider.missingWals.isNotEmpty
+ child: (memoryProvider.missingWals?.isNotEmpty ?? false)

// Move this logic to the provider or model layer
- "You have ${memoryProvider.missingWals.map((val) => val.seconds).reduce((a, b) => a + b)}s stereo localy, sync now?"
+ "You have ${memoryProvider.totalMissingSeconds}s stereo localy, sync now?"

Please consider moving the calculation logic to a method in the MemoryProvider class:

class MemoryProvider {
  // Other properties and methods...

  int get totalMissingSeconds {
    return missingWals?.map((val) => val.seconds)?.reduce((a, b) => a + b) ?? 0;
  }
}

const SliverToBoxAdapter(child: SizedBox(height: 32)),
const SliverToBoxAdapter(child: SpeechProfileCardWidget()),
const SliverToBoxAdapter(child: UpdateFirmwareCardWidget()),
Expand Down
26 changes: 13 additions & 13 deletions app/lib/providers/capture_provider.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import 'package:friend_private/services/notifications.dart';
import 'package:friend_private/services/services.dart';
import 'package:friend_private/services/sockets/sdcard_socket.dart';
import 'package:friend_private/services/sockets/transcription_connection.dart';
import 'package:friend_private/services/wals.dart';
import 'package:friend_private/utils/analytics/mixpanel.dart';
import 'package:friend_private/utils/enums.dart';
import 'package:friend_private/utils/logger.dart';
Expand All @@ -41,6 +42,8 @@ class CaptureProvider extends ChangeNotifier

ServerMemory? get inProgressMemory => _inProgressMemory;

IWalService get wal => ServiceManager.instance().wal;

void updateProviderInstances(MemoryProvider? mp, MessageProvider? p) {
memoryProvider = mp;
messageProvider = p;
Expand Down Expand Up @@ -146,19 +149,16 @@ class CaptureProvider extends ChangeNotifier

Future streamAudioToWs(String id, BleAudioCodec codec) async {
debugPrint('streamAudioToWs in capture_provider');
if (_bleBytesStream != null) {
_bleBytesStream?.cancel();
}
_bleBytesStream = await _getBleAudioBytesListener(
id,
onAudioBytesReceived: (List<int> value) {
if (value.isEmpty) return;
if (_socket?.state == SocketServiceState.connected) {
final trimmedValue = value.sublist(3);
_socket?.send(trimmedValue);
}
},
);
_bleBytesStream?.cancel();
_bleBytesStream = await _getBleAudioBytesListener(id, onAudioBytesReceived: (List<int> value) {
if (value.isEmpty) return;
wal.onByteStream(value);
if (_socket?.state == SocketServiceState.connected) {
final trimmedValue = value.sublist(3);
_socket?.send(trimmedValue);
wal.onBytesSync(value);
}
});
Comment on lines +152 to +161
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The new implementation introduces a Write-Ahead Logging (WAL) service to the audio streaming operation. However, there's a potential issue with the order of operations in this block. The wal.onByteStream(value); is called before checking if the socket state is connected. If the socket is not connected, the WAL service will still log the byte stream, which might not be the intended behavior. Consider moving the wal.onByteStream(value); call inside the condition where the socket state is checked.

-      wal.onByteStream(value);
       if (_socket?.state == SocketServiceState.connected) {
+         wal.onByteStream(value);
          final trimmedValue = value.sublist(3);
          _socket?.send(trimmedValue);
          wal.onBytesSync(value);
        }

This way, the byte stream is only logged when the socket is connected.

setAudioBytesConnected(true);
notifyListeners();
}
Expand Down
38 changes: 37 additions & 1 deletion app/lib/providers/memory_provider.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@ import 'package:friend_private/backend/http/api/memories.dart';
import 'package:friend_private/backend/preferences.dart';
import 'package:friend_private/backend/schema/memory.dart';
import 'package:friend_private/backend/schema/structured.dart';
import 'package:friend_private/services/services.dart';
import 'package:friend_private/services/wals.dart';
Comment on lines +9 to +10
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The new imports for services.dart and wals.dart are added correctly. No issues found.

+ import 'package:friend_private/services/services.dart';
+ import 'package:friend_private/services/wals.dart';

import 'package:friend_private/utils/analytics/mixpanel.dart';
import 'package:friend_private/utils/features/calendar.dart';

class MemoryProvider extends ChangeNotifier {
class MemoryProvider extends ChangeNotifier implements IWalServiceListener {
List<ServerMemory> memories = [];
Map<DateTime, List<ServerMemory>> groupedMemories = {};

Expand All @@ -22,6 +24,20 @@ class MemoryProvider extends ChangeNotifier {

List<ServerMemory> processingMemories = [];

IWalService get _wal => ServiceManager.instance().wal;
List<Wal> _missingWals = [];
List<Wal> get missingWals => _missingWals;
Comment on lines +27 to +29
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The _missingWals field is directly exposed via a getter. This could potentially allow external modification of the list, which might lead to unexpected behavior. Consider returning a copy of the list instead to ensure encapsulation.

- List<Wal> get missingWals => _missingWals;
+ List<Wal> get missingWals => List.from(_missingWals);


MemoryProvider() {
_wal.subscribe(this, this);
_preload();
}
Comment on lines +31 to +34
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

In the constructor, you are subscribing to the _wal service before calling _preload(). If the _wal service emits events between these two calls, those events will be missed. To avoid this race condition, consider preloading the data before subscribing to the service.

- MemoryProvider() {
-   _wal.subscribe(this, this);
-   _preload();
- }
+ MemoryProvider() {
+   _preload().then((_) {
+     _wal.subscribe(this, this);
+   });
+ }

Comment on lines +27 to +34
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The constructor of MemoryProvider now subscribes to the IWalService and preloads missing Wals. This is a good practice as it initializes necessary data at object creation. However, error handling is missing in _preload() method. If there's an error while getting missing Wals, it should be handled properly.

+  IWalService get _wal => ServiceManager.instance().wal;
+  List<Wal> _missingWals = [];
+  List<Wal> get missingWals => _missingWals;
+
+  MemoryProvider() {
+    _wal.subscribe(this, this);
+    _preload();
+  }
+
+  _preload() async {
+    try {
+      _missingWals = await _wal.getMissingWals();
+    } catch (e) {
+      // Handle or log the error here
+    }
+    notifyListeners();
+  }


_preload() async {
_missingWals = await _wal.getMissingWals();
notifyListeners();
}
Comment on lines +27 to +39
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The _preload method is called in the constructor of MemoryProvider. This could potentially lead to unhandled exceptions if _wal.getMissingWals() throws an error. It's generally not a good practice to call async methods without handling errors in constructors. Consider moving this logic to an initialization method and handle potential exceptions.

-  MemoryProvider() {
-    _wal.subscribe(this, this);
-    _preload();
-  }
+  MemoryProvider() {
+    _wal.subscribe(this, this);
+    init();
+  }
+
+  Future<void> init() async {
+    try {
+      await _preload();
+    } catch (e) {
+      // Handle or log exception
+    }
+  }


void addProcessingMemory(ServerMemory memory) {
processingMemories.add(memory);
notifyListeners();
Expand Down Expand Up @@ -288,6 +304,26 @@ class MemoryProvider extends ChangeNotifier {
@override
void dispose() {
_processingMemoryWatchTimer?.cancel();
_wal.unsubscribe(this);
super.dispose();
Comment on lines +307 to 308
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

Unsubscribing from the IWalService in the dispose method is a good practice to prevent memory leaks. No issues found.

+    _wal.unsubscribe(this);
+    super.dispose();

}

@override
void onNewMissingWal(Wal wal) async {
_missingWals = await _wal.getMissingWals();
notifyListeners();
}

@override
void onWalSynced(Wal wal, {ServerMemory? memory}) async {
_missingWals = await _wal.getMissingWals();
notifyListeners();
}
Comment on lines +312 to +321
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The methods onNewMissingWal and onWalSynced are updating _missingWals and notifying listeners. However, they are doing the same thing. Consider refactoring this to avoid code duplication. Also, add error handling when getting missing Wals.

+  @override
+  void onNewMissingWal(Wal wal) async {
+    await updateMissingWals();
+  }
+
+  @override
+  void onWalSynced(Wal wal, {ServerMemory? memory}) async {
+    await updateMissingWals();
+  }
+
+  Future<void> updateMissingWals() async {
+    try {
+      _missingWals = await _wal.getMissingWals();
+    } catch (e) {
+      // Handle or log the error here
+    }
+    notifyListeners();
+  }


@override
void onStatusChanged(WalServiceStatus status) {}

void syncWals() {
_wal.syncAll();
}
Comment on lines +324 to +328
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The onStatusChanged method is empty. If it's not used, consider removing it to avoid confusion. The syncWals method correctly triggers syncing of all Wals.

-  @override
-  void onStatusChanged(WalServiceStatus status) {}
+
+  void syncWals() {
+    _wal.syncAll();
+  }

}
9 changes: 8 additions & 1 deletion app/lib/services/services.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@ import 'package:flutter_background_service/flutter_background_service.dart';
import 'package:flutter_sound/flutter_sound.dart';
import 'package:friend_private/services/devices.dart';
import 'package:friend_private/services/sockets.dart';
import 'package:friend_private/services/wals.dart';

class ServiceManager {
late IMicRecorderService _mic;
late IDeviceService _device;
late ISocketService _socket;
late IWalService _wal;

static ServiceManager? _instance;

Expand All @@ -22,6 +24,7 @@ class ServiceManager {
);
sm._device = DeviceService();
sm._socket = SocketServicePool();
sm._wal = WalService();

return sm;
}
Expand All @@ -40,6 +43,8 @@ class ServiceManager {

ISocketService get socket => _socket;

IWalService get wal => _wal;

static void init() {
if (_instance != null) {
throw Exception("Service manager is initiated");
Expand All @@ -49,9 +54,11 @@ class ServiceManager {

Future<void> start() async {
_device.start();
_wal.start();
}

void deinit() {
void deinit() async {
await _wal.stop();
Comment on lines +57 to +61
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

There's a potential issue with the deinit method. It's declared as a synchronous method but you're using await inside it which is an asynchronous operation. This could lead to unexpected behavior. You should declare deinit as an asynchronous method.

-  void deinit() async {
+  Future<void> deinit() async {
     await _wal.stop();
     _mic.stop();
     _device.stop();
   }

_mic.stop();
_device.stop();
}
Expand Down
Loading
Loading