[Voice Typing] add functionality to pipe audio level updates to waveform UI [chromium/src : main]
Amya Singhal (Gerrit)
Amya Singhal added 1 comment![]( "Open in Gerrit")
[Voice Typing] add functionality to pipe audio level updates to waveform UIinstead of Layered Forwarding, approaching this with direct Backend-to-UI 'Fast Path' using base::RepeatingCallbackList in DictationMultiplexer. Helps bypass business logic and pipe real-time audio updates directly to WaveformView.
Reason: Prevents interface pollution in core business layers by keeping high-frequency audio updates isolated as a strict UI-only concern.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Amya Singhal (Gerrit)
Amya Singhal added 1 comment![]( "Open in Gerrit")
base::RepeatingCallbackList<void(float)> audio_level_callback_list_;Not sure if I should use single callback approach or keep it to CallbackListSubscription. I did the current approach because it makes it almost impossible to create dangling pointers.
@bo...@chromium.org let me know your thoughts on this.
Related details
- David Bokan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin McNee (Gerrit)
Kevin McNee added 1 comment![]( "Open in Gerrit")
class DictationMultiplexer {It doesn't look like we need to go through the multiplexer, since it's not tied to a stream. Could we do this on DictationKeyedService?
Related details
- Amya Singhal
- David Bokan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin McNee (Gerrit)
Kevin McNee added 1 comment![]( "Open in Gerrit")
#include "chrome/browser/dictation/dictation_multiplexer.h"Also I'm not sure this include is allowed. What you could do instead is create a delegate interface (like "AudioLevelProvider" or something) that lives in this views/dictation directory. And then have the class in c/b/dictation implement it.
Amya Singhal (Gerrit)
Amya Singhal added 2 comments![]( "Open in Gerrit")
It doesn't look like we need to go through the multiplexer, since it's not tied to a stream. Could we do this on DictationKeyedService?
you are right, I added this here because I was receiving streamID with audio_level before, followed your suggestion above and created ```AudioLevelProvider```
#include "chrome/browser/dictation/dictation_multiplexer.h"Also I'm not sure this include is allowed. What you could do instead is create a delegate interface (like "AudioLevelProvider" or something) that lives in this views/dictation directory. And then have the class in c/b/dictation implement it.
- Kevin McNee
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bokan (Gerrit)
David Bokan added 5 comments![]( "Open in Gerrit")
not a strong preference but IMHO it'd be simpler to just add an UpdateAudioLevel method on the Ui interface and on the DictationBubbleUi and just have that call the equivalent method on the Waveform.
"//chrome/browser/ui/views/dictation",Can you make this a regular `deps` or does something complain?
bool DictationKeyedService::UpdateAudioLevel(float audio_level) {
audio_level_callback_list_.Notify(audio_level);
return true;
}
base::CallbackListSubscription DictationKeyedService::AddAudioLevelCallback(
base::RepeatingCallback<void(float)> callback) {
return audio_level_callback_list_.Add(std::move(callback));
}These should be in SessionController. DictationKeyedSerivce is more for browser-integration. This is a core dictation functionality which is what SessionController is for. i.e. if the feature was migrated to iOS (which doesn't have chrome/browser) we would still want the AudioLevel functionality of SessionController.
You can keep UpdateAudioLevel here but it should forward the call to the SessionController if one is available
DictationKeyedService* service =
DictationKeyedService::Get(window->GetProfile());
CHECK(service);This class shouldn't know about DictationKeyedService, it should interact only with the SessionUiDelegate (which is the SessionController)
explicit WaveformView(AudioLevelProvider& provider);Do we need to do this through a callback? `SetAudioLevel` is already public, can the BubbleUi just call SetAudioLevel whenever an update arrives?
Related details
- Amya Singhal
- Kevin McNee
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Amya Singhal (Gerrit)
Amya Singhal voted and added 5 comments![]( "Open in Gerrit")
Votes added by Amya Singhal
| Commit-Queue | +0 |
5 comments
Can you make this a regular `deps` or does something complain?
as we removed AudioLevelProvider, we can move it from ```public_deps``` to ```deps```
bool DictationKeyedService::UpdateAudioLevel(float audio_level) {
audio_level_callback_list_.Notify(audio_level);
return true;
}
base::CallbackListSubscription DictationKeyedService::AddAudioLevelCallback(
base::RepeatingCallback<void(float)> callback) {
return audio_level_callback_list_.Add(std::move(callback));
}These should be in SessionController. DictationKeyedSerivce is more for browser-integration. This is a core dictation functionality which is what SessionController is for. i.e. if the feature was migrated to iOS (which doesn't have chrome/browser) we would still want the AudioLevel functionality of SessionController.
You can keep UpdateAudioLevel here but it should forward the call to the SessionController if one is available
makes sense.
the new flow looks like ```DictationKeyedService::UpdateAudioLevel``` --> ```SessionController::UpdateAudioLevel``` --> ```SessionUi::UpdateAudioLevel``` --> ```DictationBubbleUi::UpdateAudioLevel``` --> ```WaveformView::SetAudioLevel```
base::RepeatingCallbackList<void(float)> audio_level_callback_list_;Not sure if I should use single callback approach or keep it to CallbackListSubscription. I did the current approach because it makes it almost impossible to create dangling pointers.
@bo...@chromium.org let me know your thoughts on this.
Marked as resolved.
DictationKeyedService* service =
DictationKeyedService::Get(window->GetProfile());
CHECK(service);This class shouldn't know about DictationKeyedService, it should interact only with the SessionUiDelegate (which is the SessionController)
done
Do we need to do this through a callback? `SetAudioLevel` is already public, can the BubbleUi just call SetAudioLevel whenever an update arrives?
- David Bokan
- Kevin McNee
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin McNee (Gerrit)
Kevin McNee voted and added 7 comments![]( "Open in Gerrit")
Votes added by Kevin McNee
| Code-Review | +1 |
7 comments
return true;nit: At a glance, I don't see anything check the return value and it always returns true anyway. This method could return void.
service->UpdateAudioLevel(static_cast<float>(params->audio_level));Let's also check that this value is in a valid range and respond with an error if not.
#include "base/functional/bind.h"nit: Appears unused.
// Expose a public hook to drive the wave with real mic volume (0.0 to 1.0).Pre-existing nit: This sounds like a TODO rather than a description of the function. I'd just say "Drives the wave..."
#include "base/functional/bind.h"nit: Appears unused.
// Updates the audio level of the stream.Let's also document the valid values for the audio level.
Related details
- Amya Singhal
- David Bokan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bokan (Gerrit)
David Bokan added 8 comments![]( "Open in Gerrit")
few nits + what Kevin said but otherwise LGTM
+tjudkins@ for extensions API
+jlulejian@ for enums.xml
// Broadcasts audio level update to subscribers.Update comment - updates audio level in the current session
if (bubble_ui_) {nit: This is never null - it's created in the constructor and never cleared.
service->UpdateAudioLevel(static_cast<float>(params->audio_level));they shouldn't happen but use `base::saturated_cast` to handle any weird edge cases should they crop up
I think we need a test in this file corresponding to the `setAudioLevel` call in test.js
// Calling this will automatically disable the internal speech simulation.We should remove this now that we have real input (the comment and the functionality)
float audio_level() const { return audio_level_; }appears used only in testing, name it `audio_level_for_testing`
constexpr float kBoostFactor = 10.0f;Add a description of what this is and where it comes from - I'm guessing this is experimentally determined to adjust the 0-1 audio level into a value that works well for the visualization. If so that's good to document in a comment so readers know this is arbitrary and used to adjust visual polish.
Related details
- Amya Singhal
- Justin Lulejian
- Tim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Lulejian (Gerrit)
Justin Lulejian voted and added 1 comment![]( "Open in Gerrit")
Votes added by Justin Lulejian
| Code-Review | +1 |
1 comment
lgtm tools/metrics/histograms/metadata/extensions/enums.xml
Related details
- Amya Singhal
- Tim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Amya Singhal (Gerrit)
Amya Singhal added 13 comments![]( "Open in Gerrit")
Update comment - updates audio level in the current session
done
nit: At a glance, I don't see anything check the return value and it always returns true anyway. This method could return void.
good catch, made the method void
nit: This is never null - it's created in the constructor and never cleared.
makes sense, removed the check
service->UpdateAudioLevel(static_cast<float>(params->audio_level));Let's also check that this value is in a valid range and respond with an error if not.
added the check and error
service->UpdateAudioLevel(static_cast<float>(params->audio_level));they shouldn't happen but use `base::saturated_cast` to handle any weird edge cases should they crop up
done
I think we need a test in this file corresponding to the `setAudioLevel` call in test.js
In this browser test setup, we bypass StartSession to register the mock provider directly, meaning DictationKeyedService::session_ is null. As a result, UpdateAudioLevel is a no-op in C++ here and cannot be asserted upon.
The API binding itself is smoke-tested via the call in test.js (which succeeds without throwing), while the actual backend propagation logic is thoroughly verified in dictation_keyed_service_unittest.cc
#include "base/functional/bind.h"Amya Singhalnit: Appears unused.
removed
// Calling this will automatically disable the internal speech simulation.We should remove this now that we have real input (the comment and the functionality)
done
// Expose a public hook to drive the wave with real mic volume (0.0 to 1.0).Pre-existing nit: This sounds like a TODO rather than a description of the function. I'd just say "Drives the wave..."
fixed the comment
appears used only in testing, name it `audio_level_for_testing`
done
#include "base/functional/bind.h"Amya Singhalnit: Appears unused.
removed
Add a description of what this is and where it comes from - I'm guessing this is experimentally determined to adjust the 0-1 audio level into a value that works well for the visualization. If so that's good to document in a comment so readers know this is arbitrary and used to adjust visual polish.
added a comment
Let's also document the valid values for the audio level.
- David Bokan
- Justin Lulejian
- Kevin McNee
- Tim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bokan (Gerrit)
David Bokan added 1 comment![]( "Open in Gerrit")
Amya SinghalI think we need a test in this file corresponding to the `setAudioLevel` call in test.js
In this browser test setup, we bypass StartSession to register the mock provider directly, meaning DictationKeyedService::session_ is null. As a result, UpdateAudioLevel is a no-op in C++ here and cannot be asserted upon.
The API binding itself is smoke-tested via the call in test.js (which succeeds without throwing), while the actual backend propagation logic is thoroughly verified in dictation_keyed_service_unittest.cc
I see - please file a bug and add a TODO to update these tests to use a session like a real stream provider would.
You could manually create a session here but I think the issue is you have no way to read the set value or expect a call...I think the right way to do this would be to use a MockDictationKeyedService which allows a test to mock the SessionUI and then add an EXPECT_CALL on UpdateAudioLevel - that would require the above bug to be fixed. Lets leave a bug+TODO here to add a test for this functionality once that's fixed. I'm tracking any missing test coverage under bug b/528859214 so you can make it a child of that.
Related details
- Amya Singhal
- Justin Lulejian
- Kevin McNee
- Tim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Amya Singhal (Gerrit)
Amya Singhal added 1 comment![]( "Open in Gerrit")
Amya SinghalI think we need a test in this file corresponding to the `setAudioLevel` call in test.js
David BokanIn this browser test setup, we bypass StartSession to register the mock provider directly, meaning DictationKeyedService::session_ is null. As a result, UpdateAudioLevel is a no-op in C++ here and cannot be asserted upon.
The API binding itself is smoke-tested via the call in test.js (which succeeds without throwing), while the actual backend propagation logic is thoroughly verified in dictation_keyed_service_unittest.cc
I see - please file a bug and add a TODO to update these tests to use a session like a real stream provider would.
You could manually create a session here but I think the issue is you have no way to read the set value or expect a call...I think the right way to do this would be to use a MockDictationKeyedService which allows a test to mock the SessionUI and then add an EXPECT_CALL on UpdateAudioLevel - that would require the above bug to be fixed. Lets leave a bug+TODO here to add a test for this functionality once that's fixed. I'm tracking any missing test coverage under bug b/528859214 so you can make it a child of that.
- David Bokan
- Justin Lulejian
- Kevin McNee
- Tim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bokan (Gerrit)
David Bokan voted Code-Review+1![]( "Open in Gerrit")
- Amya Singhal
- Justin Lulejian
- Kevin McNee
- Tim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Lulejian (Gerrit)
Justin Lulejian voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Amya Singhal
- Kevin McNee
- Tim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tim (Gerrit)
Tim voted and added 4 comments![]( "Open in Gerrit")
Votes added by Tim
| Code-Review | +1 |
4 comments
Thanks Amya, extensions LGTM with a few nits.
constexpr std::string_view kInvalidAudioLevelError = "Invalid audio level.";optional: Do you want to make this error message a little more useful and actionable by stating the valid range?
views::AsViewClass<DictationToastView>(GetContentsView())optional: this could return nullptr if `GetContentsView()` ever returned something that wasn't a `DictationToastView` and then we'd immediately dereference it. Probably not something we need to worry about but something to be aware of with how this is written.
// Updates the audio level of the stream. Valid values are between 0.0 and 1.0.nit: we try to wrap comments in the IDL schemas at 80 characters where appropriate.
Related details
- Amya Singhal
- Kevin McNee
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Amya Singhal (Gerrit)
Amya Singhal added 3 comments![]( "Open in Gerrit")
constexpr std::string_view kInvalidAudioLevelError = "Invalid audio level.";optional: Do you want to make this error message a little more useful and actionable by stating the valid range?
great suggestion, made the error more descriptive
optional: this could return nullptr if `GetContentsView()` ever returned something that wasn't a `DictationToastView` and then we'd immediately dereference it. Probably not something we need to worry about but something to be aware of with how this is written.
Agreed! It's safe in this specific case because the contents view type is invariant after construction, but definitely a pattern to be careful with.
// Updates the audio level of the stream. Valid values are between 0.0 and 1.0.nit: we try to wrap comments in the IDL schemas at 80 characters where appropriate.
- David Bokan
- Justin Lulejian
- Kevin McNee
- Tim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bokan (Gerrit)
David Bokan voted Code-Review+1![]( "Open in Gerrit")
- Amya Singhal
- Justin Lulejian
- Kevin McNee
- Tim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin McNee (Gerrit)
Kevin McNee voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Amya Singhal
- Justin Lulejian
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin McNee (Gerrit)
Kevin McNee voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
David Bokan (Gerrit)
David Bokan voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Amya Singhal
- Justin Lulejian
- Kevin McNee
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |