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

[MM-59866] Prevent failure if track context has no samples #29

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

streamer45
Copy link
Contributor

Summary

An interesting case happened during our last developers meeting. If a participant is unmuted when this job starts but mutes right before the matching recording job begins, there could be a situation where the OGG file for the (now muted) track is created but no audio packet is ever written to it. This would cause decoding to fail during the post-call process and the whole job to exit.

PR adds a check to detect if the context is empty (no samples) and avoids sending it. We also make the post-processing side a little more forgiving and just ignore empty audio.

Ticket Link

https://mattermost.atlassian.net/browse/MM-59866

@streamer45 streamer45 added the 2: Dev Review Requires review by a core committer label Jul 25, 2024
@streamer45 streamer45 added this to the v0.4.0 milestone Jul 25, 2024
@streamer45 streamer45 requested a review from cpoile July 25, 2024 09:22
@streamer45 streamer45 self-assigned this Jul 25, 2024
@@ -181,7 +189,7 @@ func (t *Transcriber) processLiveTrack(track trackRemote, sessionID string) {
}

var gap uint64
if ctx.startTS == 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing this since, in theory, startTS could be still 0 even after setting it the first time if we are very unlucky and the start time was set just now. Very unlikely but you never know :)

Comment on lines +51 to +59
dir, err := os.MkdirTemp("", "data")
if err != nil {
require.NoError(t, err)
}
os.Setenv("DATA_DIR", dir)
t.Cleanup(func() {
os.Unsetenv("DATA_DIR")
os.RemoveAll(dir)
})
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 is needed both to simplify local testing and also to fix an existing test that was passing purely because of a failure creating the OGG file.

Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

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

What an unlikely bug, nice find.

@streamer45 streamer45 added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Jul 25, 2024
@streamer45 streamer45 merged commit 05b7744 into master Jul 25, 2024
2 checks passed
@streamer45 streamer45 deleted the MM-59866 branch July 25, 2024 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants