Jzecm wal local sync 958 #992
Conversation
|
WalkthroughThis update introduces conditional checks for codec types and modifies the logic for frame synchronization in the Changes
Uplevel your code reviews with Entelligence.AI ProEntelligence.AI ProIf you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. Moreover, Entelligence.AI Pro is free for open source projects. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (2)
- app/lib/providers/capture_provider.dart (1)
- app/lib/services/wals.dart (7)
Review comments generated (9)
- Review: 8
- LGTM: 1
Tips
Chat with Entelligence.AI Bot (@Entelligence.AI)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@Entelligence.AIin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@Entelligence.AI: ignoreanywhere in the PR description to pause further reviews from the bot.
| // support: opus | ||
| var isWalSupported = codec == BleAudioCodec.opus; | ||
| if (isWalSupported) { | ||
| wal.onByteStream(value); | ||
| } |
There was a problem hiding this comment.
The check for isWalSupported is a good addition to ensure that the codec type is supported before invoking wal.onByteStream(value). However, it would be better to return early if the codec is not supported. This way, we avoid unnecessary operations and improve code readability.
- var isWalSupported = codec == BleAudioCodec.opus;
- if (isWalSupported) {
- wal.onByteStream(value);
- }
+ if (codec != BleAudioCodec.opus) {
+ return;
+ }
+ wal.onByteStream(value);| // synced | ||
| if (isWalSupported) { | ||
| wal.onBytesSync(value); | ||
| } |
There was a problem hiding this comment.
Similar to the previous comment, it's good to see a check for isWalSupported before invoking wal.onBytesSync(value). But again, consider returning early if the codec is not supported.
- if (isWalSupported) {
- wal.onBytesSync(value);
- }
+ if (codec != BleAudioCodec.opus) {
+ return;
+ }
+ wal.onBytesSync(value);Please note that these changes assume that there are no other operations after these checks that need to be executed regardless of the codec type. If there are, you might want to adjust the logic accordingly.
| @@ -74,7 +71,7 @@ class Wal { | |||
| this.filePath, | |||
| this.data = const []}); | |||
|
|
|||
| get seconds => ChunkSizeInSeconds; | |||
| get seconds => chunkSizeInSeconds; | |||
There was a problem hiding this comment.
The seconds getter has been updated to use the new constant chunkSizeInSeconds. This change is consistent with the update in the constants section. However, it's important to ensure that this new value doesn't negatively impact the system's performance or functionality.
- get seconds => ChunkSizeInSeconds;
+ get seconds => chunkSizeInSeconds;
app/lib/services/wals.dart
Outdated
| @@ -166,6 +163,7 @@ class WalService implements IWalService, IWalSocketServiceListener { | |||
| } | |||
|
|
|||
| var framesPerSeconds = 100; | |||
| var lossesThreshold = 1 * framesPerSeconds; // 1s | |||
There was a problem hiding this comment.
A new variable lossesThreshold has been introduced. It seems to be used for determining a threshold for frame losses. Please ensure that this new logic does not introduce any unexpected behavior in the frame processing logic.
+ var lossesThreshold = 1 * framesPerSeconds; // 1s
There was a problem hiding this comment.
Files selected (1)
- app/lib/services/wals.dart (7)
Review comments generated (7)
- Review: 6
- LGTM: 1
Tips
Chat with Entelligence.AI Bot (@Entelligence.AI)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@Entelligence.AIin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@Entelligence.AI: ignoreanywhere in the PR description to pause further reviews from the bot.
| import 'dart:io'; | ||
| import 'dart:math'; | ||
| import 'dart:typed_data'; | ||
|
|
||
| import 'package:flutter/foundation.dart'; | ||
| 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/message_event.dart'; | ||
| import 'package:friend_private/providers/message_provider.dart'; | ||
| import 'package:friend_private/services/sockets/transcription_connection.dart'; | ||
| import 'package:friend_private/services/sockets/wal_connection.dart'; | ||
| import 'package:path_provider/path_provider.dart'; | ||
|
|
||
| const ChunkSizeInSeconds = 7; // 30 | ||
| const FlushIntervalInSeconds = 15; //300 | ||
| const chunkSizeInSeconds = 30; | ||
| const flushIntervalInSeconds = 60; |
There was a problem hiding this comment.
The removal of unused imports and constants is a good practice for code cleanliness and readability. The renaming of the constants ChunkSizeInSeconds and FlushIntervalInSeconds to chunkSizeInSeconds and flushIntervalInSeconds follows Dart's naming conventions for constants, which is also a good change.
- import 'dart:ffi';
- import 'dart:math';
- const ChunkSizeInSeconds = 7; // 30
- const FlushIntervalInSeconds = 15; //300
+ const chunkSizeInSeconds = 30;
+ const flushIntervalInSeconds = 60;| _chunkingTimer = Timer.periodic(const Duration(seconds: chunkSizeInSeconds), (t) async { | ||
| await _chunk(); | ||
| }); | ||
| _flushingTimer = Timer.periodic(const Duration(seconds: FlushIntervalInSeconds), (t) async { | ||
| _flushingTimer = Timer.periodic(const Duration(seconds: flushIntervalInSeconds), (t) async { |
There was a problem hiding this comment.
The timer intervals have been updated to use the renamed constants. This maintains consistency in the codebase.
- _chunkingTimer = Timer.periodic(const Duration(seconds: ChunkSizeInSeconds), (t) async {
- _flushingTimer = Timer.periodic(const Duration(seconds: FlushIntervalInSeconds), (t) async {
+ _chunkingTimer = Timer.periodic(const Duration(seconds: chunkSizeInSeconds), (t) async {
+ _flushingTimer = Timer.periodic(const Duration(seconds: flushIntervalInSeconds), (t) async {| @@ -166,6 +163,7 @@ class WalService implements IWalService, IWalSocketServiceListener { | |||
| } | |||
|
|
|||
| var framesPerSeconds = 100; | |||
| var lossesThreshold = 3 * framesPerSeconds; // 3s | |||
There was a problem hiding this comment.
The introduction of lossesThreshold improves the readability of the code by making it clear what this value represents. It also makes it easier to adjust this value in the future if needed.
+ var lossesThreshold = 3 * framesPerSeconds; // 3s| var low = high - framesPerSeconds * chunkSizeInSeconds; | ||
| if (low < 0) { | ||
| low = 0; | ||
| } | ||
| var synced = true; | ||
| var losses = 0; | ||
| var chunk = _frames.sublist(low, high); | ||
| for (var f in chunk) { | ||
| var head = f.sublist(0, 3); | ||
| var seq = Uint8List.fromList(head..add(0)).buffer.asByteData().getInt32(0); | ||
| if (!_syncFrameSeq.contains(seq)) { | ||
| synced = false; | ||
| break; | ||
| losses++; | ||
| if (losses >= lossesThreshold) { | ||
| synced = false; | ||
| break; | ||
| } |
There was a problem hiding this comment.
The logic for frame synchronization has been adjusted. Now, instead of marking a chunk as unsynced as soon as a single frame is not found in _syncFrameSeq, it allows for up to lossesThreshold missing frames before marking the chunk as unsynced. This could potentially improve the robustness of the synchronization process against minor data losses.
- if (!_syncFrameSeq.contains(seq)) {
- synced = false;
- break;
+ if (!_syncFrameSeq.contains(seq)) {
+ losses++;
+ if (losses >= lossesThreshold) {
+ synced = false;
+ break;| @@ -218,7 +220,7 @@ class WalService implements IWalService, IWalSocketServiceListener { | |||
| } | |||
|
|
|||
| // next | |||
| timerEnd -= ChunkSizeInSeconds; | |||
| timerEnd -= chunkSizeInSeconds; | |||
There was a problem hiding this comment.
The timer end calculation has been updated to use the renamed constant, maintaining consistency in the codebase.
- timerEnd -= ChunkSizeInSeconds;
+ timerEnd -= chunkSizeInSeconds;| Future stop() async { | ||
| _socket?.stop(); | ||
|
|
||
| debugPrint("wal service stop"); | ||
| _chunkingTimer?.cancel(); | ||
| _flushingTimer?.cancel(); | ||
|
|
There was a problem hiding this comment.
The removal of the debug print statement improves the cleanliness of the code. The rest of the changes are consistent with the previous state of the code.
- debugPrint("wal service stop");
Summary by Entelligence.AI
WalServiceclass for better performance.wal.onByteStreamandwal.onBytesSyncfunctions based on the codec type.ChunkSizeInSecondsandFlushIntervalInSecondstochunkSizeInSecondsandflushIntervalInSecondsfor consistency with Dart naming conventions.