Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
551823005:
[Hotword] Adding audio parameters to handle audio coming from the DSP. (Closed)
|
Created:
6 years, 3 months ago by rpetterson Modified:
6 years ago Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[Hotword] Adding audio parameters to handle audio coming from the DSP.
BUG=403138
Committed: https://crrev.com/605d12b7a480a6f1b2fc479f7aa87a579bcbef42
Cr-Commit-Position: refs/heads/master@{#308487}
Patch Set 1 #Patch Set 2 : add cancelable closure #Patch Set 3 : fix up calling order, start unittest #Patch Set 4 : using new api #Patch Set 5 : a little cleanup #
Total comments: 18
Patch Set 6 : respond to comments #Patch Set 7 : change parameter name to hotword #Patch Set 8 : put call to add stream in else statement #
Total comments: 2
Patch Set 9 : move destroy #Patch Set 10 : disable callback on Stop() #Patch Set 11 : unify stream, add hotword flag #Patch Set 12 : remove unnecessary changes (easier to diff) #Patch Set 13 : remove extra line #Patch Set 14 : moving test to audio_input_unittest #Patch Set 15 : remove old test #Patch Set 16 : remove duplicated code #Patch Set 17 : remove test #Patch Set 18 : actually remove test #
Total comments: 4
Patch Set 19 : update stream type #
Total comments: 2
Messages
Total messages: 37 (4 generated)
rlp@chromium.org changed reviewers: + dalecurtis@chromium.org
dgreid@chromium.org changed reviewers: + dgreid@chromium.org
https://codereview.chromium.org/551823005/diff/80001/media/audio/audio_parame... File media/audio/audio_parameters.h (right): https://codereview.chromium.org/551823005/diff/80001/media/audio/audio_parame... media/audio/audio_parameters.h:54: HARDWARE_INPUT = 0x8, Can HARDWARE_INPUT have a different name? All the streams are hardware inputs except loopback currently. https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... File media/audio/cras/cras_input.cc (right): https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:198: This can go above the creation of stream_params, and either return early or otherwise avoid adding an input stream. https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:248: me->ReadAudio(frames, single_sample, sample_ts); this can be as much as two seconds of audio. Does it need to be broken in to smaller chunks to fit though the audio bus? https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... File media/audio/cras/cras_input.h (right): https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... media/audio/cras/cras_input.h:12: #include "base/cancelable_callback.h" Don't need this one. https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... media/audio/cras/cras_input.h:55: // Handles requests to get a sample from the provided buffer. This will be I'd remove the first sentence of this comment.
https://codereview.chromium.org/551823005/diff/80001/media/audio/audio_parame... File media/audio/audio_parameters.h (right): https://codereview.chromium.org/551823005/diff/80001/media/audio/audio_parame... media/audio/audio_parameters.h:54: HARDWARE_INPUT = 0x8, On 2014/09/17 21:16:22, dgreid wrote: > Can HARDWARE_INPUT have a different name? All the streams are hardware inputs > except loopback currently. I'll change it to HOTWORD to mimic the callbacks. https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... File media/audio/cras/cras_input.cc (right): https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:198: On 2014/09/17 21:16:23, dgreid wrote: > This can go above the creation of stream_params, and either return early or > otherwise avoid adding an input stream. I've moved it and wrapped it in an else sinc ei assume we still need to deal with adding the stream (or does this take care of that? should that be in the else as well?) and destroy the stream params when done. https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:248: me->ReadAudio(frames, single_sample, sample_ts); On 2014/09/17 21:16:23, dgreid wrote: > this can be as much as two seconds of audio. Does it need to be broken in to > smaller chunks to fit though the audio bus? Question for Dale. https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... File media/audio/cras/cras_input.h (right): https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... media/audio/cras/cras_input.h:12: #include "base/cancelable_callback.h" On 2014/09/17 21:16:23, dgreid wrote: > Don't need this one. Done. https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... media/audio/cras/cras_input.h:55: // Handles requests to get a sample from the provided buffer. This will be On 2014/09/17 21:16:23, dgreid wrote: > I'd remove the first sentence of this comment. Done.
https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... File media/audio/cras/cras_input.cc (right): https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:198: On 2014/09/17 21:32:00, rpetterson wrote: > On 2014/09/17 21:16:23, dgreid wrote: > > This can go above the creation of stream_params, and either return early or > > otherwise avoid adding an input stream. > > I've moved it and wrapped it in an else sinc ei assume we still need to deal > with adding the stream (or does this take care of that? should that be in the > else as well?) and destroy the stream params when done. Yes put that in the else as well, there is no need to add the stream. The call to add_hotword_callback replaces the call to add_stream.
https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... File media/audio/cras/cras_input.cc (right): https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:198: On 2014/09/17 21:36:33, dgreid wrote: > On 2014/09/17 21:32:00, rpetterson wrote: > > On 2014/09/17 21:16:23, dgreid wrote: > > > This can go above the creation of stream_params, and either return early or > > > otherwise avoid adding an input stream. > > > > I've moved it and wrapped it in an else sinc ei assume we still need to deal > > with adding the stream (or does this take care of that? should that be in the > > else as well?) and destroy the stream params when done. > > Yes put that in the else as well, there is no need to add the stream. The call > to add_hotword_callback replaces the call to add_stream. Done.
This looks like it can work, I'll try to get the CRAS side change it depends on merged tonight. https://codereview.chromium.org/551823005/diff/130001/media/audio/cras/cras_i... File media/audio/cras/cras_input.cc (right): https://codereview.chromium.org/551823005/diff/130001/media/audio/cras/cras_i... media/audio/cras/cras_input.cc:207: cras_client_stream_params_destroy(stream_params); move stream_params_destory into the else as well.
https://codereview.chromium.org/551823005/diff/130001/media/audio/cras/cras_i... File media/audio/cras/cras_input.cc (right): https://codereview.chromium.org/551823005/diff/130001/media/audio/cras/cras_i... media/audio/cras/cras_input.cc:207: cras_client_stream_params_destroy(stream_params); On 2014/09/17 22:03:00, dgreid wrote: > move stream_params_destory into the else as well. Done.
https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... File media/audio/cras/cras_input.cc (right): https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:248: me->ReadAudio(frames, single_sample, sample_ts); On 2014/09/17 21:32:00, rpetterson wrote: > On 2014/09/17 21:16:23, dgreid wrote: > > this can be as much as two seconds of audio. Does it need to be broken in to > > smaller chunks to fit though the audio bus? > > Question for Dale. Yes, this needs to be converted to match the parameters given during construction.
I've also added the line to cancel the callback in Stop(). https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... File media/audio/cras/cras_input.cc (right): https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:248: me->ReadAudio(frames, single_sample, sample_ts); On 2014/09/17 22:29:22, DaleCurtis wrote: > On 2014/09/17 21:32:00, rpetterson wrote: > > On 2014/09/17 21:16:23, dgreid wrote: > > > this can be as much as two seconds of audio. Does it need to be broken in > to > > > smaller chunks to fit though the audio bus? > > > > Question for Dale. > > Yes, this needs to be converted to match the parameters given during > construction. During construction of the stream? Which parameters? Is this something which needs to happen in ReadAudio then?
https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... File media/audio/cras/cras_input.cc (right): https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:248: me->ReadAudio(frames, single_sample, sample_ts); On 2014/09/22 22:24:40, rpetterson wrote: > On 2014/09/17 22:29:22, DaleCurtis wrote: > > On 2014/09/17 21:32:00, rpetterson wrote: > > > On 2014/09/17 21:16:23, dgreid wrote: > > > > this can be as much as two seconds of audio. Does it need to be broken in > > to > > > > smaller chunks to fit though the audio bus? > > > > > > Question for Dale. > > > > Yes, this needs to be converted to match the parameters given during > > construction. > > During construction of the stream? Which parameters? Is this something which > needs to happen in ReadAudio then? As mentioned, the parameters passed in during construction of CrasInputStream. Regarding ReadAudio, that's up to you and Dylan. If ReadAudio() returns the entire DSP buffer at parameters other than those mentioned, you'll need to instantiate an AudioConverter and deliver them in a delayed fashion as if it was real-time playout. See xians@ email about not overloading the shared memory. The API as written seems to indicate that you'll need to do the above.
I've removed the test since there's been a lot of debate about the best way to test this in an automated fashion but no consensus. It'd be great to figure this out, but at this point I think it best to put the test in a separate CL. The change itself seems innocuous until we actually start creating any hotword streams (which to my knowledge we are not and therefore behavior should remain the same). Dylan, is this the right way to pass the flags? Dale, happy for your comments and/or approval.
I defer to Dylan, there's not enough here for me to say anything about the correctness of this approach. That said, it'd be nice to not land this until the backend is ready for such calls; to avoid unnecessary churn.
I think this will work with the constant changed. I'm doing a build now to confirm. Do we have code to test with that uses the new flag? https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... File media/audio/cras/cras_input.cc (right): https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:248: me->ReadAudio(frames, single_sample, sample_ts); On 2014/09/22 22:24:40, rpetterson wrote: > On 2014/09/17 22:29:22, DaleCurtis wrote: > > On 2014/09/17 21:32:00, rpetterson wrote: > > > On 2014/09/17 21:16:23, dgreid wrote: > > > > this can be as much as two seconds of audio. Does it need to be broken in > > to > > > > smaller chunks to fit though the audio bus? > > > > > > Question for Dale. > > > > Yes, this needs to be converted to match the parameters given during > > construction. > > During construction of the stream? Which parameters? Is this something which > needs to happen in ReadAudio then? It might be better to do it here, call ReadAudio repeatedly with audio_bus_->frames() worth of samples until all the samples have been sent. This is awkward, but we are using a streaming interface to send a big chunk of data. I'm not sure we can avoid it. https://codereview.chromium.org/551823005/diff/330001/media/audio/cras/cras_i... File media/audio/cras/cras_input.cc (right): https://codereview.chromium.org/551823005/diff/330001/media/audio/cras/cras_i... media/audio/cras/cras_input.cc:18: static const uint32_t kHotwordParam = 1; This will be 0x03. or even better HOTWORD_STREAM from cras_types.h once this lands: https://chromium-review.googlesource.com/#/c/233104/2/cras/src/common/cras_ty...
On 2014/12/10 00:23:35, dgreid wrote: > I think this will work with the constant changed. I'm doing a build now to > confirm. Do we have code to test with that uses the new flag? > I don't think so. Since it's not in the codebase yet, nothing uses it. +amistry who can point us to where it should go in the code. https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... File media/audio/cras/cras_input.cc (right): https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:248: me->ReadAudio(frames, single_sample, sample_ts); On 2014/12/10 00:23:34, dgreid wrote: > On 2014/09/22 22:24:40, rpetterson wrote: > > On 2014/09/17 22:29:22, DaleCurtis wrote: > > > On 2014/09/17 21:32:00, rpetterson wrote: > > > > On 2014/09/17 21:16:23, dgreid wrote: > > > > > this can be as much as two seconds of audio. Does it need to be broken > in > > > to > > > > > smaller chunks to fit though the audio bus? > > > > > > > > Question for Dale. > > > > > > Yes, this needs to be converted to match the parameters given during > > > construction. > > > > During construction of the stream? Which parameters? Is this something which > > needs to happen in ReadAudio then? > It might be better to do it here, call ReadAudio repeatedly with > audio_bus_->frames() worth of samples until all the samples have been sent. > > This is awkward, but we are using a streaming interface to send a big chunk of > data. I'm not sure we can avoid it. > I'm unclear here. What do I need to change? https://codereview.chromium.org/551823005/diff/330001/media/audio/cras/cras_i... File media/audio/cras/cras_input.cc (right): https://codereview.chromium.org/551823005/diff/330001/media/audio/cras/cras_i... media/audio/cras/cras_input.cc:18: static const uint32_t kHotwordParam = 1; On 2014/12/10 00:23:35, dgreid wrote: > This will be 0x03. or even better HOTWORD_STREAM from cras_types.h once this > lands: > https://chromium-review.googlesource.com/#/c/233104/2/cras/src/common/cras_ty... Cool. When do you think that will land?
https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... File media/audio/cras/cras_input.cc (right): https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... media/audio/cras/cras_input.cc:248: me->ReadAudio(frames, single_sample, sample_ts); ignore that comment. it must have been a draft from an older patch set that got sent when I sent the other message. https://codereview.chromium.org/551823005/diff/330001/media/audio/cras/cras_i... File media/audio/cras/cras_input.cc (right): https://codereview.chromium.org/551823005/diff/330001/media/audio/cras/cras_i... media/audio/cras/cras_input.cc:18: static const uint32_t kHotwordParam = 1; On 2014/12/10 00:37:43, rpetterson wrote: > On 2014/12/10 00:23:35, dgreid wrote: > > This will be 0x03. or even better HOTWORD_STREAM from cras_types.h once this > > lands: > > > https://chromium-review.googlesource.com/#/c/233104/2/cras/src/common/cras_ty... > > Cool. When do you think that will land? hopefully soon. I need to get a build without it and dependencies to push to dev and make sure a crash is fixed, then I can land it.
On 2014/12/10 00:54:50, dgreid wrote: > https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... > File media/audio/cras/cras_input.cc (right): > > https://codereview.chromium.org/551823005/diff/80001/media/audio/cras/cras_in... > media/audio/cras/cras_input.cc:248: me->ReadAudio(frames, single_sample, > sample_ts); > > ignore that comment. it must have been a draft from an older patch set that got > sent when I sent the other message. > > https://codereview.chromium.org/551823005/diff/330001/media/audio/cras/cras_i... > File media/audio/cras/cras_input.cc (right): > > https://codereview.chromium.org/551823005/diff/330001/media/audio/cras/cras_i... > media/audio/cras/cras_input.cc:18: static const uint32_t kHotwordParam = 1; > On 2014/12/10 00:37:43, rpetterson wrote: > > On 2014/12/10 00:23:35, dgreid wrote: > > > This will be 0x03. or even better HOTWORD_STREAM from cras_types.h once this > > > lands: > > > > > > https://chromium-review.googlesource.com/#/c/233104/2/cras/src/common/cras_ty... > > > > Cool. When do you think that will land? > > hopefully soon. I need to get a build without it and dependencies to push to dev > and make sure a crash is fixed, then I can land it. lgtm, All the platform changes landed over night and are present in Today's chrome os canary: 6568.0.0
https://codereview.chromium.org/551823005/diff/330001/media/audio/cras/cras_i... File media/audio/cras/cras_input.cc (right): https://codereview.chromium.org/551823005/diff/330001/media/audio/cras/cras_i... media/audio/cras/cras_input.cc:18: static const uint32_t kHotwordParam = 1; On 2014/12/10 00:54:50, dgreid wrote: > On 2014/12/10 00:37:43, rpetterson wrote: > > On 2014/12/10 00:23:35, dgreid wrote: > > > This will be 0x03. or even better HOTWORD_STREAM from cras_types.h once this > > > lands: > > > > > > https://chromium-review.googlesource.com/#/c/233104/2/cras/src/common/cras_ty... > > > > Cool. When do you think that will land? > > hopefully soon. I need to get a build without it and dependencies to push to dev > and make sure a crash is fixed, then I can land it. I've updated the way I think you mean. Let me know if it looks okay.
On 2014/12/13 01:04:00, rpetterson wrote: > https://codereview.chromium.org/551823005/diff/330001/media/audio/cras/cras_i... > File media/audio/cras/cras_input.cc (right): > > https://codereview.chromium.org/551823005/diff/330001/media/audio/cras/cras_i... > media/audio/cras/cras_input.cc:18: static const uint32_t kHotwordParam = 1; > On 2014/12/10 00:54:50, dgreid wrote: > > On 2014/12/10 00:37:43, rpetterson wrote: > > > On 2014/12/10 00:23:35, dgreid wrote: > > > > This will be 0x03. or even better HOTWORD_STREAM from cras_types.h once > this > > > > lands: > > > > > > > > > > https://chromium-review.googlesource.com/#/c/233104/2/cras/src/common/cras_ty... > > > > > > Cool. When do you think that will land? > > > > hopefully soon. I need to get a build without it and dependencies to push to > dev > > and make sure a crash is fixed, then I can land it. > > I've updated the way I think you mean. Let me know if it looks okay. Also is there any reason for this issue to still be private?
that looks right. I'm not sure why this issue was private to begin with.
ping to dalecurtis
ping to dalecurtis
lgtm % some questions: - Is the DSP buffer now returned in the format requested by the AudioParameters given to CRAS? - Where's the other end of the code which is specifying this flag? I expected to see some PPAPI changes at least for this.
On 2014/12/15 18:12:02, DaleCurtis wrote: > lgtm % some questions: > > - Is the DSP buffer now returned in the format requested by the AudioParameters > given to CRAS? Yes, and the transer size of each callback can be set to whatever we need to make it work. CRAS will break up the buffer into digestible chunks if needed.
On 2014/12/15 18:12:02, DaleCurtis wrote: > - Where's the other end of the code which is specifying this flag? I expected to > see some PPAPI changes at least for this. Dylan, correct me if I'm wrong, but the other end of this code would be in the extension javascript code when the stream is created.
I don't believe there's any way for an extension (definitely not in JS, I don't think in PPAPI either) to specify effects flags currently. You need to plumb that.
On 2014/12/15 19:36:23, DaleCurtis wrote: > I don't believe there's any way for an extension (definitely not in JS, I don't > think in PPAPI either) to specify effects flags currently. You need to plumb > that. I'm not sure what you mean, but JS can pass constraints on the media stream to open. See https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... which translates into https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
I'm just saying there's no HOTWORD flag there, so it needs to be plumbed.
It sounds like the PPAPI stuff is in a different chunk of code, so I'll get this checked in and then either Anand or I will do that part in a different CL.
The CQ bit was checked by rlp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/551823005/350001
Message was sent while issue was closed.
Committed patchset #19 (id:350001)
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/605d12b7a480a6f1b2fc479f7aa87a579bcbef42 Cr-Commit-Position: refs/heads/master@{#308487}
Message was sent while issue was closed.
smckay@chromium.org changed reviewers: + smckay@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/551823005/diff/350001/media/audio/cras/cras_i... File media/audio/cras/cras_input.cc (right): https://codereview.chromium.org/551823005/diff/350001/media/audio/cras/cras_i... media/audio/cras/cras_input.cc:165: flags = HOTWORD_STREAM; This is still broken for me. Where is this value defined?
Message was sent while issue was closed.
https://codereview.chromium.org/551823005/diff/350001/media/audio/cras/cras_i... File media/audio/cras/cras_input.cc (right): https://codereview.chromium.org/551823005/diff/350001/media/audio/cras/cras_i... media/audio/cras/cras_input.cc:165: flags = HOTWORD_STREAM; On 2014/12/17 17:41:29, Steve McKay wrote: > This is still broken for me. Where is this value defined? in cras_types.h, you'll need an up to date libcras from a recent chromeos checkout.
Message was sent while issue was closed.
On 2014/12/17 17:44:20, dgreid wrote: > https://codereview.chromium.org/551823005/diff/350001/media/audio/cras/cras_i... > File media/audio/cras/cras_input.cc (right): > > https://codereview.chromium.org/551823005/diff/350001/media/audio/cras/cras_i... > media/audio/cras/cras_input.cc:165: flags = HOTWORD_STREAM; > On 2014/12/17 17:41:29, Steve McKay wrote: > > This is still broken for me. Where is this value defined? > > in cras_types.h, you'll need an up to date libcras from a recent chromeos > checkout. What about simple chrome (http://www.chromium.org/chromium-os/how-tos-and-troubleshooting/building-chro... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
