-
Notifications
You must be signed in to change notification settings - Fork 28
[Fix]Local audio waveform not always working #912
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.
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
[Fix]Local audio waveform not always working #912
Conversation
Generated by 🚫 Danger |
Public Interface public struct LogSubsystem: OptionSet, CustomStringConvertible, Sendable
- public var description: String
+ public static let audioRecording
-
+ public var description: String
-
+
- public init(rawValue: Int)
+
+ public init(rawValue: Int)
open class StreamCallAudioRecorder: @unchecked Sendable
- open private lazy var metersPublisher: AnyPublisher<Float, Never>
+ @Published open private var meters: Float
-
+ @Published open private var isRecording: Bool
-
+
- public init(filename: String)
+
-
+ public convenience init()
-
+
- open func startRecording(ignoreActiveCall: Bool = false)async
+
- open func stopRecording()async
+ open func startRecording(ignoreActiveCall: Bool = false)
+ open func stopRecording() |
SDK Size
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, left few comments. Let's test this on the calls.
...Video/Utils/AudioSession/AudioRecorder/CallAudioRecordingStore/CallAudioRecordingStore.swift
Outdated
Show resolved
Hide resolved
Sources/StreamVideo/Utils/AudioSession/AudioRecorder/AVAudioRecorderBuilder.swift
Outdated
Show resolved
Hide resolved
|
||
// MARK: - Private Helpers | ||
|
||
private func didUpdate(_ applicationState: ApplicationState) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like this param is unused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't but as the triggering event is the applicationState change, i have added it for reference. Do you prefer to remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be confusing yes - maybe just rename the method to didUpdateApplicationState
?
.sink { [weak self] _ in self?.store?.dispatch(.setIsRecording(false)) } | ||
} | ||
|
||
func apply( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this method? I mostly see no-op implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the method is necessary in the Redux scope. The reason we aren't using it in a few of the current middleware is because those instances are being used to synchronise values from outside the store, with the store.
...Video/Utils/AudioSession/AudioRecorder/CallAudioRecordingStore/CallAudioRecordingStore.swift
Outdated
Show resolved
Hide resolved
bc85466
to
debd03b
Compare
|
||
import Foundation | ||
|
||
protocol StoreNamespace { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This setup will allow us not only to instantiate stores easily (and configure them) but also to mock them for unit tests (as we can replace the executor with a new implementation and spy every execution).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything here looks like poetry, great stuff! Let's test this a bit and add unit tests before merging.
import Combine | ||
import Foundation | ||
|
||
final class Store<Namespace: StoreNamespace>: @unchecked Sendable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very nice!
Create Store component and reuse
debd03b
to
2536175
Compare
There was a problem hiding this comment.
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 refactors the StreamCallAudioRecorder to use a new Store-based architecture, addressing the issue where local audio waveform visualization wasn't always working properly by providing better alignment between the AudioRecorder and the underlying RTCAudioStore.
- Introduces a new Store/Reducer/Middleware pattern for managing audio recording state
- Completely rewrites StreamCallAudioRecorder to use the new store architecture
- Replaces the old
StreamActiveCallProvider
dependency injection pattern with direct StreamVideo state observing
Reviewed Changes
Copilot reviewed 56 out of 56 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
StreamCallAudioRecorder.swift | Refactored to use new Store-based architecture with reactive state management |
Store framework files | New Store/Reducer/Middleware pattern implementation for state management |
Test files | Updated and added comprehensive tests for the new Store architecture and audio recorder |
IdleTimerAdapter.swift | Updated to observe StreamVideo state directly instead of using provider pattern |
Mock files | Updated mocks to work with new Store architecture |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
_ keyPath: KeyPath<Namespace.State, V> | ||
) -> AnyPublisher<V, Never> { | ||
stateSubject | ||
.map { $0[keyPath: keyPath] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The publisher method creates a new publisher chain on every call without any deduplication. Consider using .removeDuplicates()
on the mapped keyPath value to prevent unnecessary emissions when the specific property hasn't changed.
.map { $0[keyPath: keyPath] } | |
.map { $0[keyPath: keyPath] } | |
.removeDuplicates() |
Copilot uses AI. Check for mistakes.
// Given | ||
let iterations = 1000 | ||
nonisolated(unsafe) var capturedStates: [RaceTestState] = [] | ||
let lock = UnfairQueue() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test uses UnfairQueue
for synchronization but also uses nonisolated(unsafe)
for the capturedStates
array. This combination could lead to data races. Consider using @Atomic
wrapper or ensuring all access to capturedStates
goes through the lock.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The usage of propertyWrappers inside a function body feels weird tbh. That's why i'm using it. The end result should be the same though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nah, don't listen to it 😅
/// Measures publisher notification performance. | ||
func test_measurePublisherPerformance() { | ||
let iterations = 1000 | ||
var receivedCount = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The receivedCount
variable is accessed from multiple publisher callbacks without proper synchronization. This could lead to race conditions. Consider using @Atomic
wrapper or proper locking mechanism.
Copilot uses AI. Check for mistakes.
|
||
init() { | ||
super.init(filename: "mock_file") | ||
super.init(mockStore) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mock automatically sets itself as the injected value in the initializer, which is a side effect that makes testing less predictable. Consider making this explicit by requiring callers to call a separate setup method or providing a factory method.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice and elegant implementation! Left few small comments, but there's no need to re-review.
Let's test this a bit more before merging.
...es/StreamVideo/Utils/AudioSession/AudioRecorder/Extensions/AVAudioRecorder+Convenience.swift
Outdated
Show resolved
Hide resolved
// Copyright © 2025 Stream.io Inc. All rights reserved. | ||
// | ||
|
||
/// Base class for store middleware that intercept actions for side effects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*intercepts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few typos here, which I think you do on purpose, since you consider middleware as plural, but in general it should be singular.
Anyway, not important for sure, feel free to ignore these.
self.identifier = identifier | ||
stateSubject = .init(initialState) | ||
self.reducers = reducers | ||
self.middleware = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this set to empty array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Middleware requires installation when they are added to the store (the dispatcher and stateProvider). This logic exists in the addMiddleware method and i didn't want to duplicate it. Let me add a comment to clarify the intent.
// MARK: - Complex State Tests | ||
|
||
/// Measures performance with complex state updates. | ||
func test_measureComplexStateUpdates() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great! Just one question - do you think these might become flaky depending on which machine they're run on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that on CI we use m machines i think we are safe. But even if it starts happening we can adapt the acceptable threshold.
// MARK: - Memory Performance Tests | ||
|
||
/// Tests memory usage with large state. | ||
func test_memoryUsageWithLargeState() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another clever one!
/// - Middleware state access during updates | ||
/// - Publisher timing issues | ||
/// - Cleanup during active operations | ||
final class Store_RaceConditionTests: XCTestCase, @unchecked Sendable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Store testing final boss
// Given | ||
let iterations = 1000 | ||
nonisolated(unsafe) var capturedStates: [RaceTestState] = [] | ||
let lock = UnfairQueue() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nah, don't listen to it 😅
// MARK: - State Read/Write Race Tests | ||
|
||
/// Tests reading state while it's being updated. | ||
func test_stateReadDuringWrite_isConsistent() async throws { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests like these are really useful. Thinking if we should generalize them at some point, to test race conditions elsewhere (other parts of the codebase, other products). Maybe move them to core testing utils.
Anyway, not in scope for this PR.
|
🔗 Issue Links
Resolves https://linear.app/stream/issue/IOS-1078/audio-waveform-isnt-always-updating
🎯 Goal
This revision aims to provide alignment between the AudioRecorder - that is being used to provide the local participant's waveform visualisation - and the underline RTCAudioStore.
🛠 Implementation
The StreamCallAudioRecorder has been updated and linked to the underline RTCAudioStore.
🧪 Manual Testing Notes
☑️ Contributor Checklist