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.

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

Accurate audio track counting #477

Merged
merged 6 commits into from
Sep 9, 2024
Merged

Conversation

hiroshihorie
Copy link
Member

@hiroshihorie hiroshihorie commented Sep 6, 2024

Not sure what can cause trackDidStop to be called unbalanced with trackDidStart but better guard this.
Accurate counting of audio tracks instead of counting publish / unpublish.

@hiroshihorie hiroshihorie changed the title Guard audio track counter Accurate audio track counting Sep 6, 2024
Copy link
Contributor

@bcherry bcherry left a comment

Choose a reason for hiding this comment

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

is it still possible for a track that's already stopped to call stopCapture? in the case everything is already at 0 this won't matter, but if if the values are greater than 0 it could cause miscounting.

a more durable approach may be to store sets of the active track ids, instead of counts, and it will prevent miscounts in the case of extra starts or stops for a single track.

(But this looks like it will fix the issue as reported, where we reached -1 before anything got published, so this should be good to merge)

@hiroshihorie
Copy link
Member Author

start/stopCapture is guarded by a trackState lock so it should be ok:

// Intended for child class to override
func startCapture() async throws {}
// Intended for child class to override
func stopCapture() async throws {}
@objc
public final func start() async throws {
guard _state.trackState != .started else {
log("Already started", .warning)
return
}
try await startCapture()
if self is RemoteTrack { try await enable() }
_state.mutate { $0.trackState = .started }
}
@objc
public final func stop() async throws {
guard _state.trackState != .stopped else {
log("Already stopped", .warning)
return
}
try await stopCapture()
if self is RemoteTrack { try await disable() }
_state.mutate { $0.trackState = .stopped }
}

But I agree there with more work I can make it more solid.

@bcherry
Copy link
Contributor

bcherry commented Sep 6, 2024

@hiroshihorie yeah that's perfect - this should probably work as it is then

@hiroshihorie
Copy link
Member Author

Ok when I look again it's still not race proof stop() could get executed concurrently

@hiroshihorie hiroshihorie merged commit 4ca5fa2 into main Sep 9, 2024
12 checks passed
@hiroshihorie hiroshihorie deleted the hiroshi/guard-track-counter branch September 9, 2024 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants