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

Queue incoming events #28

Merged
merged 2 commits into from
Jan 24, 2025
Merged

Queue incoming events #28

merged 2 commits into from
Jan 24, 2025

Conversation

aarthificial
Copy link
Contributor

  • Requests the mic access first, before initiating a connection
  • Queues any websocket events that may happen during AudioContext initialization

@aarthificial aarthificial requested a review from gmrchk January 24, 2025 18:33
try {
const parsedEvent = JSON.parse(event.data);
if (!isValidSocketEvent(parsedEvent)) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should actually report this in console? Not a point of the PR so we can skip.

Comment on lines +83 to +89
// some browsers won't allow calling getSupportedConstraints or enumerateDevices
// before getting approval for microphone access
const preliminaryInputStream = await navigator.mediaDevices.getUserMedia({
audio: true,
});
preliminaryInputStream?.getTracks().forEach(track => track.stop());

Copy link
Member

Choose a reason for hiding this comment

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

I think this was moved here to init the microphone before everything else, but we close it right after. I'm guessing it's solving the problem from your testing, but it's a bit weird that we'd close it here and then recreate shortly after without awaiting. Seems like it could still keep some issues.

Would it make sense to create Input first instead here? That one already had this check, and we can await Input creation before moving to anything else. This bit of the code would stay where it was, and the code related to input (and the comment) would stay in place where the input code was meant to be.

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, this is how it used to be before. But the input can now have a different sampling rate depending on the agent configuration, so we need to establish a connection first, before we know how to set up the streams.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha! Perhaps it could be within Input.prepare method or something, which would be called here? The code (at least with the current comment about getSupportedConstraints or enumerateDevices) doesn't make much sense here.

packages/client/src/index.ts Show resolved Hide resolved
packages/client/src/utils/connection.ts Outdated Show resolved Hide resolved
@aarthificial aarthificial merged commit 4c75006 into main Jan 24, 2025
3 checks passed
@aarthificial aarthificial deleted the queue-input-events branch January 24, 2025 23:50
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