Skip to content

Conversation

lugia19
Copy link
Contributor

@lugia19 lugia19 commented Jan 22, 2024

Proposed changes

Refactored the Microphone class:

  • Add the ability to select a playback device instead of always using the default one
  • Utilize PyAudio's callback mode instead of a While True loop (more appropriate for this usecase)
  • Change self.exit to be an event rather than a bool with a lock
  • Only check for push_callback being a coroutine during init, and wrap to be synchronous it if it is

Types of changes

What types of changes does your code introduce to the Deepgram Python SDK?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update or tests (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING doc
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Further comments

N/A

@davidvonthenen
Copy link
Contributor

Hi @lugia19

Thanks for this PR. This looks pretty interesting! Will check this out and get back to provide some feedback!

@davidvonthenen
Copy link
Contributor

Also, just a heads up @lugia19 I haven't taken a look, but just a reminder that interfaces cannot change until a major release meaning how a user implements their code should not break (need to preserve backward compatibility).

If we need to break the interfaces, we can do something special for the microphone class since they aren't really a part of the API. If it needs to happen (ie breaking changes), then we probably should discuss.

I see you are still making changes, ping me when ready to review.

@lugia19
Copy link
Contributor Author

lugia19 commented Jan 22, 2024

@dvonthenen Oh it's ready, that was the last change I had to do since I realized I implemented the asyncio callback stuff in a non-thread safe way.

As far as the interface, I'm aware - I made sure the interface would remain the same (bar the new optional argument for the init to set the input device, but I made it so by default not including it maintains the same behavior as before). There shouldn't be any differences.

@davidvonthenen
Copy link
Contributor

Cool. Will take a look when I get a free moment.

Copy link
Contributor

@davidvonthenen davidvonthenen left a comment

Choose a reason for hiding this comment

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

Very cool! Thanks for this contribution! Left some feedback for you. If you can also squash commits after updating, that would be awesome!

Copy link
Contributor

@davidvonthenen davidvonthenen left a comment

Choose a reason for hiding this comment

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

Few other things I just noticed and we are good to merge.

Improve handling of coroutine push_callback with asyncio
Add parameter input_device_index instead of always defaulting to the default input device
Make self.exit an event instead of a bool+lock
Copy link
Contributor

@davidvonthenen davidvonthenen left a comment

Choose a reason for hiding this comment

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

Sweeeeet! Yay!

@davidvonthenen
Copy link
Contributor

Thanks for your help with this @lugia19 !

@davidvonthenen davidvonthenen merged commit 8c88a38 into deepgram:main Jan 25, 2024
@lugia19 lugia19 deleted the refactor-microphone-callback branch January 25, 2024 17:20
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