-
Notifications
You must be signed in to change notification settings - Fork 397
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
Offline Speech Recognition #2089 #2242
base: main
Are you sure you want to change the base?
Conversation
src/CommunityToolkit.Maui.Core/Essentials/SpeechToText/ISpeechToText.shared.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VladislavAntonyuk thanks for this. I think it looks great! I only had a few comments to get your thoughts on some things.
Finally, this might be outside of the scope of this PR but I wanted to raise it because I think it should be in a follow-up PR; I would love the ability to chain the 2 implementations together when registering with DI, something like:
builder.Services.AddSpeechToText(SpeechToText.Default).WithFallback(OfflineSpeechToText.Default);
And in theory developers could chain the other way round:
builder.Services.AddSpeechToText(OfflineSpeechToText.Default).WithFallback(SpeechToText.Default);
What do you think to the above? We might have to wrap this in another class rather than complicating the flow of the 2 current implementations so it probably is best in a follow-up PR.
src/CommunityToolkit.Maui.Core/Essentials/SpeechToText/ISpeechToText.shared.cs
Show resolved
Hide resolved
src/CommunityToolkit.Maui.Core/Essentials/SpeechToText/OfflineSpeechToText.shared.cs
Show resolved
Hide resolved
...mmunityToolkit.Maui.Core/Essentials/SpeechToText/OfflineSpeechToTextImplementation.shared.cs
Show resolved
Hide resolved
Thank you @bijington. Also, we need to provide the lifetime of the service. And what is under the hood of |
Yes I agree the developer will need to define the lifetime of the service which increases the complexity. Perhaps we could move the builder.Services.AddSingleton(OfflineSpeechToText.Default.WithFallback(SpeechToText.Default)); Then public static void ISpeechToTextExtensions
{
public ISpeechToText WithFallback(this ISpeechToText primary, ISpeechToText secondary)
{
return new PriorityBasedSpeechToText(primary, secondary);
}
}
internal class PriorityBasedSpeechToText : ISpeechToText
{
readonly ISpeechToText primaryService;
readonly ISpeechToText secondaryService;
public PriorityBasedSpeechToText(this ISpeechToText primary, ISpeechToText secondary)
{
primaryService = primary;
secondaryService = secondary;
}
public Task<SpeechToTextRecognitionResult> StartListenAsync()
{
// attempt primary, if fails fallback to secondary...
}
} It could well become more complicated with things like permissions, so we may have to request all permissions for both What do you think? |
I see pros and cons of such approach. We could technically hide the online/offline recognition in the implementation and keep single service. We had such implementation for Windows in our initial release. The main idea of Offline Speech To Text to allow developers explicitly specify the required implementation. We can open a discussion for the next month. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VladislavAntonyuk thanks for this! I have a few comments and I think they are mostly pretty minor
samples/CommunityToolkit.Maui.Sample/ViewModels/Essentials/OfflineSpeechToTextViewModel.cs
Outdated
Show resolved
Hide resolved
samples/CommunityToolkit.Maui.Sample/ViewModels/Essentials/OfflineSpeechToTextViewModel.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui.Core/Essentials/SpeechToText/ISpeechToText.shared.cs
Outdated
Show resolved
Hide resolved
…CommunityToolkit/Maui into 2089-offline-speech-recognition # Conflicts: # src/CommunityToolkit.Maui.Core/Essentials/SpeechToText/OfflineSpeechToTextImplementation.tizen.cs
3892a6e
to
67894fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @VladislavAntonyuk I have added some xml docs improvements but the rest looks good to me
src/CommunityToolkit.Maui.Core/Essentials/SpeechToText/ISpeechToText.shared.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui.Core/Essentials/SpeechToText/ISpeechToText.shared.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui.Core/Essentials/SpeechToText/ISpeechToText.shared.cs
Outdated
Show resolved
Hide resolved
samples/CommunityToolkit.Maui.Sample/ViewModels/Essentials/OfflineSpeechToTextViewModel.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Shaun Lawrence <[email protected]>
Co-authored-by: Shaun Lawrence <[email protected]>
…lineSpeechToTextViewModel.cs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Do we just need to write the docs? Is this something you are happy to do? If not I'm happy to write something
Thank you Shaun! I will create docs PR this weekend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
07ba0b4
to
7ddd7fe
Compare
will try again during this week, asap |
Description of Change
Added 2 new methods for Offline Speech Recognition, Removed ListenAsync method, as it is impossible (with current implementation) to correctly Stop Listening the recognition. I also added a new State, allowing us to see if Listening is Active, but Silience.
Linked Issues
PR Checklist
approved
(bug) orChampioned
(feature/proposal)main
at time of PRAdditional information