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

Feat: Record Plugin - Choose Mic #3256

Merged
merged 5 commits into from
Oct 14, 2023

Conversation

tscritch
Copy link
Contributor

@tscritch tscritch commented Oct 13, 2023

Short description

Resolves #3245
Open to feedback!

Implementation details

  • Adds options to startMic and startRecording to use a specific mic (via deviceId)
  • Add function to get a list of available audio devices getAvailableAudioDevices

How to test it

  1. Open the Record example.
  2. If you haven't allowed mic permissions on the site before, for this example you will need to hit the record button first, allow permissions and refresh the page for the list to be updated.
  3. You should be able to select a mic from the list.
  4. Hit record and test the mic.

In Chrome, you can reset the "Allow Mic" permissions by going to Settings > Privacy and Security > Site settings > Microphone > Click trash icon on wavesurfer.xyz or localhost:9090.

Screenshots

000219 - 2023-10-13_01 34 23

Checklist

  • This PR is covered by e2e tests
  • It introduces no breaking API changes

Comment on lines 179 to 190
// Request access to the microphone before enumerating devices, otherwise
// the browser will not return any device IDs
let stream: MediaStream
try {
stream = await navigator.mediaDevices.getUserMedia({
audio: true,
})
} catch (err) {
throw new Error('Error getting audio devices: ' + (err as Error).message)
}
this.stream = stream
this.stopMic()
Copy link
Owner

Choose a reason for hiding this comment

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

Looks good overall, thank you!

These lines are almost the same as in startMic, should we just call it here? Or maybe throw an error like Please start the microphone first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly. I was concerned about how these lines from startMic would affect it? It doesn't need to render the stream when calling the function. What do you think?

    const onDestroy = this.renderMicStream(stream)
    this.subscriptions.push(this.once('destroy', onDestroy))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could also use this: https://developer.mozilla.org/en-US/docs/Web/API/Permissions/query
Then if no permission - call startMic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for all the suggestions here. It would be nice if this was actually a static function and didn't touch the main class properties. It would keep similar code here but just use the local variable and use similar code to stopMic to ensure the steam gets deallocated.

Copy link
Contributor Author

@tscritch tscritch Oct 14, 2023

Choose a reason for hiding this comment

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

Landed on removing starting a stream and just adding better docs.
The permissions api I shared doesn't support a microphone query in Firefox 🤷🏻‍♂️

@tscritch
Copy link
Contributor Author

@katspaugh updated. Let me know your thoughts 🙏🏻

@tscritch tscritch changed the title Record Plugin: Choose Mic Feat: Record Plugin - Choose Mic Oct 14, 2023
Copy link
Owner

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

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

Thanks!

@katspaugh katspaugh merged commit 284c424 into katspaugh:main Oct 14, 2023
@hannahblair
Copy link
Contributor

hannahblair commented Oct 16, 2023

@katspaugh @tscritch I'm getting an error when trying to use getAvailableAudioDevices: Uncaught TypeError: record.getAvailableAudioDevices is not a function. It's occurring locally and in the Record example in the docs.

P.S. Thanks for implementing this super useful feature!

const micSelect = document.querySelector('#mic-select')
{
// Mic selection
record.getAvailableAudioDevices().then((devices) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to be RecordPlugin

Copy link
Owner

Choose a reason for hiding this comment

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

Good catch! Could you make a PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

On it!

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

3 participants