Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2306803003:
Hotword: Fix always-on case by removing MS_CONFIGURED timeout (Closed)
DescriptionHotword: Fix always-on case by removing MS_CONFIGURED timeout
The behavior of MediaStreamAudioTrack::Configure() has changed:
Configure() used to be able to complete before any audio sample
is received because the AudioParameter can be obtained from a
WebRtcLocalAudioTrack according to
https://codereview.chromium.org/857093002
After the media stream refactoring http://crbug.com/577874
Configure() remains pending until the first audio frame is
received. For always-on hotword detection with a DSP audio
source, no audio sample is sent until the DSP detects a
potential hotword trigger.
This patch removes MS_CONFIGURED timeout so that the plugin
can keep waiting for a hotword instead of reporting a timeout
error.
BUG=616203
TEST=Verified always-on hotwording on Samus and Chell
Signed-off-by: Ben Zhang <benzh@chromium.org>;
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/3d143aff4728e8bc2a06cd82a2f6d2444fc14726
Cr-Commit-Position: refs/heads/master@{#418997}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Hotword: Fix always-on case by removing MS_CONFIGURED timeout #Messages
Total messages: 19 (11 generated)
Description was changed from ========== Hotword: Fix always-on case by removing MS_CONFIGURED timeout BUG=616203 TEST=Verified always-on hotwording on Samus and Chell Signed-off-by: Ben Zhang <benzh@chromium.org> ========== to ========== Hotword: Fix always-on case by removing MS_CONFIGURED timeout BUG=616203 TEST=Verified always-on hotwording on Samus and Chell Signed-off-by: Ben Zhang <benzh@chromium.org> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Hotword: Fix always-on case by removing MS_CONFIGURED timeout BUG=616203 TEST=Verified always-on hotwording on Samus and Chell Signed-off-by: Ben Zhang <benzh@chromium.org> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Hotword: Fix always-on case by removing MS_CONFIGURED timeout BUG=616203 TEST=Verified always-on hotwording on Samus and Chell Signed-off-by: Ben Zhang <benzh@chromium.org> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
benzh@chromium.org changed reviewers: + mgiuca@chromium.org, penghuang@chromium.org
benzh@chromium.org changed reviewers: + dgreid@chromium.org
Hi Ben, sorry I missed this for days and days. I'm not able to comment on the technical side of things as I'm not familiar with the code. However, it seems reasonable. I have a comment about the comment. Otherwise, lgtm but please wait for one other person to look over it who is more knowledgeable about this code. https://codereview.chromium.org/2306803003/diff/1/chrome/browser/resources/ho... File chrome/browser/resources/hotword/nacl_manager.js (right): https://codereview.chromium.org/2306803003/diff/1/chrome/browser/resources/ho... chrome/browser/resources/hotword/nacl_manager.js:464: // it. After the recent MediaStreamAudioTrack refactoring, the behavior of The code should describe what it is *now*, not refer to what it was (which will become less and less relevant as time goes on). Especially avoid time-sensitive terms like "recent". It would be good to move the discussion of recent changes to the commit log rather than the code. For the code comments, how about: "The plugin will send a MS_CONFIGURED, but don't set a timeout waiting for it. MediaStreamAudioTrack::Configure() will remain pending until the first audio buffer is received. When the audio source is a DSP for always-on detection, no audio sample is sent until the DSP detects a potential hotword trigger. Thus, Configure would remain pending indefinitely if we were to wait here. See https://crbug.com/616203." (Note also: I added "https://" to the URL so it is clickable in some editors.)
Description was changed from ========== Hotword: Fix always-on case by removing MS_CONFIGURED timeout BUG=616203 TEST=Verified always-on hotwording on Samus and Chell Signed-off-by: Ben Zhang <benzh@chromium.org> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Hotword: Fix always-on case by removing MS_CONFIGURED timeout The behavior of MediaStreamAudioTrack::Configure() has changed: Configure() used to be able to complete before any audio sample is received because the AudioParameter can be obtained from a WebRtcLocalAudioTrack according to https://codereview.chromium.org/857093002 After the media stream refactoring http://crbug.com/577874 Configure() remains pending until the first audio frame is received. For always-on hotword detection with a DSP audio source, no audio sample is sent until the DSP detects a potential hotword trigger. This patch removes MS_CONFIGURED timeout so that the plugin can keep waiting for a hotword instead of reporting a timeout error. BUG=616203 TEST=Verified always-on hotwording on Samus and Chell Signed-off-by: Ben Zhang <benzh@chromium.org> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
benzh@chromium.org changed reviewers: + amistry@chromium.org
Thanks for the review Matt. I've updated the comment and commit message.
mgiuca@chromium.org changed reviewers: - amistry@chromium.org
slgtm Ping dgreid, penghuang for review.
On 2016/09/09 01:49:39, Matt Giuca wrote: > slgtm > > Ping dgreid, penghuang for review. Seems good to me. But you'll need owner approval still.
On 2016/09/09 02:10:25, dgreid wrote: > On 2016/09/09 01:49:39, Matt Giuca wrote: > > slgtm > > > > Ping dgreid, penghuang for review. > > Seems good to me. But you'll need owner approval still. I am an owner, I am just not actually familiar with the code so I wanted a better set of eyes. Seems like you've got approvals from both now.
The CQ bit was checked by benzh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2306803003/#ps20001 (title: "Hotword: Fix always-on case by removing MS_CONFIGURED timeout")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Hotword: Fix always-on case by removing MS_CONFIGURED timeout The behavior of MediaStreamAudioTrack::Configure() has changed: Configure() used to be able to complete before any audio sample is received because the AudioParameter can be obtained from a WebRtcLocalAudioTrack according to https://codereview.chromium.org/857093002 After the media stream refactoring http://crbug.com/577874 Configure() remains pending until the first audio frame is received. For always-on hotword detection with a DSP audio source, no audio sample is sent until the DSP detects a potential hotword trigger. This patch removes MS_CONFIGURED timeout so that the plugin can keep waiting for a hotword instead of reporting a timeout error. BUG=616203 TEST=Verified always-on hotwording on Samus and Chell Signed-off-by: Ben Zhang <benzh@chromium.org> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Hotword: Fix always-on case by removing MS_CONFIGURED timeout The behavior of MediaStreamAudioTrack::Configure() has changed: Configure() used to be able to complete before any audio sample is received because the AudioParameter can be obtained from a WebRtcLocalAudioTrack according to https://codereview.chromium.org/857093002 After the media stream refactoring http://crbug.com/577874 Configure() remains pending until the first audio frame is received. For always-on hotword detection with a DSP audio source, no audio sample is sent until the DSP detects a potential hotword trigger. This patch removes MS_CONFIGURED timeout so that the plugin can keep waiting for a hotword instead of reporting a timeout error. BUG=616203 TEST=Verified always-on hotwording on Samus and Chell Signed-off-by: Ben Zhang <benzh@chromium.org> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Hotword: Fix always-on case by removing MS_CONFIGURED timeout The behavior of MediaStreamAudioTrack::Configure() has changed: Configure() used to be able to complete before any audio sample is received because the AudioParameter can be obtained from a WebRtcLocalAudioTrack according to https://codereview.chromium.org/857093002 After the media stream refactoring http://crbug.com/577874 Configure() remains pending until the first audio frame is received. For always-on hotword detection with a DSP audio source, no audio sample is sent until the DSP detects a potential hotword trigger. This patch removes MS_CONFIGURED timeout so that the plugin can keep waiting for a hotword instead of reporting a timeout error. BUG=616203 TEST=Verified always-on hotwording on Samus and Chell Signed-off-by: Ben Zhang <benzh@chromium.org> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Hotword: Fix always-on case by removing MS_CONFIGURED timeout The behavior of MediaStreamAudioTrack::Configure() has changed: Configure() used to be able to complete before any audio sample is received because the AudioParameter can be obtained from a WebRtcLocalAudioTrack according to https://codereview.chromium.org/857093002 After the media stream refactoring http://crbug.com/577874 Configure() remains pending until the first audio frame is received. For always-on hotword detection with a DSP audio source, no audio sample is sent until the DSP detects a potential hotword trigger. This patch removes MS_CONFIGURED timeout so that the plugin can keep waiting for a hotword instead of reporting a timeout error. BUG=616203 TEST=Verified always-on hotwording on Samus and Chell Signed-off-by: Ben Zhang <benzh@chromium.org> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/3d143aff4728e8bc2a06cd82a2f6d2444fc14726 Cr-Commit-Position: refs/heads/master@{#418997} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/3d143aff4728e8bc2a06cd82a2f6d2444fc14726 Cr-Commit-Position: refs/heads/master@{#418997} |
