Skip to content

[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

Merged
merged 11 commits into from
Aug 20, 2025

Conversation

ipavlidakis
Copy link
Contributor

@ipavlidakis ipavlidakis commented Aug 13, 2025

🔗 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

  • Join many calls (one after another) and ensure that your audio waveform always works as expected.

☑️ Contributor Checklist

  • I have signed the Stream CLA (required)
  • This change follows zero ⚠️ policy (required)
  • This change should receive manual QA
  • Changelog is updated with client-facing changes
  • New code is covered by unit tests
  • Comparison screenshots added for visual changes
  • Affected documentation updated (tutorial, CMS)

@ipavlidakis ipavlidakis self-assigned this Aug 13, 2025
@ipavlidakis ipavlidakis requested a review from a team as a code owner August 13, 2025 15:23
@ipavlidakis ipavlidakis added bug Something isn't working enhancement New feature or request labels Aug 13, 2025
Copy link

github-actions bot commented Aug 13, 2025

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

Copy link

github-actions bot commented Aug 13, 2025

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()

@Stream-SDK-Bot
Copy link
Collaborator

Stream-SDK-Bot commented Aug 13, 2025

SDK Size

title develop branch diff status
StreamVideo 8.18 MB 8.21 MB +24 KB 🟢
StreamVideoSwiftUI 2.29 MB 2.29 MB 0 KB 🟢
StreamVideoUIKit 2.41 MB 2.41 MB 0 KB 🟢
StreamWebRTC 9.85 MB 9.85 MB 0 KB 🟢

Copy link
Contributor

@martinmitrevski martinmitrevski left a 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.


// MARK: - Private Helpers

private func didUpdate(_ applicationState: ApplicationState) async {
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

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(
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@ipavlidakis ipavlidakis force-pushed the fix/local-audio-waveform-not-always-working branch 5 times, most recently from bc85466 to debd03b Compare August 14, 2025 16:06

import Foundation

protocol StoreNamespace {
Copy link
Contributor Author

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).

Copy link
Contributor

@martinmitrevski martinmitrevski left a 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks very nice!

@ipavlidakis ipavlidakis force-pushed the fix/local-audio-waveform-not-always-working branch from debd03b to 2536175 Compare August 18, 2025 15:04
@ipavlidakis ipavlidakis requested a review from Copilot August 19, 2025 09:57
Copy link

@Copilot Copilot AI left a 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] }
Copy link
Preview

Copilot AI Aug 19, 2025

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.

Suggested change
.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()
Copy link
Preview

Copilot AI Aug 19, 2025

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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
Copy link
Preview

Copilot AI Aug 19, 2025

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)
Copy link
Preview

Copilot AI Aug 19, 2025

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.

Copy link
Contributor

@martinmitrevski martinmitrevski left a 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.

// Copyright © 2025 Stream.io Inc. All rights reserved.
//

/// Base class for store middleware that intercept actions for side effects.
Copy link
Contributor

Choose a reason for hiding this comment

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

*intercepts

Copy link
Contributor

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 = []
Copy link
Contributor

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?

Copy link
Contributor Author

@ipavlidakis ipavlidakis Aug 20, 2025

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() {
Copy link
Contributor

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?

Copy link
Contributor Author

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() {
Copy link
Contributor

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 {
Copy link
Contributor

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()
Copy link
Contributor

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 {
Copy link
Contributor

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.

Copy link

@ipavlidakis ipavlidakis enabled auto-merge (squash) August 20, 2025 09:29
@ipavlidakis ipavlidakis merged commit 2f867a8 into develop Aug 20, 2025
12 checks passed
@ipavlidakis ipavlidakis deleted the fix/local-audio-waveform-not-always-working branch August 20, 2025 09:38
@Stream-SDK-Bot Stream-SDK-Bot mentioned this pull request Aug 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants