Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
499233003:
Binding media stream audio track to speech recognition [renderer] (Closed)
|
Created:
6 years, 4 months ago by burnik Modified:
6 years, 2 months ago Reviewers:
jamesr, tommi (sloooow) - chröme, kenrb, no longer working on chromium, henrika (OOO until Aug 14), jochen (gone - plz use gerrit) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionChrome support for binding media stream audio track to speech recognition - renderer implementation.
WebKit CL : http://crrev.com/448163003
[ Renderer CL ]
Browser CL : http://crrev.com/549373003
SpeechRecognitionAudioSink is an audio sink which delivers audio data
via SharedMemory and SyncSocket to the peer (speech recognizer impl on the browser).
Audio resampling is done here rather than on the browser process.
Synchronization is done by managing two buffer indices, one in SpeechRecognitionAudioSink
incremented locally and the other in shared memory incremented by the peer.
Both buffer indices must match before a new signal is issued to the peer via |SyncSocket::Send()|.
That 4B signal is complemented by a |SyncSocket::Receive()| and used to trigger consumption on the peer.
WebMediaStreamTrack object is checked to be sourced from an audio device, avoiding possible WebAudio injection.
Unsupported tracks get notified to WebKit and session fails to start.
Issues with signaling over the SyncSocket are logged.
Design doc: http://goo.gl/9Ot3PC
BUG=408940
Committed: https://crrev.com/2eeb46675144086a100ce6497f4b8ac5e56fcdb1
Cr-Commit-Position: refs/heads/master@{#298981}
Patch Set 1 #Patch Set 2 : style fix #
Total comments: 46
Patch Set 3 : SyncSocket implementation + refactoring #Patch Set 4 : Platform checks removed from dispatcher #
Total comments: 78
Patch Set 5 : Add unit test and refactor #
Total comments: 85
Patch Set 6 : Refactoring on callbacks and error states #
Total comments: 72
Patch Set 7 : Refactoring unit test and source provider, moved to media #
Total comments: 8
Patch Set 8 : SyncSocket leak and FIFO fixes. Test 8-192KHz for input. #
Total comments: 54
Patch Set 9 : Refactoring, error states, more comments. #
Total comments: 45
Patch Set 10 : s/SpeechRecognitionAudioSourceProvider/SpeechRecognitionAudioSink/ #
Total comments: 1
Patch Set 11 : Change error type for unsupported tracks #
Total comments: 16
Patch Set 12 : Nits, comments, refactoring, rebasing. #
Total comments: 13
Patch Set 13 : Comments + bugfix #
Total comments: 15
Patch Set 14 : Unit test nits + ctor comments #
Total comments: 4
Patch Set 15 : Remove peer_buffer_index_ #
Total comments: 2
Patch Set 16 : s/audio_input_buffer/GetAudioInputBuffer/ #Patch Set 17 : Add ENABLE_WEBRTC flag checks #
Total comments: 2
Patch Set 18 : Remove override for blink impl #Patch Set 19 : Rebase on master - merge fix #
Total comments: 8
Patch Set 20 : s/OnSharedAudioBusReady/OnAudioReceiverReady/ + nits #
Total comments: 4
Patch Set 21 : Nits done. Preland checks. #Messages
Total messages: 91 (11 generated)
Please review my CL. :-)
This is a very large CL adding about 400 lines of code. Any design doc showing the principles of the data flow? Shijing, I assume you will go first given that you know the background. No unit test or other test which can be used to drive the code?
I would like to see an updated version of CL which uses sync socket + shared memory as a way to send the audio data to the browser. Also, an unittest, some clean-up on comments and code are required.
First review round. Neatly written code. https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.cc (right): https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:22: ) put on previous line? https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:25: output_params_(params), should output_params_ be const? https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:31: DCHECK(shared_memory_.Map(memory_length)); I think this is a bug... DCHECK()ed code isn't included in release builds, so Map() will never run. https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:42: SpeechRecognitionAudioSourceProvider::~SpeechRecognitionAudioSourceProvider() { missing thread check for dtor https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:56: DCHECK(capture_thread_checker_.CalledOnValidThread()); this dcheck will never hit because of the Detach on the previous line https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:57: DCHECK(input_params.IsValid()); would it make sense to assert that input_params_ (member variable) has not been set when we get here? https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:68: DCHECK_EQ(0,output_params_.frames_per_buffer() * input_params_.sample_rate() % space after , https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:69: output_params_.sample_rate()); 4 space indent https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:71: input_params_.sample_rate() / output_params_.sample_rate(); 4 spaces https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:80: void SpeechRecognitionAudioSourceProvider::OnReadyStateChanged( thread check here? https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:83: track_stopped_ = true; would it make sense to add else DCHECK(!track_stopped_); ? https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:86: void SpeechRecognitionAudioSourceProvider::OnData( on which thread does this function run? https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:91: remove empty line https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:94: if (fifo_->frames() + number_of_frames > fifo_->max_frames()) { it's not clear to me if fifo_ needs protection etc. I think it would be good if every method would have a thread check so that the threading model can be easily understood and verified. https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:103: DCHECK(capture_thread_checker_.CalledOnValidThread()); ah... this should be at the top of the function. As is, you don't run this check if you get into the if() statement above. https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:126: // make sure the previous output buffer was consumed by the client why is it safe to touch the above member variables without the lock but not the ones below? (some documentation would be good to have to explain - I may have missed it too, so just point me to it if that's the case) https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:141: on_data_callback_.Run(); if there's a way to avoid holding the lock when firing this callback, then that would be good. I think that on_data_callback_ is const and won't ever change throughout the lifetime of |this|, so we likely do not need to hold the lock here. https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.h (right): https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:51: // MediaStreamAudioSink implementation. Do these implementations need to be a part of the public interface of SpeechRecognitionAudioSourceProvider? If not (i.e. if they are an implementation detail of SpeechRecognitionAudioSourceProvider), then I'd like to make these protected or private. https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:68: virtual void NotifyAudioBusConsumed(); OVERRIDE? https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:71: const int kNumberOfBuffersInFifo = 2; static? https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:72: int fifo_buffer_size_; can you add documentation about what this represents and how big the buffer can expected to be?
Agree, looks really good! Just some nits for now; see that Tommi added some comments which should get you started. https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.h (right): https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:80: base::SharedMemory shared_memory_; Could you add more information about what these members do? https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... File content/renderer/speech_recognition_dispatcher.h (right): https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.h:73: // for receiving audio data via audio track nit, capital 'F' end with period '.' as all other comments. https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.h:89: bool audio_track_set_; Comment. ...and, capitals for all comments below ;-) https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.h:91: // Used by policy on which audio sources are allowed Rather unclear comment to me. What is meant by "used by policy"?
SyncSocket implementation. Updating design doc & working on unit test. https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.cc (right): https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:22: ) On 2014/08/25 14:38:40, tommi wrote: > put on previous line? Done. https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:25: output_params_(params), I think so. AudioConverter marks them const. I'm only reading them here. On 2014/08/25 14:38:45, tommi wrote: > should output_params_ be const? https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:31: DCHECK(shared_memory_.Map(memory_length)); On 2014/08/25 14:38:45, tommi wrote: > I think this is a bug... DCHECK()ed code isn't included in release builds, so > Map() will never run. Done. https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:42: SpeechRecognitionAudioSourceProvider::~SpeechRecognitionAudioSourceProvider() { On 2014/08/25 14:38:45, tommi wrote: > missing thread check for dtor Done. https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:57: DCHECK(input_params.IsValid()); I believe previous state of input_params_ does not matter. We overwrite it (or write it for the first time) when this event occurs. On 2014/08/25 14:38:45, tommi wrote: > would it make sense to assert that input_params_ (member variable) has not been > set when we get here? https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:68: DCHECK_EQ(0,output_params_.frames_per_buffer() * input_params_.sample_rate() % On 2014/08/25 14:38:45, tommi wrote: > space after , Done. https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:71: input_params_.sample_rate() / output_params_.sample_rate(); On 2014/08/25 14:38:44, tommi wrote: > 4 spaces Done. https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:80: void SpeechRecognitionAudioSourceProvider::OnReadyStateChanged( On 2014/08/25 14:38:45, tommi wrote: > thread check here? Done. https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:83: track_stopped_ = true; On 2014/08/25 14:38:40, tommi wrote: > would it make sense to add > > else > DCHECK(!track_stopped_); > > ? Done. https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:86: void SpeechRecognitionAudioSourceProvider::OnData( capturer thread. Done. On 2014/08/25 14:38:45, tommi wrote: > on which thread does this function run? https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:91: On 2014/08/25 14:38:45, tommi wrote: > remove empty line Done. https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:94: if (fifo_->frames() + number_of_frames > fifo_->max_frames()) { On 2014/08/25 14:38:45, tommi wrote: > it's not clear to me if fifo_ needs protection etc. I think it would be good if > every method would have a thread check so that the threading model can be easily > understood and verified. Done. https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:103: DCHECK(capture_thread_checker_.CalledOnValidThread()); On 2014/08/25 14:38:40, tommi wrote: > ah... this should be at the top of the function. As is, you don't run this > check if you get into the if() statement above. Done. https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:126: // make sure the previous output buffer was consumed by the client This was only intended for protecting unconsumed_audio_buffers_. That's because the NotifyAudioBusConsumed() was called on the main render thread, and OnData on the capture thread. This event model has been removed in the next patchset. On 2014/08/25 14:38:45, tommi wrote: > why is it safe to touch the above member variables without the lock but not the > ones below? (some documentation would be good to have to explain - I may have > missed it too, so just point me to it if that's the case) https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:141: on_data_callback_.Run(); Locks removed from the design. On 2014/08/25 14:38:45, tommi wrote: > if there's a way to avoid holding the lock when firing this callback, then that > would be good. I think that on_data_callback_ is const and won't ever change > throughout the lifetime of |this|, so we likely do not need to hold the lock > here. https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.h (right): https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:51: // MediaStreamAudioSink implementation. On 2014/08/25 14:38:46, tommi wrote: > Do these implementations need to be a part of the public interface of > SpeechRecognitionAudioSourceProvider? > If not (i.e. if they are an implementation detail of > SpeechRecognitionAudioSourceProvider), then I'd like to make these protected or > private. Done. https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:68: virtual void NotifyAudioBusConsumed(); Removed due to sync_socket. On 2014/08/25 14:38:46, tommi wrote: > OVERRIDE? https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:71: const int kNumberOfBuffersInFifo = 2; On 2014/08/25 14:38:45, tommi wrote: > static? Done. https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:80: base::SharedMemory shared_memory_; On 2014/08/25 14:46:00, henrika wrote: > Could you add more information about what these members do? Done.
https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.cc (right): https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:25: output_params_(params), On 2014/08/29 09:18:16, burnik wrote: > I think so. AudioConverter marks them const. I'm only reading them here. > On 2014/08/25 14:38:45, tommi wrote: > > should output_params_ be const? > OK, but I'm not seeing that it has been changed to const. https://codereview.chromium.org/499233003/diff/20001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:56: DCHECK(capture_thread_checker_.CalledOnValidThread()); On 2014/08/25 14:38:45, tommi wrote: > this dcheck will never hit because of the Detach on the previous line Anything needed to be done here? https://codereview.chromium.org/499233003/diff/60001/base/native_sync_socket.h File base/native_sync_socket.h (right): https://codereview.chromium.org/499233003/diff/60001/base/native_sync_socket.... base/native_sync_socket.h:18: class NativeSyncSocket { This class seems to rely on SyncSocket but doesn't provide any non-static functionality (i.e. it appears to be more of a namespace). Why wouldn't we simply add these methods to SyncSocket instead of introducing new files for these very basic helpers? https://codereview.chromium.org/499233003/diff/60001/base/native_sync_socket.... base/native_sync_socket.h:23: static bool PrepareForeignSocketDescriptor( implementation should be in the cc file https://codereview.chromium.org/499233003/diff/60001/base/native_sync_socket.... base/native_sync_socket.h:23: static bool PrepareForeignSocketDescriptor( I know that this method name comes from elsewhere, but if we're moving it into base/ I think we should come up with a more descriptive name that makes it clear that handover from one process to another is taking place. https://codereview.chromium.org/499233003/diff/60001/base/native_sync_socket.... base/native_sync_socket.h:49: static int Unwrap(const Descriptor& descriptor) { use base::SyncSocket::Handle here as well as the return value. The implementation should be in the .cc file and once you've fixed the return type, the #if defined() check can be inside the function and we can avoid having multiple declarations of the function. I also think that this could cause compiler warnings (which could break the build) at some higher levels since although Handle might be typedefed as an int, it's still a specific type. Calling code should ideally not have to do the #if checks. https://codereview.chromium.org/499233003/diff/60001/content/common/speech_re... File content/common/speech_recognition_messages.h (right): https://codereview.chromium.org/499233003/diff/60001/content/common/speech_re... content/common/speech_recognition_messages.h:130: IPC_MESSAGE_ROUTED5(SpeechRecognitionMsg_AudioTrackReady, is this approach the same as what we do elsewhere where we use SyncSocket? (I ask since NativeSyncSocket is a new class but we've been using SyncSocket for a while) https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.cc (right): https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:32: DLOG(ERROR) << "Could not map the shared memory"; Would this be a serious enough of an error to just do this? CHECK(shared_memory_.Map(memory_length)); (maybe you could check other places where we call SharedMemory::Map in the renderer process) https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:50: bool SpeechRecognitionAudioSourceProvider::IsAllowedAudioTrack( this method is static, right? If so, there should be a comment above indicating that: // static bool SpeechRecognitionAudioSourceProvider::IsAllowedAudioTrack( If it is not static, then it's missing a thread check. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:54: static_cast <MediaStreamAudioSource*>(track.source().extraData()); no space after static_cast https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:55: StreamDeviceInfo device_info = native_source->device_info(); no need to create a new StreamDeviceInfo instance. if you need a variable, just use a const reference: const StreamDeviceInfo& device_info = native_source->device_info(); or, just call device_info() inline in expression you return: return native_source->device_info().device.type == content::MEDIA_DEVICE_AUDIO_CAPTURE; https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:56: return (device_info.device.type == content::MEDIA_DEVICE_AUDIO_CAPTURE); this seems to me to be a 'supported' check rather than an 'allowed' check. "Allowed" usually refers to policy or permission checks. If this function is simply supposed to answer the question whether the implementation supports this type of track, I'd prefer to name it IsAudioTrackSupported() or possibly IsTrackTypeSupported(). https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:136: if (bytes_received == 0) should there be a NOTREACHED() in the body of this if statement? https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.h (right): https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:46: // determines the policy on what types of tracks are allowed nit: empty line above this comment for readability. ultra nit: Comments start with a capital letter and end with a period. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... File content/renderer/speech_recognition_dispatcher.cc (right): https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:72: remove this empty line https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:75: SpeechRecognitionAudioSourceProvider::IsAllowedAudioTrack(audio_track); would it make sense to have the IsAllowedAudioTrack() (or whatever it might get renamed to) implemented in this file? (in an anonymous namespace at the top of this file) This seems to be the only code that needs that functionality. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:76: audio_track_ = audio_track; first DCHECK that audio_track_ isn't valid? https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:77: audio_track_set_ = true; Should we DCHECK(!audio_track_set_); at the top of this function? https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:83: audio_track_set_ = false; reset/clear audio_track_ as well and set is_allowed_audio_track_ to false? https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:96: if (audio_track_set_ && !is_allowed_audio_track_) { do you really need is_allowed_audio_track_? If the track isn't allowed/supported, then shouldn't we simply not set audio_track_set_ to true? https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:118: msg_params.using_audio_track = (audio_track_set_ && is_allowed_audio_track_); looks like you always test these variables together.. actually, I'm wondering if you need either of them. What if you change attach() so that it does not assign to audio_track_ if the track isn't allowed/supported and then simply check audio_track_.isNull() here (and elsewhere where you currently check these two variables)? detach() would of course have to call audio_track_.reset() then (which I think it should anyway). https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:198: audio_source_provider_.reset(); I'm assuming that the browser side will be aware of the now 'stopped or aborted' state. Is that correct? https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:219: audio_source_provider_.reset(); same question here since this feels functionally comparable to stop() or abort() minus the message sent to the browser. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:263: // TODO(burnik): Log and DCHECK(!audio_source_provider_). can you do this now? https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:265: audio_source_provider_.reset(); should this be done in detach() also? https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:269: audio_track_, params, memory, socket, length)); indent https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... File content/renderer/speech_recognition_dispatcher.h (right): https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.h:49: const blink::WebMediaStreamTrack&, fix indent here and below
https://codereview.chromium.org/499233003/diff/60001/base/native_sync_socket.h File base/native_sync_socket.h (right): https://codereview.chromium.org/499233003/diff/60001/base/native_sync_socket.... base/native_sync_socket.h:18: class NativeSyncSocket { You and I should have a discussion with Tommi on if we should have this new class in base/ or not. https://codereview.chromium.org/499233003/diff/60001/content/common/speech_re... File content/common/speech_recognition_messages.h (right): https://codereview.chromium.org/499233003/diff/60001/content/common/speech_re... content/common/speech_recognition_messages.h:130: IPC_MESSAGE_ROUTED5(SpeechRecognitionMsg_AudioTrackReady, On 2014/08/29 11:25:30, tommi wrote: > is this approach the same as what we do elsewhere where we use SyncSocket? (I > ask since NativeSyncSocket is a new class but we've been using SyncSocket for a > while) I have the same concern here, Tommi, FYI, our existing code is using #if to handle the case: #if defined(OS_WIN) IPC_MESSAGE_CONTROL4(AudioMsg_NotifyStreamCreated, int /* stream id */, base::SharedMemoryHandle /* handle */, base::SyncSocket::Handle /* socket handle */, uint32 /* length */) #else IPC_MESSAGE_CONTROL4(AudioMsg_NotifyStreamCreated, int /* stream id */, base::SharedMemoryHandle /* handle */, base::FileDescriptor /* socket handle uint32 /* length */) #endif Though, I personally don't like having a NativeSyncSocket in base. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.cc (right): https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:42: if (audio_converter_.get() && attached_converter_) you are complicating things, you should just add the input to the converter when you construct the converter and you probably don't even need to call RemoveInput() in the destructor since the object is going away. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:53: MediaStreamAudioSource* native_source = you need to check if the track is local or not, if it is a remote track, it might not have MediaStreamAudioSource https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:74: new media::AudioConverter(input_params, output_params_, false)); Call AddInput here. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:123: audio_converter_->AddInput(this); I guess these code is workaround to fix AudioConverter calling ProvideInput twice at the beginning, I don't really think it is a good idea, instead, trying changing the code a few lines above to: if (fifo_->frames() <= fifo_buffer_size_) return; And add a comment to explain why you do this. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:133: size_t bytes_received = socket_.ReceiveWithTimeout(&peer_buffer_index, you have remove this ReceiveWithTimeout call, this OnData() is called on the real time audio thread, you can't block it. we need to figure out some other way to do the synchronization. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:145: // Notify client to consume buffer |buffer_index_| on |output_bus_|. empty line. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:147: // The send can fail if the user changes his input audio device is it true? https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:159: // Consume queued input frames by passing them to |audio_converter_| nit, empty line. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.h (right): https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:67: static const int kNumberOfBuffersInFifo = 2; move it to the implementation. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:111: // We attach the resampler once we have enough data in FIFO and not before. confusing comment, can it make it more clear on what this flag is used for? https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... File content/renderer/speech_recognition_dispatcher.cc (right): https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:93: // destroy any previous instance not to starve it waiting on chunk ACKs Destroy https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:101: WebSpeechRecognizerClient::NotAllowedError); probably you should fail the start call in such case. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... File content/renderer/speech_recognition_dispatcher.h (right): https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.h:96: scoped_refptr<base::MessageLoopProxy> render_loop_; why do you need this?
Seems like you have a large set of comments and I don't want to add even more at this stage. However, I do agree that you should use copy-paste of existing SyncSocket usage instead of making up a new scheme/style.
All good comments. Some discusssion is required to resolve a few questions. https://codereview.chromium.org/499233003/diff/60001/base/native_sync_socket.h File base/native_sync_socket.h (right): https://codereview.chromium.org/499233003/diff/60001/base/native_sync_socket.... base/native_sync_socket.h:18: class NativeSyncSocket { I agree. SyncSocket should have a descriptor which is cross-platform to avoid these checks (as it's done in multiple places where code relies on using the |SyncSocket|). Also should provide a method to prepare it for transit. On 2014/08/29 11:25:30, tommi wrote: > This class seems to rely on SyncSocket but doesn't provide any non-static > functionality (i.e. it appears to be more of a namespace). Why wouldn't we > simply add these methods to SyncSocket instead of introducing new files for > these very basic helpers? https://codereview.chromium.org/499233003/diff/60001/base/native_sync_socket.... base/native_sync_socket.h:18: class NativeSyncSocket { Agreed. Socket should be more cross-platform friendly. On 2014/08/29 12:23:06, xians1 wrote: > You and I should have a discussion with Tommi on if we should have this new > class in base/ or not. https://codereview.chromium.org/499233003/diff/60001/base/native_sync_socket.... base/native_sync_socket.h:23: static bool PrepareForeignSocketDescriptor( Agreed. I think name should be something like |PreparePeerSocketDescriptor|. On 2014/08/29 11:25:30, tommi wrote: > I know that this method name comes from elsewhere, but if we're moving it into > base/ I think we should come up with a more descriptive name that makes it clear > that handover from one process to another is taking place. https://codereview.chromium.org/499233003/diff/60001/base/native_sync_socket.... base/native_sync_socket.h:23: static bool PrepareForeignSocketDescriptor( On 2014/08/29 11:25:30, tommi wrote: > implementation should be in the cc file Acknowledged. https://codereview.chromium.org/499233003/diff/60001/base/native_sync_socket.... base/native_sync_socket.h:49: static int Unwrap(const Descriptor& descriptor) { Good point. When impl moves to .cc it will make more sense. On 2014/08/29 11:25:30, tommi wrote: > use base::SyncSocket::Handle here as well as the return value. > > The implementation should be in the .cc file and once you've fixed the return > type, the #if defined() check can be inside the function and we can avoid having > multiple declarations of the function. > I also think that this could cause compiler warnings (which could break the > build) at some higher levels since although Handle might be typedefed as an int, > it's still a specific type. Calling code should ideally not have to do the #if > checks. https://codereview.chromium.org/499233003/diff/60001/content/common/speech_re... File content/common/speech_recognition_messages.h (right): https://codereview.chromium.org/499233003/diff/60001/content/common/speech_re... content/common/speech_recognition_messages.h:130: IPC_MESSAGE_ROUTED5(SpeechRecognitionMsg_AudioTrackReady, I think it would be ok if we had |base::SyncSocket::SharedDescriptor|. It would make more sense and indicate that this descriptor is to be used by the peer process. On 2014/08/29 11:25:30, tommi wrote: > is this approach the same as what we do elsewhere where we use SyncSocket? (I > ask since NativeSyncSocket is a new class but we've been using SyncSocket for a > while) https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.cc (right): https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:32: DLOG(ERROR) << "Could not map the shared memory"; Done. Although I will revisit this when I do more work on errors. On 2014/08/29 11:25:31, tommi wrote: > Would this be a serious enough of an error to just do this? > > CHECK(shared_memory_.Map(memory_length)); > > (maybe you could check other places where we call SharedMemory::Map in the > renderer process) https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:42: if (audio_converter_.get() && attached_converter_) Adding the converter on construction will trigger problems with |ProvideInput()| since we won't have enough data on the FIFO. Therefore I would have to Zero() out the bus delivering empty data to the browser. I will check if RemoveInput is necessary or not. On 2014/08/29 12:23:06, xians1 wrote: > you are complicating things, you should just add the input to the converter when > you construct the converter and you probably don't even need to call > RemoveInput() in the destructor since the object is going away. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:50: bool SpeechRecognitionAudioSourceProvider::IsAllowedAudioTrack( It is static. Done. On 2014/08/29 11:25:31, tommi wrote: > this method is static, right? If so, there should be a comment above indicating > that: > > // static > bool SpeechRecognitionAudioSourceProvider::IsAllowedAudioTrack( > > If it is not static, then it's missing a thread check. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:53: MediaStreamAudioSource* native_source = Here I check if it's WebAudio. On 2014/08/29 12:23:06, xians1 wrote: > you need to check if the track is local or not, if it is a remote track, it > might not have MediaStreamAudioSource https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:54: static_cast <MediaStreamAudioSource*>(track.source().extraData()); On 2014/08/29 11:25:30, tommi wrote: > no space after static_cast Done. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:55: StreamDeviceInfo device_info = native_source->device_info(); On 2014/08/29 11:25:30, tommi wrote: > no need to create a new StreamDeviceInfo instance. if you need a variable, just > use a const reference: > const StreamDeviceInfo& device_info = native_source->device_info(); > > or, just call device_info() inline in expression you return: > > return native_source->device_info().device.type == > content::MEDIA_DEVICE_AUDIO_CAPTURE; Done. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:56: return (device_info.device.type == content::MEDIA_DEVICE_AUDIO_CAPTURE); This is implemented as a response to a suggestion from an e-mail thread saying WebAudio should not be allowed yet, although it is supported (e.g. if we return true here, we can feed a WebAudio track). Policy might change or even be moved from here. On 2014/08/29 11:25:31, tommi wrote: > this seems to me to be a 'supported' check rather than an 'allowed' check. > "Allowed" usually refers to policy or permission checks. If this function is > simply supposed to answer the question whether the implementation supports this > type of track, I'd prefer to name it IsAudioTrackSupported() or possibly > IsTrackTypeSupported(). https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:74: new media::AudioConverter(input_params, output_params_, false)); Same comment regarding the way |ProvideInput()| is called. On 2014/08/29 12:23:06, xians1 wrote: > Call AddInput here. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:123: audio_converter_->AddInput(this); I might try this out and see what happens. On 2014/08/29 12:23:06, xians1 wrote: > I guess these code is workaround to fix AudioConverter calling ProvideInput > twice at the beginning, I don't really think it is a good idea, instead, trying > changing the code a few lines above to: > if (fifo_->frames() <= fifo_buffer_size_) > return; > > And add a comment to explain why you do this. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:133: size_t bytes_received = socket_.ReceiveWithTimeout(&peer_buffer_index, On 2014/08/29 12:23:06, xians1 wrote: > you have remove this ReceiveWithTimeout call, this OnData() is called on the > real time audio thread, you can't block it. we need to figure out some other way > to do the synchronization. Acknowledged. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:136: if (bytes_received == 0) Good point. Done. And I'm still considering ways to handle these dangerous situations which could potentially fill up the FIFO. On 2014/08/29 11:25:30, tommi wrote: > should there be a NOTREACHED() in the body of this if statement? https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:147: // The send can fail if the user changes his input audio device As far as I've tested. On 2014/08/29 12:23:06, xians1 wrote: > is it true? https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:159: // Consume queued input frames by passing them to |audio_converter_| On 2014/08/29 12:23:06, xians1 wrote: > nit, empty line. Done. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.h (right): https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:46: // determines the policy on what types of tracks are allowed On 2014/08/29 11:25:31, tommi wrote: > nit: empty line above this comment for readability. ultra nit: Comments start > with a capital letter and end with a period. Done. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:67: static const int kNumberOfBuffersInFifo = 2; On 2014/08/29 12:23:07, xians1 wrote: > move it to the implementation. Done. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:111: // We attach the resampler once we have enough data in FIFO and not before. Done. On 2014/08/29 12:23:07, xians1 wrote: > confusing comment, can it make it more clear on what this flag is used for? https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... File content/renderer/speech_recognition_dispatcher.cc (right): https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:72: On 2014/08/29 11:25:31, tommi wrote: > remove this empty line Done. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:75: SpeechRecognitionAudioSourceProvider::IsAllowedAudioTrack(audio_track); My intention is to move away as much logic as possible from the dispatcher. Makes it much more easier to test. I agree that this static method should belong perhaps to another class - such as |SpeechRecognitionAudioTrackPolicy| so I can inject the policy rather than having it hardcoded. On 2014/08/29 11:25:31, tommi wrote: > would it make sense to have the IsAllowedAudioTrack() (or whatever it might get > renamed to) implemented in this file? (in an anonymous namespace at the top of > this file) > > This seems to be the only code that needs that functionality. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:76: audio_track_ = audio_track; Method needs refactoring. On 2014/08/29 11:25:31, tommi wrote: > first DCHECK that audio_track_ isn't valid? https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:83: audio_track_set_ = false; Method needs refactoring. On 2014/08/29 11:25:31, tommi wrote: > reset/clear audio_track_ as well and set is_allowed_audio_track_ to false? https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:93: // destroy any previous instance not to starve it waiting on chunk ACKs On 2014/08/29 12:23:07, xians1 wrote: > Destroy Done. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:96: if (audio_track_set_ && !is_allowed_audio_track_) { The JS |webkitSpeechRecognition.audioTrack| is not a mandatory field and can therefore be null. That indicates audio_track_set_ == false and falls back to default implementation on the browser (using AudioInputController). However if we set |webkitSpeechRecognition.audioTrack| to a WebAudio MediaStreamTrack, then it is set (the reference holds in JS). We should then tell the JS dev that this is not allowed but still fall back so SR session starts. That's why I need two members. However - I think I can get rid of audio_track_set_ and use a pointer for audio_track_ instead. BTW - This is a good topic of discussion regarding how this should behave towards the user in these corner cases. On 2014/08/29 11:25:31, tommi wrote: > do you really need is_allowed_audio_track_? If the track isn't > allowed/supported, then shouldn't we simply not set audio_track_set_ to true? https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:101: WebSpeechRecognizerClient::NotAllowedError); Good for discussion. On 2014/08/29 12:23:07, xians1 wrote: > probably you should fail the start call in such case. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:118: msg_params.using_audio_track = (audio_track_set_ && is_allowed_audio_track_); Same as comment above. The JS API behaviour is not set in stone. More discussion should resolve this as well. On 2014/08/29 11:25:31, tommi wrote: > looks like you always test these variables together.. > > actually, I'm wondering if you need either of them. What if you change attach() > so that it does not assign to audio_track_ if the track isn't allowed/supported > and then simply check audio_track_.isNull() here (and elsewhere where you > currently check these two variables)? > > detach() would of course have to call audio_track_.reset() then (which I think > it should anyway). https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:198: audio_source_provider_.reset(); This message is received from the browser. Sockets should resolve the rest. Although, I should inspect this event further. On 2014/08/29 11:25:31, tommi wrote: > I'm assuming that the browser side will be aware of the now 'stopped or aborted' > state. Is that correct? https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:219: audio_source_provider_.reset(); This effectively kills of the sockets so the audio thread on the browser stops. On 2014/08/29 11:25:31, tommi wrote: > same question here since this feels functionally comparable to stop() or abort() > minus the message sent to the browser. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:263: // TODO(burnik): Log and DCHECK(!audio_source_provider_). It's still a bit fuzzy because we can only have one active session and I'm not sure what multiple sessions would do. Tests will show. On 2014/08/29 11:25:31, tommi wrote: > can you do this now? https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:265: audio_source_provider_.reset(); I would have to dig in deeper to check if this would be needed. I will probably make these methods more robust. Tests will show. On 2014/08/29 11:25:31, tommi wrote: > should this be done in detach() also? https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:269: audio_track_, params, memory, socket, length)); On 2014/08/29 11:25:31, tommi wrote: > indent Done. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... File content/renderer/speech_recognition_dispatcher.h (right): https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.h:49: const blink::WebMediaStreamTrack&, On 2014/08/29 11:25:31, tommi wrote: > fix indent here and below Done. https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.h:96: scoped_refptr<base::MessageLoopProxy> render_loop_; Don't need it anymore. On 2014/08/29 12:23:07, xians1 wrote: > why do you need this?
burnik@chromium.org changed reviewers: + phoglund@chromium.org
Took some time, but finally - unit test included. Also updated design doc. Hopefully it's a bit more clear with those two now in place. This is a pretty large iteration, I know... However, this CL is (should be) atomic from a build perspective. phoglund@: Any tips from the test master are very welcome! :-) https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.cc (right): https://codereview.chromium.org/499233003/diff/60001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:42: if (audio_converter_.get() && attached_converter_) On 2014/08/29 13:26:16, burnik wrote: > Adding the converter on construction will trigger problems with |ProvideInput()| > since we won't have enough data on the FIFO. Therefore I would have to Zero() > out the bus delivering empty data to the browser. I will check if RemoveInput is > necessary or not. > > On 2014/08/29 12:23:06, xians1 wrote: > > you are complicating things, you should just add the input to the converter > when > > you construct the converter and you probably don't even need to call > > RemoveInput() in the destructor since the object is going away. > Done. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider_unittest.cc (right): https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider_unittest.cc:29: // Buffer to be shared between two fake sockets. 'fake' is interchangeable with 'mock' regarding sockets. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider_unittest.cc:442: // (3) Miraculasly recovered from the socket failure. * Miraculously https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider_unittest.cc:453: // First round of input has to have one additional buffer This comment is deprecated. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... File content/renderer/speech_recognition_dispatcher.cc (right): https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:299: void SpeechRecognitionDispatcher::OnAudioTrackError( Clearly not useful yet. Consider it a stub.
Looks impressive and a bit too big for me to swallow in one bite. Starting off with some simple comments where I have not done any detailed analysis. I think it takes a reviewer who has worked more on the code as primary reviewer here. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.h (right): https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:32: // WebRtcLocalAudioTrack and stores the capture data to a FIFO. I would say, "and stores the captured data in a FIFO". And "When there is enough data in the FIFO..." and perhaps also explain what is meant by enough. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:40: // Used for notifying the renderer client there is an issue with "...if/when there is an issue" https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:43: // Indicates a notification send failed. Recoverable. "Indicates that sending a notification failed" https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:45: // Indicates client hasn't consumed last buffer. Recoverable. "indicates client" feels wrong; can you rewrite? What about: "Indicates that a client..."? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:49: // Indicates the audio track has stopped. Provider can then be destroyed. "..that the audio track..." https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:63: // Determines the policy on what types of tracks are allowed. Is this correct. How can a method which returns true or false determine anything? It is a plain getter, right? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:67: // MediaStreamAudioSink implementation. No namespace here? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:77: // so it has been under the protection of |lock_|. "so it has been under..." sounds odd to me. Do you mean "..and a lock is therefore added to make the method thread safe"? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:81: // Notifies client there is an issue with delivering frames. "Notifies client there is" does not sound correct. I would say "Notifies the client when there is an issue..." https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:82: // TODO(burnik): Runs on capture thread. Should run on main renderer thread! This TODO needs a corresponding crbug. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:87: // consume it on the |output_bus_|. Size of the buffer is depends on the "is depends"?? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:124: // Whether the track has been stopped on the input. "stopped on the input"?? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:127: // Local counter of audio buffers for synchronization on consumed buffers. Isn't "of consumed" better? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:133: // Callback notifying an error has occured. .."notifying that an...", or "Callback which is activated when..."
Nice work in general. I haven't really looked at the unittest yet, it might be better for you to address the comments in production code and rebase your unittest on the changes before I look at the unittest. About the error callback code, I think it can be simplified a lot by removing some useless error codes. https://codereview.chromium.org/499233003/diff/80001/content/common/speech_re... File content/common/speech_recognition_messages.h (right): https://codereview.chromium.org/499233003/diff/80001/content/common/speech_re... content/common/speech_recognition_messages.h:21: #include "base/file_descriptor_posix.h" ?? why do you need this? Also, shouldn't this IPC msg be done on a separate CL? https://codereview.chromium.org/499233003/diff/80001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/499233003/diff/80001/content/content_tests.gy... content/content_tests.gypi:684: 'renderer/speech_recognition_audio_source_provider_unittest.cc', alphabet order https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.cc (right): https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:10: #include "base/time/time.h" nit, alphabet order https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:37: peer_buffer_index_ = &(buffer->params.size); I think it is a bit wrong, the shared_memory_ has not been used before, why should you read the value there? Simply, you can initialize peer_buffer_index_ to 0 here. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:53: bool SpeechRecognitionAudioSourceProvider::IsAllowedAudioTrack( IsAudioTrackSupported() seems a more suitable name here. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:55: DCHECK(track.source().type() == blink::WebMediaStreamSource::TypeAudio); you can't put DCHECK here, this method is trigger by JS, and developer can do whatever they want. Just return false if it is not TypeAudio https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:58: DCHECK(native_source); Same here, return false if native_source does not exist. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:73: fifo_buffer_size_ = output_params_.frames_per_buffer() * how is this cast? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:98: if (track_stopped_) return; new line. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:99: if (state == blink::WebMediaStreamSource::ReadyStateEnded) { add an empty line before the second if ( https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:101: MediaStreamAudioSink::RemoveFromAudioTrack(this, track_); Remove this line of code. track_ has already been ended, you should not call into the track_ any more. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:102: NotifyErrorState(ErrorState::TRACK_STOPPED); hmm, track ended state is not an error, ErrorState should not include TRACK_STOPPED at all. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:115: NotifyErrorState(ErrorState::AUDIO_FIFO_OVERFLOW); Log it. Also, could you please explain what the client supposes to do when getting a AUDIO_FIFO_OVERFLOW callback? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:125: if (fifo_->frames() < fifo_buffer_size_) return; empty line for the return https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:160: audio_bus->Zero(); do you know if the else case can happen here? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:161: return 1.0; empty line before the return. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.h (right): https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:48: AUDIO_FIFO_OVERFLOW, I am not sure if there is any value to most of these error codes here? what the client is supposed to do when getting errors like SEND_FAILED, BUFFER_SYNC_LAG and AUIDO_FIFO_OVERFLOW? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:104: base::SyncSocket* socket_; why the socket_ is raw pointer? who owns it? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider_unittest.cc (right): https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider_unittest.cc:31: uint8 data[100000]; noooooo, you can't allocate 100000 bytes in stack like this, change it the code to use heap.
burnik@chromium.org changed reviewers: + jamesr@chromium.org, kenrb@chromium.org - phoglund@chromium.org
Cleaned up a bit and simplified. Hoping to get more feedback from other reviewers. Special focus on the unit test. +kenrb for IPC +jamesr for Content https://codereview.chromium.org/499233003/diff/80001/content/common/speech_re... File content/common/speech_recognition_messages.h (right): https://codereview.chromium.org/499233003/diff/80001/content/common/speech_re... content/common/speech_recognition_messages.h:21: #include "base/file_descriptor_posix.h" Legacy from before SyncSocket::TransitDescriptor. Removed. Will add IPC reviewers. On 2014/09/15 08:31:27, xians1 wrote: > ?? why do you need this? > > Also, shouldn't this IPC msg be done on a separate CL? https://codereview.chromium.org/499233003/diff/80001/content/content_tests.gypi File content/content_tests.gypi (right): https://codereview.chromium.org/499233003/diff/80001/content/content_tests.gy... content/content_tests.gypi:684: 'renderer/speech_recognition_audio_source_provider_unittest.cc', On 2014/09/15 08:31:27, xians1 wrote: > alphabet order Done. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.cc (right): https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:10: #include "base/time/time.h" Alphabetic order of what? On 2014/09/15 08:31:28, xians1 wrote: > nit, alphabet order https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:37: peer_buffer_index_ = &(buffer->params.size); It has been used, on the browser process. Was init to 0 upon alloc and share. Makes sense to me to alloc and init in the same place. On 2014/09/15 08:31:29, xians1 wrote: > I think it is a bit wrong, the shared_memory_ has not been used before, why > should you read the value there? > Simply, you can initialize peer_buffer_index_ to 0 here. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:53: bool SpeechRecognitionAudioSourceProvider::IsAllowedAudioTrack( "Supported" would indicate there is a technical barrier to supporting. Here it's actually a policy because of the dreaded *abuse* SR could experience. On 2014/09/15 08:31:29, xians1 wrote: > IsAudioTrackSupported() seems a more suitable name here. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:55: DCHECK(track.source().type() == blink::WebMediaStreamSource::TypeAudio); True, no checks were done elsewhere. Done. On 2014/09/15 08:31:28, xians1 wrote: > you can't put DCHECK here, this method is trigger by JS, and developer can do > whatever they want. > Just return false if it is not TypeAudio https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:58: DCHECK(native_source); On 2014/09/15 08:31:28, xians1 wrote: > Same here, return false if native_source does not exist. Done. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:73: fifo_buffer_size_ = output_params_.frames_per_buffer() * Floored. Integer division. On 2014/09/15 08:31:28, xians1 wrote: > how is this cast? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:98: if (track_stopped_) return; Done. However, clang-format proposes this way. On 2014/09/15 08:31:27, xians1 wrote: > new line. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:99: if (state == blink::WebMediaStreamSource::ReadyStateEnded) { On 2014/09/15 08:31:28, xians1 wrote: > add an empty line before the second if ( Done. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:101: MediaStreamAudioSink::RemoveFromAudioTrack(this, track_); Are you sure? Will the MediaStreamAudioSink remove the track on it's own? Can you point me to that code, please? This is paired with the dtor of the class. On 2014/09/15 08:31:28, xians1 wrote: > Remove this line of code. > track_ has already been ended, you should not call into the track_ any more. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:102: NotifyErrorState(ErrorState::TRACK_STOPPED); Agreed. It's here for now as I refactor. On 2014/09/15 08:31:29, xians1 wrote: > hmm, track ended state is not an error, ErrorState should not include > TRACK_STOPPED at all. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:115: NotifyErrorState(ErrorState::AUDIO_FIFO_OVERFLOW); Logged via DLOG(ERROR). Client can destroy the audio source provider and potentially end the session early. On 2014/09/15 08:31:28, xians1 wrote: > Log it. > Also, could you please explain what the client supposes to do when getting a > AUDIO_FIFO_OVERFLOW callback? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:125: if (fifo_->frames() < fifo_buffer_size_) return; On 2014/09/15 08:31:28, xians1 wrote: > empty line for the return Done. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:160: audio_bus->Zero(); Yes. The else happens when we attach to the converter in |OnSetFormat|. Otherwise wouldn't be removing the |attached_converter_|. On 2014/09/15 08:31:29, xians1 wrote: > do you know if the else case can happen here? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:161: return 1.0; On 2014/09/15 08:31:28, xians1 wrote: > empty line before the return. Done. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.h (right): https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:32: // WebRtcLocalAudioTrack and stores the capture data to a FIFO. Comment expanded. On 2014/09/12 12:27:46, henrika wrote: > I would say, "and stores the captured data in a FIFO". And "When there is enough > data in the FIFO..." and perhaps also explain what is meant by enough. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:40: // Used for notifying the renderer client there is an issue with Removed enum. On 2014/09/12 12:27:46, henrika wrote: > "...if/when there is an issue" https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:43: // Indicates a notification send failed. Recoverable. Ditto. On 2014/09/12 12:27:47, henrika wrote: > "Indicates that sending a notification failed" https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:45: // Indicates client hasn't consumed last buffer. Recoverable. Ditto. On 2014/09/12 12:27:47, henrika wrote: > "indicates client" feels wrong; can you rewrite? What about: "Indicates that a > client..."? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:48: AUDIO_FIFO_OVERFLOW, Logged instead. Only stop propagated via callback. On 2014/09/15 08:31:29, xians1 wrote: > I am not sure if there is any value to most of these error codes here? what the > client is supposed to do when getting errors like SEND_FAILED, BUFFER_SYNC_LAG > and AUIDO_FIFO_OVERFLOW? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:49: // Indicates the audio track has stopped. Provider can then be destroyed. Ditto. On 2014/09/12 12:27:46, henrika wrote: > "..that the audio track..." https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:63: // Determines the policy on what types of tracks are allowed. Good point. We determine. Implementation enforces. And is used outside via the client. On 2014/09/12 12:27:47, henrika wrote: > Is this correct. How can a method which returns true or false determine > anything? It is a plain getter, right? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:67: // MediaStreamAudioSink implementation. It's |content| as well. But added. On 2014/09/12 12:27:46, henrika wrote: > No namespace here? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:77: // so it has been under the protection of |lock_|. Comment deprecated. On 2014/09/12 12:27:46, henrika wrote: > "so it has been under..." sounds odd to me. Do you mean "..and a lock is > therefore added to make the method thread safe"? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:81: // Notifies client there is an issue with delivering frames. Removed from design. On 2014/09/12 12:27:46, henrika wrote: > "Notifies client there is" does not sound correct. I would say "Notifies the > client when there is an issue..." https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:82: // TODO(burnik): Runs on capture thread. Should run on main renderer thread! TODO was for before landing. Removed from design. On 2014/09/12 12:27:47, henrika wrote: > This TODO needs a corresponding crbug. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:87: // consume it on the |output_bus_|. Size of the buffer is depends on the On 2014/09/12 12:27:46, henrika wrote: > "is depends"?? Done. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:104: base::SyncSocket* socket_; Client owns it (renderer - dispatcher). This is for dependency injection as well. On 2014/09/15 08:31:29, xians1 wrote: > why the socket_ is raw pointer? who owns it? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:124: // Whether the track has been stopped on the input. On 2014/09/12 12:27:47, henrika wrote: > "stopped on the input"?? Done. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:127: // Local counter of audio buffers for synchronization on consumed buffers. Looks excessive actually. Removed. On 2014/09/12 12:27:47, henrika wrote: > Isn't "of consumed" better? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:133: // Callback notifying an error has occured. Removed from design. Replaced by OnStoppedCB and commented. On 2014/09/12 12:27:47, henrika wrote: > .."notifying that an...", or "Callback which is activated when..." https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider_unittest.cc (right): https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider_unittest.cc:29: // Buffer to be shared between two fake sockets. On 2014/09/12 12:09:12, burnik wrote: > 'fake' is interchangeable with 'mock' regarding sockets. Done. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider_unittest.cc:31: uint8 data[100000]; On 2014/09/15 08:31:29, xians1 wrote: > noooooo, you can't allocate 100000 bytes in stack like this, change it the code > to use heap. This is allocated on and owned by the FakeSpeechRecognizer. Size is reduced to 8. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider_unittest.cc:442: // (3) Miraculasly recovered from the socket failure. On 2014/09/12 12:09:12, burnik wrote: > * Miraculously Done. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider_unittest.cc:453: // First round of input has to have one additional buffer On 2014/09/12 12:09:12, burnik wrote: > This comment is deprecated. Done. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... File content/renderer/speech_recognition_dispatcher.cc (right): https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_dispatcher.cc:299: void SpeechRecognitionDispatcher::OnAudioTrackError( Refactored stub - just used for detecting a stop. Refactoring further in next iteration. On 2014/09/12 12:09:12, burnik wrote: > Clearly not useful yet. Consider it a stub.
Could this go in content/renderer/media/ instead?
ipc lgtm
On 2014/09/15 17:22:10, jamesr wrote: > Could this go in content/renderer/media/ instead? Done. Next patchset relocates.
https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.cc (right): https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:9: #include "base/threading/thread_restrictions.h" why do you have this base/threading/thread_restrictions.h in both the header and implementation? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:10: #include "base/time/time.h" On 2014/09/15 15:00:06, burnik wrote: > Alphabetic order of what? > On 2014/09/15 08:31:28, xians1 wrote: > > nit, alphabet order > I was wrong, ignore it. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:53: bool SpeechRecognitionAudioSourceProvider::IsAllowedAudioTrack( On 2014/09/15 15:00:05, burnik wrote: > "Supported" would indicate there is a technical barrier to supporting. Here it's > actually a policy because of the dreaded *abuse* SR could experience. > On 2014/09/15 08:31:29, xians1 wrote: > > IsAudioTrackSupported() seems a more suitable name here. > The policy you mentioned is just one of the purposes why we have this method, the |track| can be any track JS injects, like video track, remote audio track, or screen cast track. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:73: fifo_buffer_size_ = output_params_.frames_per_buffer() * On 2014/09/15 15:00:05, burnik wrote: > Floored. Integer division. > On 2014/09/15 08:31:28, xians1 wrote: > > how is this cast? > Do it in C++ way, add static_cast<int>() here. Also, could you explain why floor is used instead of ceiling? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.h (right): https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.h:104: base::SyncSocket* socket_; On 2014/09/15 15:00:07, burnik wrote: > Client owns it (renderer - dispatcher). This is for dependency injection as > well. > On 2014/09/15 08:31:29, xians1 wrote: > > why the socket_ is raw pointer? who owns it? > I looked at the dispatcher code, it has: audio_source_provider_.reset(new SpeechRecognitionAudioSourceProvider( audio_track_, params, memory, socket.release(), ... Which clearly passes the ownership to the audio_source_provider_. The socket is leaking here. Also, I don't think the dispatcher should own the socket at all. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... File content/renderer/speech_recognition_audio_source_provider.cc (right): https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:28: DCHECK(params.IsValid()); add an DCHECK here: DCHECK(IsAllowedAudioTrack(track)); https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:36: // Keep params for sync with client via |params.size| on the shared memory. move this comment up above uint8* ptr = static_cast<uint8*>(shared_memory_.memory()); https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:38: // Client must manage his own counter and reset it. add an empty line before the comment. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:41: // Connect the source provider to the track as a sink. ditto https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:47: if (audio_converter_.get()) audio_converter_->RemoveInput(this); new line after the if (), this comment applies to the rest of the code in this file. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:57: MediaStreamAudioSource* native_source = nit, add an empty line. after the return false; https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:59: if (!native_source) return false; ditto https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:78: DCHECK_GE(fifo_buffer_size_, output_params_.frames_per_buffer()); these two DCHECKs are not always right, for example, what if input_params_.sample_rate() is 8K and output_params_.sample_rate() is 16K. Some more thought is needed here. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:99: if (track_stopped_) You should never get OnReadyStateChanged() more than once. DCHECK(!track_stopped_); https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:146: // The send usually fails if the user changes his input audio device. is this comment true? https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:150: // in |ErrorState::AUDIO_FIFO_OVERFLOW|. update the comment. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:164: audio_bus->Zero(); can the else case be possible at all? https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... File content/renderer/speech_recognition_audio_source_provider.h (right): https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.h:13: #include "base/threading/thread_restrictions.h" I might just miss some code, why this thread_restrictions.h is needed? https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.h:17: #include "media/audio/audio_device_thread.h" same question for this audio_device_thread.h https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.h:18: #include "media/audio/audio_parameters.h" you are forward declare the AudioParameters in line 26, move this include to the implementation https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.h:24: class AudioConverter; you include the audio_converter.h, so you don't need this forward declaration. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.h:56: // content::MediaStreamAudioSink implementation. these implementation of interfaces all can be private. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.h:114: const uint32* peer_buffer_index_; you don't need this member variable, use a local variable in OnData() instead. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... File content/renderer/speech_recognition_audio_source_provider_unittest.cc (right): https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:70: if (in_failure_mode_) return 0; new line for the if () https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:74: on_send_cb_.Run(); add an empty line before on_send_cb_.Run(); https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:82: // Since buffer is used atomically, we can reset the buffer indices here. empty line. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:103: virtual void EmulateReceiveThreadLoopIteration(); why is it virtual? https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:111: DCHECK(foreign_socket_.get()); remove this DCHECK since you derefer the pointer below anyway. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:189: void FakeSpeechRecognizer::EmulateReceiveThreadLoopIteration() { nit, just inline the implementation in lin 103 https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:199: // Input audio format nit, end with period https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:200: static const media::AudioParameters::Format kInputFormat = put all these declaration under anonymous namespace https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:232: virtual void SetUp() OVERRIDE { just do these SetUp in the constructor, and use the constructor initializer to initialize some members. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:287: WebRtcAudioDeviceImpl* device = new WebRtcAudioDeviceImpl(); you are leaking this |device|. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:389: // fill audio input frames with 0,1,2,3,...,440 all the comments start with capital letter and end with period. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:390: for (size_t i = 0; i < source_data_length_; ++i) source_data_[i] = i; empty line
Patchset #7 (id:120001) has been deleted
Patchset #7 (id:140001) has been deleted
Refactoring unit test and source provider, moved to media
Comments addressed. A few questions still open. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.cc (right): https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:9: #include "base/threading/thread_restrictions.h" Removed from implementation. On 2014/09/16 12:44:05, xians1 wrote: > why do you have this base/threading/thread_restrictions.h in both the header and > implementation? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:10: #include "base/time/time.h" On 2014/09/16 12:44:05, xians1 wrote: > On 2014/09/15 15:00:06, burnik wrote: > > Alphabetic order of what? > > On 2014/09/15 08:31:28, xians1 wrote: > > > nit, alphabet order > > > > I was wrong, ignore it. Acknowledged. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:73: fifo_buffer_size_ = output_params_.frames_per_buffer() * Input and output params are of media::AudioParameters type. All members here are int. Integer division omits decimals. Added DCHECK(output_params_.IsValid()); to next patchset which will check if output sample rate is 0. In production - input will be 44100 with 441 frames and output will be 16000 with 1600 frames. Also, DCHECKS which follow check if we have enough buffer. On 2014/09/16 12:44:05, xians1 wrote: > On 2014/09/15 15:00:05, burnik wrote: > > Floored. Integer division. > > On 2014/09/15 08:31:28, xians1 wrote: > > > how is this cast? > > > > Do it in C++ way, add static_cast<int>() here. > > Also, could you explain why floor is used instead of ceiling? https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... File content/renderer/speech_recognition_audio_source_provider.cc (right): https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:9: #include "base/threading/thread_restrictions.h" Removed. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:28: DCHECK(params.IsValid()); On 2014/09/16 12:44:06, xians1 wrote: > add an DCHECK here: > DCHECK(IsAllowedAudioTrack(track)); Done. And s/IsAllowedAudioTrack/IsSupportedTrack/. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:36: // Keep params for sync with client via |params.size| on the shared memory. Done. And refactored. On 2014/09/16 12:44:06, xians1 wrote: > move this comment up above uint8* ptr = > static_cast<uint8*>(shared_memory_.memory()); https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:38: // Client must manage his own counter and reset it. On 2014/09/16 12:44:06, xians1 wrote: > add an empty line before the comment. Done. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:41: // Connect the source provider to the track as a sink. On 2014/09/16 12:44:06, xians1 wrote: > ditto Done. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:47: if (audio_converter_.get()) audio_converter_->RemoveInput(this); On 2014/09/16 12:44:06, xians1 wrote: > new line after the if (), this comment applies to the rest of the code in this > file. Done. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:57: MediaStreamAudioSource* native_source = On 2014/09/16 12:44:06, xians1 wrote: > nit, add an empty line. after the return false; Done. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:59: if (!native_source) return false; On 2014/09/16 12:44:05, xians1 wrote: > ditto Done. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:78: DCHECK_GE(fifo_buffer_size_, output_params_.frames_per_buffer()); Do we ever have mic input which is 8K? What should be the size of the FIFO then in a general case? On 2014/09/16 12:44:06, xians1 wrote: > these two DCHECKs are not always right, for example, what if > input_params_.sample_rate() is 8K and output_params_.sample_rate() is 16K. > Some more thought is needed here. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:99: if (track_stopped_) On 2014/09/16 12:44:05, xians1 wrote: > You should never get OnReadyStateChanged() more than once. > > DCHECK(!track_stopped_); Done. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:146: // The send usually fails if the user changes his input audio device. As far as I've tested, yes. On 2014/09/16 12:44:06, xians1 wrote: > is this comment true? https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:150: // in |ErrorState::AUDIO_FIFO_OVERFLOW|. On 2014/09/16 12:44:05, xians1 wrote: > update the comment. Done. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:164: audio_bus->Zero(); Yes. Shown many times while testing. This occurs when we attach the sink via *audio_converter_->AddInput(this)*. This is related to the story of the now deprecated |attached_converter_| if you look back to one of the earliest patchsets. On 2014/09/16 12:44:06, xians1 wrote: > can the else case be possible at all? https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... File content/renderer/speech_recognition_audio_source_provider.h (right): https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.h:13: #include "base/threading/thread_restrictions.h" Legacy from tests. Removed. On 2014/09/16 12:44:06, xians1 wrote: > I might just miss some code, why this thread_restrictions.h is needed? https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.h:17: #include "media/audio/audio_device_thread.h" On 2014/09/16 12:44:06, xians1 wrote: > same question for this audio_device_thread.h Done. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.h:18: #include "media/audio/audio_parameters.h" On 2014/09/16 12:44:06, xians1 wrote: > you are forward declare the AudioParameters in line 26, move this include to the > implementation Done. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.h:24: class AudioConverter; On 2014/09/16 12:44:06, xians1 wrote: > you include the audio_converter.h, so you don't need this forward declaration. Done. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.h:56: // content::MediaStreamAudioSink implementation. On 2014/09/16 12:44:06, xians1 wrote: > these implementation of interfaces all can be private. Done. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.h:114: const uint32* peer_buffer_index_; I must disagree. This would mean I should keep the buffer (media::AudioInputBuffer) as a member instead. Which means I should always access the output bus via that *buffer_* member, instead of directly. Other way would be to cast the shared memory again to media::AudioInputBuffer in OnData - aka code duplication. Third point: kills symmetry of having input/output bus and local/peer counter IMHO. I remind here that media::AudioInputBuffer is just a shared memory "packaging" convenience here and my CL for rectifying that is here: http://crrev.com/526113002 On 2014/09/16 12:44:06, xians1 wrote: > you don't need this member variable, use a local variable in OnData() instead. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... File content/renderer/speech_recognition_audio_source_provider_unittest.cc (right): https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:70: if (in_failure_mode_) return 0; On 2014/09/16 12:44:07, xians1 wrote: > new line for the if () Done. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:74: on_send_cb_.Run(); On 2014/09/16 12:44:07, xians1 wrote: > add an empty line before on_send_cb_.Run(); Done. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:82: // Since buffer is used atomically, we can reset the buffer indices here. On 2014/09/16 12:44:07, xians1 wrote: > empty line. Done. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:103: virtual void EmulateReceiveThreadLoopIteration(); Legacy. Removed. On 2014/09/16 12:44:07, xians1 wrote: > why is it virtual? https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:109: // Used to simulate a problem with sockets. Added newline before comment. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:111: DCHECK(foreign_socket_.get()); On 2014/09/16 12:44:07, xians1 wrote: > remove this DCHECK since you derefer the pointer below anyway. Done. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:189: void FakeSpeechRecognizer::EmulateReceiveThreadLoopIteration() { On 2014/09/16 12:44:07, xians1 wrote: > nit, just inline the implementation in lin 103 Done. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:199: // Input audio format On 2014/09/16 12:44:07, xians1 wrote: > nit, end with period Done. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:200: static const media::AudioParameters::Format kInputFormat = On 2014/09/16 12:44:07, xians1 wrote: > put all these declaration under anonymous namespace Done. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:228: // Mock for error callback. Deprecated. It's mock for when the track is stopped. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:232: virtual void SetUp() OVERRIDE { Ok, can you provide an example? On 2014/09/16 12:44:06, xians1 wrote: > just do these SetUp in the constructor, and use the constructor initializer to > initialize some members. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:279: static blink::WebMediaStreamTrack CreateBlinkTrackWithMediaStreamType( This method is now refactored, simplified and reused in the test constructor. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:287: WebRtcAudioDeviceImpl* device = new WebRtcAudioDeviceImpl(); Removed it from test. On 2014/09/16 12:44:07, xians1 wrote: > you are leaking this |device|. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:345: // Audio related members. Removed unnecessary members. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:389: // fill audio input frames with 0,1,2,3,...,440 On 2014/09/16 12:44:07, xians1 wrote: > all the comments start with capital letter and end with period. Done. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:390: for (size_t i = 0; i < source_data_length_; ++i) source_data_[i] = i; On 2014/09/16 12:44:07, xians1 wrote: > empty line Done. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider_unittest.cc:436: // (3) Miraculasly recovered from the socket failure. *Miraculously -- And miraculously removed from test.
burnik@chromium.org changed reviewers: + jochen@chromium.org - gshires@chromium.org
Reviewers +jochen -gshires1
Some more comments about the production code, I will try to take a look at the unittest later. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.cc (right): https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:73: fifo_buffer_size_ = output_params_.frames_per_buffer() * On 2014/09/16 19:10:22, burnik wrote: > Input and output params are of media::AudioParameters type. > All members here are int. Integer division omits decimals. > Added DCHECK(output_params_.IsValid()); to next patchset which will check if > output sample rate is 0. > In production - input will be 44100 with 441 frames and output will be 16000 > with 1600 frames. > Also, DCHECKS which follow check if we have enough buffer. > The example you are taking is just what it is on your machine, the input sample rate can be any of the hardware sample rates, from 8k up to 192k https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... File content/renderer/speech_recognition_audio_source_provider.cc (right): https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:78: DCHECK_GE(fifo_buffer_size_, output_params_.frames_per_buffer()); On 2014/09/16 19:10:23, burnik wrote: > Do we ever have mic input which is 8K? What should be the size of the FIFO then > in a general case? > On 2014/09/16 12:44:06, xians1 wrote: > > these two DCHECKs are not always right, for example, what if > > input_params_.sample_rate() is 8K and output_params_.sample_rate() is 16K. > > Some more thought is needed here. > Yes, 8K as sample rate does exist on all platforms. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:146: // The send usually fails if the user changes his input audio device. On 2014/09/16 19:10:22, burnik wrote: > As far as I've tested, yes. > On 2014/09/16 12:44:06, xians1 wrote: > > is this comment true? > Interesting, to double check, if you changed the input device used by getUserMedia when connecting to recognition, it will trigger this code? https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:164: audio_bus->Zero(); On 2014/09/16 19:10:23, burnik wrote: > Yes. Shown many times while testing. This occurs when we attach the sink via > *audio_converter_->AddInput(this)*. > This is related to the story of the now deprecated |attached_converter_| if you > look back to one of the earliest patchsets. > On 2014/09/16 12:44:06, xians1 wrote: > > can the else case be possible at all? > Got it, thanks. https://codereview.chromium.org/499233003/diff/160001/content/common/speech_r... File content/common/speech_recognition_messages.h (right): https://codereview.chromium.org/499233003/diff/160001/content/common/speech_r... content/common/speech_recognition_messages.h:124: IPC_MESSAGE_ROUTED4(SpeechRecognitionMsg_AudioTrackReady, int /* request_id */, nit, one line for each param if all params do not fit in one line. https://codereview.chromium.org/499233003/diff/160001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_source_provider.cc (right): https://codereview.chromium.org/499233003/diff/160001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider.cc:49: if (audio_converter_.get()) audio_converter_->RemoveInput(this); nit, new line https://codereview.chromium.org/499233003/diff/160001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider.cc:142: DLOG(WARNING) << "Buffer synchronization lag"; hmm, thinking about these code a bit, it seems risky to me. Is it possible that something wrong with the socket, and the indexes gets out of sync and they will never get recovered because of this check? https://codereview.chromium.org/499233003/diff/160001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_source_provider.h (right): https://codereview.chromium.org/499233003/diff/160001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider.h:83: base::SyncSocket* socket_; Please see my previous comments about this socket_, you are leaking it.
Fixed leaking SyncSocket - appears in next patchest. Anyone else care to take a look at the unit test? https://codereview.chromium.org/499233003/diff/160001/content/common/speech_r... File content/common/speech_recognition_messages.h (right): https://codereview.chromium.org/499233003/diff/160001/content/common/speech_r... content/common/speech_recognition_messages.h:124: IPC_MESSAGE_ROUTED4(SpeechRecognitionMsg_AudioTrackReady, int /* request_id */, On 2014/09/17 15:55:19, xians1 wrote: > nit, one line for each param if all params do not fit in one line. Done. https://codereview.chromium.org/499233003/diff/160001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_source_provider.cc (right): https://codereview.chromium.org/499233003/diff/160001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider.cc:49: if (audio_converter_.get()) audio_converter_->RemoveInput(this); On 2014/09/17 15:55:19, xians1 wrote: > nit, new line Done. https://codereview.chromium.org/499233003/diff/160001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider.cc:142: DLOG(WARNING) << "Buffer synchronization lag"; If indices are out of sync, that would indicate that the |peer_buffer_index_| is less than |buffer_index_|. The return statement here gives a chance to the peer process to catch up. If in turn it doesn't, we will end up in the NOTREACHED() and in production just ignore any incoming data since the FIFO is already full. On 2014/09/17 15:55:19, xians1 wrote: > hmm, thinking about these code a bit, it seems risky to me. Is it possible that > something wrong with the socket, and the indexes gets out of sync and they will > never get recovered because of this check? https://codereview.chromium.org/499233003/diff/160001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_source_provider.h (right): https://codereview.chromium.org/499233003/diff/160001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider.h:83: base::SyncSocket* socket_; Fixed. On 2014/09/17 15:55:19, xians1 wrote: > Please see my previous comments about this socket_, you are leaking it.
xians: Wanted to check with you if this is what you meant by calculating the size of the FIFO. https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.cc (right): https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:73: fifo_buffer_size_ = output_params_.frames_per_buffer() * On 2014/09/17 15:55:19, xians1 wrote: > On 2014/09/16 19:10:22, burnik wrote: > > Input and output params are of media::AudioParameters type. > > All members here are int. Integer division omits decimals. > > Added DCHECK(output_params_.IsValid()); to next patchset which will check if > > output sample rate is 0. > > In production - input will be 44100 with 441 frames and output will be 16000 > > with 1600 frames. > > Also, DCHECKS which follow check if we have enough buffer. > > > > The example you are taking is just what it is on your machine, the input sample > rate can be any of the hardware sample rates, from 8k up to 192k Ok, Agreed. So if I do it this way: fifo_buffer_size_ = std::ceil(output_params_.frames_per_buffer() * static_cast<double>(input_params_.sample_rate()) / output_params_.sample_rate()); I've tested, and it would work properly for these: ================================ in.sr in.fpb out.sr out.fpb -------------------------------- 8000 80 16000 1600 8000 800 16000 1600 16000 160 16000 1600 16000 1600 16000 1600 32000 320 16000 1600 32000 3200 16000 1600 44100 441 16000 1600 44100 4410 16000 1600 48000 480 16000 1600 48000 4800 16000 1600 96000 960 16000 1600 96000 9600 16000 1600 11025 111* 16000 1600 11025 1103* 16000 1600 22050 221* 16000 1600 22050 2205 16000 1600 88200 882 16000 1600 88200 8820 16000 1600 176400 1764 16000 1600 176400 17640 16000 1600 192000 1920 16000 1600 192000 19200 16000 1600 ================================ * These starred are always rounded up, right? https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... File content/renderer/speech_recognition_audio_source_provider.cc (right): https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:78: DCHECK_GE(fifo_buffer_size_, output_params_.frames_per_buffer()); True, removed the second DCHECK. It doesn't really make sense anyway since the FIFO is used for buffering the input. On 2014/09/17 15:55:19, xians1 wrote: > On 2014/09/16 19:10:23, burnik wrote: > > Do we ever have mic input which is 8K? What should be the size of the FIFO > then > > in a general case? > > On 2014/09/16 12:44:06, xians1 wrote: > > > these two DCHECKs are not always right, for example, what if > > > input_params_.sample_rate() is 8K and output_params_.sample_rate() is 16K. > > > Some more thought is needed here. > > > > Yes, 8K as sample rate does exist on all platforms. https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:146: // The send usually fails if the user changes his input audio device. If I change input device from System Sound on linux, this occurs. On 2014/09/17 15:55:19, xians1 wrote: > On 2014/09/16 19:10:22, burnik wrote: > > As far as I've tested, yes. > > On 2014/09/16 12:44:06, xians1 wrote: > > > is this comment true? > > > > Interesting, to double check, if you changed the input device used by > getUserMedia when connecting to recognition, it will trigger this code? https://codereview.chromium.org/499233003/diff/100001/content/renderer/speech... content/renderer/speech_recognition_audio_source_provider.cc:164: audio_bus->Zero(); On 2014/09/17 15:55:19, xians1 wrote: > On 2014/09/16 19:10:23, burnik wrote: > > Yes. Shown many times while testing. This occurs when we attach the sink via > > *audio_converter_->AddInput(this)*. > > This is related to the story of the now deprecated |attached_converter_| if > you > > look back to one of the earliest patchsets. > > On 2014/09/16 12:44:06, xians1 wrote: > > > can the else case be possible at all? > > > > Got it, thanks. Acknowledged.
Hi Kristijan, did you forget to upload a new version? https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... File content/renderer/speech_recognition_audio_source_provider.cc (right): https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... content/renderer/speech_recognition_audio_source_provider.cc:73: fifo_buffer_size_ = output_params_.frames_per_buffer() * On 2014/09/18 19:09:21, burnik wrote: > On 2014/09/17 15:55:19, xians1 wrote: > > On 2014/09/16 19:10:22, burnik wrote: > > > Input and output params are of media::AudioParameters type. > > > All members here are int. Integer division omits decimals. > > > Added DCHECK(output_params_.IsValid()); to next patchset which will check if > > > output sample rate is 0. > > > In production - input will be 44100 with 441 frames and output will be 16000 > > > with 1600 frames. > > > Also, DCHECKS which follow check if we have enough buffer. > > > > > > > The example you are taking is just what it is on your machine, the input > sample > > rate can be any of the hardware sample rates, from 8k up to 192k > > Ok, Agreed. > > So if I do it this way: > > fifo_buffer_size_ = > std::ceil(output_params_.frames_per_buffer() * > static_cast<double>(input_params_.sample_rate()) / > output_params_.sample_rate()); > > I've tested, and it would work properly for these: > > ================================ > in.sr in.fpb out.sr out.fpb > -------------------------------- > 8000 80 16000 1600 > 8000 800 16000 1600 > 16000 160 16000 1600 > 16000 1600 16000 1600 > 32000 320 16000 1600 > 32000 3200 16000 1600 > 44100 441 16000 1600 > 44100 4410 16000 1600 > 48000 480 16000 1600 > 48000 4800 16000 1600 > 96000 960 16000 1600 > 96000 9600 16000 1600 > 11025 111* 16000 1600 > 11025 1103* 16000 1600 > 22050 221* 16000 1600 > 22050 2205 16000 1600 > 88200 882 16000 1600 > 88200 8820 16000 1600 > 176400 1764 16000 1600 > 176400 17640 16000 1600 > 192000 1920 16000 1600 > 192000 19200 16000 1600 > ================================ > > * These starred are always rounded up, right? > I think this looks correct.
On 2014/09/19 08:58:56, xians1 wrote: > Hi Kristijan, did you forget to upload a new version? > > https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... > File content/renderer/speech_recognition_audio_source_provider.cc (right): > > https://codereview.chromium.org/499233003/diff/80001/content/renderer/speech_... > content/renderer/speech_recognition_audio_source_provider.cc:73: > fifo_buffer_size_ = output_params_.frames_per_buffer() * > On 2014/09/18 19:09:21, burnik wrote: > > On 2014/09/17 15:55:19, xians1 wrote: > > > On 2014/09/16 19:10:22, burnik wrote: > > > > Input and output params are of media::AudioParameters type. > > > > All members here are int. Integer division omits decimals. > > > > Added DCHECK(output_params_.IsValid()); to next patchset which will check > if > > > > output sample rate is 0. > > > > In production - input will be 44100 with 441 frames and output will be > 16000 > > > > with 1600 frames. > > > > Also, DCHECKS which follow check if we have enough buffer. > > > > > > > > > > The example you are taking is just what it is on your machine, the input > > sample > > > rate can be any of the hardware sample rates, from 8k up to 192k > > > > Ok, Agreed. > > > > So if I do it this way: > > > > fifo_buffer_size_ = > > std::ceil(output_params_.frames_per_buffer() * > > static_cast<double>(input_params_.sample_rate()) / > > output_params_.sample_rate()); > > > > I've tested, and it would work properly for these: > > > > ================================ > > in.sr in.fpb out.sr out.fpb > > -------------------------------- > > 8000 80 16000 1600 > > 8000 800 16000 1600 > > 16000 160 16000 1600 > > 16000 1600 16000 1600 > > 32000 320 16000 1600 > > 32000 3200 16000 1600 > > 44100 441 16000 1600 > > 44100 4410 16000 1600 > > 48000 480 16000 1600 > > 48000 4800 16000 1600 > > 96000 960 16000 1600 > > 96000 9600 16000 1600 > > 11025 111* 16000 1600 > > 11025 1103* 16000 1600 > > 22050 221* 16000 1600 > > 22050 2205 16000 1600 > > 88200 882 16000 1600 > > 88200 8820 16000 1600 > > 176400 1764 16000 1600 > > 176400 17640 16000 1600 > > 192000 1920 16000 1600 > > 192000 19200 16000 1600 > > ================================ > > > > * These starred are always rounded up, right? > > > > I think this looks correct. xians: Did not forget. Still having some changes in prep and will upload shortly.
SyncSocket leak and FIFO fixes. Test 8-192Khz for input.
As I'm refactoring for the next patchset, I would like to get more opinions on the unit test. https://codereview.chromium.org/499233003/diff/180001/content/common/speech_r... File content/common/speech_recognition_messages.h (right): https://codereview.chromium.org/499233003/diff/180001/content/common/speech_r... content/common/speech_recognition_messages.h:10: #include "base/process/process_handle.h" Include is deprecated. Removed in next patchset. https://codereview.chromium.org/499233003/diff/180001/content/content_rendere... File content/content_renderer.gypi (right): https://codereview.chromium.org/499233003/diff/180001/content/content_rendere... content/content_renderer.gypi:397: 'renderer/speech_recognition_audio_source_provider.h', Removed. https://codereview.chromium.org/499233003/diff/180001/content/content_rendere... content/content_renderer.gypi:627: 'renderer/media/speech_recognition_audio_source_provider.cc', added a line after: 'renderer/media/speech_recognition_audio_source_provider.h', https://codereview.chromium.org/499233003/diff/180001/content/renderer/speech... File content/renderer/speech_recognition_dispatcher.h (right): https://codereview.chromium.org/499233003/diff/180001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.h:12: #include "base/message_loop/message_loop_proxy.h" Removed. https://codereview.chromium.org/499233003/diff/180001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.h:69: virtual void OnAudioTrackStopped(); Removed 'virtual'.
Don't qualify as expert reviewer on the unit test. Added generic comments. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_source_provider_unittest.cc (right): https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:84: //////////////////////////////////////////////////////////////////////////////// Please remove these non-standard separators. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:86: class FakeSpeechRecognizer { This looks like a very complex helper which is now without any comments. To me, it seems like a risk to add such a complex class to a test since the test might fail due to this special implementation instead of the actual code. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:167: //////////////////////////////////////////////////////////////////////////////// remove https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:197: // Initializes the producer and consumer with specified audio parameters. Can you elaborate on what a producer and consumer is in this test. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:262: // Capturer. These comments does not add much. Please explain what they do in the test or remove. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:294: base::TimeDelta::FromMilliseconds(0), 1, false, FromMilliseconds(0)? https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:355: TEST_F(SpeechRecognitionAudioSourceProviderTest, CheckIsSupportedAudioTrack) { Could you make the name more clear? CheckIsSupported is not clear to me. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:382: TEST_F(SpeechRecognitionAudioSourceProviderTest, RecognizerNotifiedOnSocket) { Please add some lines of comments above each test explaining some more about what you test. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:400: TEST_F(SpeechRecognitionAudioSourceProviderTest, AudioDataIsResampledOnSink) { Lots of hardcoded values in this test. Makes it difficult to understand what you test. Should it only work for these settings? https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:444: // (1) Start with no problems on the socket. Remove (1) and (2)
Addresed comments. Waiting for other reviewers before next patchset. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_source_provider_unittest.cc (right): https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:84: //////////////////////////////////////////////////////////////////////////////// On 2014/09/22 08:02:19, henrika wrote: > Please remove these non-standard separators. Done. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:86: class FakeSpeechRecognizer { On 2014/09/22 08:02:19, henrika wrote: > This looks like a very complex helper which is now without any comments. To me, > it seems like a risk to add such a complex class to a test since the test might > fail due to this special implementation instead of the actual code. This is the mock consumer. Unit tests focus on the class being tested (the producer here) and should show how that class is to be used as far as I know. I've added more comments to explain what the class does. I don't really see how this unit test could fail because of this helper class. The consumer code is covered in a different test. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:167: //////////////////////////////////////////////////////////////////////////////// On 2014/09/22 08:02:19, henrika wrote: > remove Done. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:197: // Initializes the producer and consumer with specified audio parameters. On 2014/09/22 08:02:18, henrika wrote: > Can you elaborate on what a producer and consumer is in this test. Yes. It's explained on lines 228 - 238. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:262: // Capturer. On 2014/09/22 08:02:18, henrika wrote: > These comments does not add much. Please explain what they do in the test or > remove. All these comments are now removed. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:294: base::TimeDelta::FromMilliseconds(0), 1, false, On 2014/09/22 08:02:18, henrika wrote: > FromMilliseconds(0)? Yes, no delay is required in the unit test. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:355: TEST_F(SpeechRecognitionAudioSourceProviderTest, CheckIsSupportedAudioTrack) { On 2014/09/22 08:02:18, henrika wrote: > Could you make the name more clear? CheckIsSupported is not clear to me. Added comment above test. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:382: TEST_F(SpeechRecognitionAudioSourceProviderTest, RecognizerNotifiedOnSocket) { On 2014/09/22 08:02:18, henrika wrote: > Please add some lines of comments above each test explaining some more about > what you test. Done. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:400: TEST_F(SpeechRecognitionAudioSourceProviderTest, AudioDataIsResampledOnSink) { On 2014/09/22 08:02:19, henrika wrote: > Lots of hardcoded values in this test. Makes it difficult to understand what you > test. Should it only work for these settings? Added more comments. I don't test that the resampler does a good job, since that is covered in it's own unit test. I use these typical values here just to make sure that the producer also does resampling when it delivers frames to the consumer. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:444: // (1) Start with no problems on the socket. On 2014/09/22 08:02:18, henrika wrote: > Remove (1) and (2) Done.
Great work, it is getting closer. https://codereview.chromium.org/499233003/diff/180001/content/common/speech_r... File content/common/speech_recognition_messages.h (right): https://codereview.chromium.org/499233003/diff/180001/content/common/speech_r... content/common/speech_recognition_messages.h:10: #include "base/process/process_handle.h" On 2014/09/22 07:43:12, burnik wrote: > Include is deprecated. Removed in next patchset. Acknowledged. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_source_provider.h (right): https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider.h:5: #ifndef CONTENT_RENDERER_SPEECH_RECOGNITION_AUDIO_SOURCE_PROVIDER_H_ update this macro, you have moved the code to media https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider.h:23: class AudioParameters; remove this forward declaration. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider.h:84: scoped_ptr<base::SyncSocket> socket_; You should use base::CancelableSyncSocket socket_ here, check how audio_device_thread.cc is done. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider.h:98: // Params of the source audio. Can change when |OnSetFormat| occurs. nit, s/|OnSetFormat|/OnSetFormat()/g https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_source_provider_unittest.cc (right): https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:84: //////////////////////////////////////////////////////////////////////////////// On 2014/09/22 09:17:36, burnik wrote: > On 2014/09/22 08:02:19, henrika wrote: > > Please remove these non-standard separators. > > Done. Not done yet. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:137: (*buffer_index_)++; ++(*buffer_index_) https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:158: MockSyncSocket* foreign_socket_; why this is a raw pointer? https://codereview.chromium.org/499233003/diff/180001/content/renderer/speech... File content/renderer/speech_recognition_dispatcher.cc (right): https://codereview.chromium.org/499233003/diff/180001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:80: WebSpeechRecognizerClient::NotAllowedError); return here, since we are failing the start() https://codereview.chromium.org/499233003/diff/180001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:84: // Destroy any previous instance not to starve it waiting on chunk ACKs. Please add more comment to explain why we stop the audio_source_provider_ when a new session is started. https://codereview.chromium.org/499233003/diff/180001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:177: recognizer_client_->didReceiveNoMatch(GetHandleFromID(request_id), what will happen when getting a didReceiveNoMatch callback? https://codereview.chromium.org/499233003/diff/180001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:255: audio_track_, params, memory, socket.release(), you don't need to create a socket at all, just pass the handle to SpeechRecognitionAudioSourceProvider. https://codereview.chromium.org/499233003/diff/180001/content/renderer/speech... File content/renderer/speech_recognition_dispatcher.h (right): https://codereview.chromium.org/499233003/diff/180001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.h:16: #include "content/renderer/media/speech_recognition_audio_source_provider.h" remove, because you forward declare it below.
https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_source_provider_unittest.cc (right): https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:86: class FakeSpeechRecognizer { I am not saying it will fail but that is a large helper without any comments. Hence, it is not clear to me why the test must contain all this code instead of using existing (possibly mock) instead.
Refactoring, error states, more comments.
New patchset is out. :-) https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_source_provider.h (right): https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider.h:5: #ifndef CONTENT_RENDERER_SPEECH_RECOGNITION_AUDIO_SOURCE_PROVIDER_H_ On 2014/09/23 10:09:12, xians1 wrote: > update this macro, you have moved the code to media Done. It is CONTENT_RENDERER_MEDIA_SPEECH_RECOGNITION_AUDIO_SOURCE_PROVIDER_H_ in next patchset. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider.h:23: class AudioParameters; On 2014/09/23 10:09:13, xians1 wrote: > remove this forward declaration. Done. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider.h:84: scoped_ptr<base::SyncSocket> socket_; On 2014/09/23 10:09:12, xians1 wrote: > You should use base::CancelableSyncSocket socket_ here, check how > audio_device_thread.cc is done. Ok. Let's say I pass in a CancelableSyncSocket from the dispatcher (renderer client). The SyncSocket is just owned here, but is injected as a pointer from the renderer client. I think then it's ok to have a SyncSocket here since I'm using only |Send()|? https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider.h:98: // Params of the source audio. Can change when |OnSetFormat| occurs. On 2014/09/23 10:09:12, xians1 wrote: > nit, s/|OnSetFormat|/OnSetFormat()/g Done. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_source_provider_unittest.cc (right): https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:67: for (size_t i = 0; i < length; i++, buffer_->length++) Changed to prefixed increment. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:76: for (size_t i = buffer_->start; i < buffer_->length; i++, buffer_->start++) Changed to prefixed increment. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:84: //////////////////////////////////////////////////////////////////////////////// On 2014/09/23 10:09:13, xians1 wrote: > On 2014/09/22 09:17:36, burnik wrote: > > On 2014/09/22 08:02:19, henrika wrote: > > > Please remove these non-standard separators. > > > > Done. > > Not done yet. Yes, done for next patchset as advertised. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:86: class FakeSpeechRecognizer { On 2014/09/23 10:45:33, henrika wrote: > I am not saying it will fail but that is a large helper without any comments. > Hence, it is not clear to me why the test must contain all this code instead of > using existing (possibly mock) instead. Ok. I'll revisit existing unit tests to see if anything can be reused. This could possibly happen after I add new stuff to other tests to check the consumer. Until then, I think it is a fairly simple helper showing how the consumer should behave. Also, I pointed out which methods are just mocks and helpers for testing. Generally, any code duplication should be avoided, in that sense I do agree. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:137: (*buffer_index_)++; On 2014/09/23 10:09:13, xians1 wrote: > ++(*buffer_index_) Done. https://codereview.chromium.org/499233003/diff/180001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:158: MockSyncSocket* foreign_socket_; On 2014/09/23 10:09:13, xians1 wrote: > why this is a raw pointer? It is owned by the recognizer and destroyed there. https://codereview.chromium.org/499233003/diff/180001/content/renderer/speech... File content/renderer/speech_recognition_dispatcher.cc (right): https://codereview.chromium.org/499233003/diff/180001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:80: WebSpeechRecognizerClient::NotAllowedError); On 2014/09/23 10:09:13, xians1 wrote: > return here, since we are failing the start() Done. https://codereview.chromium.org/499233003/diff/180001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:84: // Destroy any previous instance not to starve it waiting on chunk ACKs. On 2014/09/23 10:09:13, xians1 wrote: > Please add more comment to explain why we stop the audio_source_provider_ when a > new session is started. Done. https://codereview.chromium.org/499233003/diff/180001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:177: recognizer_client_->didReceiveNoMatch(GetHandleFromID(request_id), On 2014/09/23 10:09:13, xians1 wrote: > what will happen when getting a didReceiveNoMatch callback? This is an API protocol thing as far as I know, not a local problem. https://codereview.chromium.org/499233003/diff/180001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:255: audio_track_, params, memory, socket.release(), On 2014/09/23 10:09:13, xians1 wrote: > you don't need to create a socket at all, just pass the handle to > SpeechRecognitionAudioSourceProvider. That would be true if the unit test did not have a fake socket. It's impossible to inject a socket via handle, I would have to add a method or an overloaded constructor which would indicate a specialized member for tests - yucky :-|. Maybe the constructor should just indicate a scoped_ptr so we know the ownership is being transferred. https://codereview.chromium.org/499233003/diff/180001/content/renderer/speech... File content/renderer/speech_recognition_dispatcher.h (right): https://codereview.chromium.org/499233003/diff/180001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.h:16: #include "content/renderer/media/speech_recognition_audio_source_provider.h" On 2014/09/23 10:09:13, xians1 wrote: > remove, because you forward declare it below. Done.
Please ping once you have an OWNER for content/renderer/media approve this. https://codereview.chromium.org/499233003/diff/200001/content/renderer/speech... File content/renderer/speech_recognition_dispatcher.h (right): https://codereview.chromium.org/499233003/diff/200001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.h:64: // Called by IPC. this comment is not helpful. All of the On...() functions are IPC handlers, that naming convention is enough https://codereview.chromium.org/499233003/diff/200001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.h:68: // Called By |audio_source_provider_|. not a very useful comment either and not really true, since we're doing the bind call audio_source_provider_ doesn't link against this. You should give this a different name (so it doesn't look like an IPC handler) and put a blank line between this and the ipc handlers
jamesr: Quick follow up on comments. https://codereview.chromium.org/499233003/diff/200001/content/renderer/speech... File content/renderer/speech_recognition_dispatcher.h (right): https://codereview.chromium.org/499233003/diff/200001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.h:64: // Called by IPC. On 2014/09/23 23:31:39, jamesr wrote: > this comment is not helpful. All of the On...() functions are IPC handlers, > that naming convention is enough Done. https://codereview.chromium.org/499233003/diff/200001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.h:68: // Called By |audio_source_provider_|. On 2014/09/23 23:31:39, jamesr wrote: > not a very useful comment either and not really true, since we're doing the bind > call audio_source_provider_ doesn't link against this. > > You should give this a different name (so it doesn't look like an IPC handler) > and put a blank line between this and the ipc handlers How does this sound? // Callback for SpeechRecognitionAudioSourceProvider::OnStoppedCB. void AudioTrackStopped();
https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_source_provider.cc (right): https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider.cc:178: return 1.0; document what this means? https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_source_provider.h (right): https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider.h:27: // SpeechRecognitionAudioSourceProvider works as an audio sink to the Is 'source provider' a good name? The reason I'm wondering is because usually things are chained together like: src->track->sink, so if this is an audio sink, it feels odd to have it be called a "source provider". That being said, I haven't looked at the implementation so perhaps this will make sense to me in a bit. https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider.h:40: SpeechRecognitionAudioSourceProvider(const blink::WebMediaStreamTrack& track, can you document these parameters? particularly how ownership is passed around etc. https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider.h:43: base::SyncSocket* socket, actually, if ownership is being passed here, please use scoped_ptr<> here (and .Pass() on the caller side). https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_source_provider_unittest.cc (right): https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:29: SharedBuffer() : start(0), length(0) {} nit: what about also initializing data? SharedBuffer() : data(), start(0), length(0) {} https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:41: in_failure_mode_(false) { } nit: {} https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:66: uint8* b = static_cast<uint8*>(const_cast<void*>(buffer)); is this safe (if it is, please add a comment)? What if someone passes truly read-only data like e.g. Send("foo", 4)? https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:68: buffer_->data[buffer_->start + buffer_->length] = b[i]; hmm... I don't see why you need to cast away the constness... where do you write to |b|? https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:75: uint8* b = static_cast<uint8*>(const_cast<void*>(buffer)); buffer isn't const, so no need for the const_cast https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:110: buffer_index_ = &(buffer->params.size); what about just having a member variable of type media::AudioInputBuffer* instead? this feels like a bit more magic since it's just an int pointer into space, whereas the AudioInputBuffer will point to a more better defined type. https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:148: uint32 buffer_index() { return *buffer_index_; } isn't this returning 'size' rather than a buffer index? https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:372: const int kAudioParams[kNumAudioParamTuples][2] = { add 24000? https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:400: for (uint32 i = 0; i < kSourceDataLength; ++i) {} https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:407: int16 sink_data[kSinkDataLength]; nit: = {0}; https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:417: for (uint32 i = 0; i < kNumFramesToTest; ++i) {} https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:419: ASSERT_EQ(0, sink_data[i * kOutputChannels + c]); EXPECT_EQ? https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:434: for (uint32 i = 0; i < kNumFramesToTest; ++i) {} https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:436: ASSERT_EQ(kExpectedData[i], sink_data[i * kOutputChannels + c]); EXPECT_EQ? https://codereview.chromium.org/499233003/diff/200001/content/renderer/speech... File content/renderer/speech_recognition_dispatcher.cc (right): https://codereview.chromium.org/499233003/diff/200001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:33: next_id_(1) { } {} https://codereview.chromium.org/499233003/diff/200001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:39: audio_source_provider_.reset(); on which thread does this execute?
Good review round. There are a few open questions for jamesr, tommi and Shijing. Please reply before I push the next patchset. https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_source_provider.cc (right): https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider.cc:178: return 1.0; On 2014/09/24 09:51:59, tommi wrote: > document what this means? // Return volume greater than zero to indicate we have more data. SGTU? https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_source_provider.h (right): https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider.h:27: // SpeechRecognitionAudioSourceProvider works as an audio sink to the On 2014/09/24 09:51:59, tommi wrote: > Is 'source provider' a good name? The reason I'm wondering is because usually > things are chained together like: src->track->sink, so if this is an audio sink, > it feels odd to have it be called a "source provider". That being said, I > haven't looked at the implementation so perhaps this will make sense to me in a > bit. Did it make sense in the impl? My reasoning that this audio data is not being consumed here on the render, rather on the browser. Do you think I should rename it to SpeechRecognitionAudioSink? Shijing, your thoughts? https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider.h:40: SpeechRecognitionAudioSourceProvider(const blink::WebMediaStreamTrack& track, On 2014/09/24 09:51:59, tommi wrote: > can you document these parameters? particularly how ownership is passed around > etc. Added a TODO for next round. https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider.h:43: base::SyncSocket* socket, On 2014/09/24 09:51:59, tommi wrote: > actually, if ownership is being passed here, please use scoped_ptr<> here (and > .Pass() on the caller side). Done. I suppose I should initialize in constructor like this as well: : socket_(socket.Pass()) Right? https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_source_provider_unittest.cc (right): https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:29: SharedBuffer() : start(0), length(0) {} On 2014/09/24 09:52:00, tommi wrote: > nit: what about also initializing data? > > SharedBuffer() : data(), start(0), length(0) {} Done. https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:41: in_failure_mode_(false) { } On 2014/09/24 09:51:59, tommi wrote: > nit: {} Done. https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:66: uint8* b = static_cast<uint8*>(const_cast<void*>(buffer)); On 2014/09/24 09:51:59, tommi wrote: > is this safe (if it is, please add a comment)? What if someone passes truly > read-only data like e.g. Send("foo", 4)? Would this be safe? const uint8* b = static_cast<const uint8*>(buffer); https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:68: buffer_->data[buffer_->start + buffer_->length] = b[i]; On 2014/09/24 09:51:59, tommi wrote: > hmm... I don't see why you need to cast away the constness... where do you write > to |b|? Acknowledged. https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:75: uint8* b = static_cast<uint8*>(const_cast<void*>(buffer)); On 2014/09/24 09:51:59, tommi wrote: > buffer isn't const, so no need for the const_cast Done. https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:110: buffer_index_ = &(buffer->params.size); On 2014/09/24 09:51:59, tommi wrote: > what about just having a member variable of type media::AudioInputBuffer* > instead? > this feels like a bit more magic since it's just an int pointer into space, > whereas the AudioInputBuffer will point to a more better defined type. I think AudioInputBuffer is a terrible name for this purpose. It is used on both ends and that's the reason of proposing AudioSharedBuffer which would mitigate this magic. https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:148: uint32 buffer_index() { return *buffer_index_; } On 2014/09/24 09:51:59, tommi wrote: > isn't this returning 'size' rather than a buffer index? Again. I'm actually using 'size' to count the buffers. See what I mean with AudioSharedBuffer? https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:372: const int kAudioParams[kNumAudioParamTuples][2] = { On 2014/09/24 09:52:00, tommi wrote: > add 24000? Done. https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:400: for (uint32 i = 0; i < kSourceDataLength; ++i) On 2014/09/24 09:51:59, tommi wrote: > {} Done. https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:407: int16 sink_data[kSinkDataLength]; On 2014/09/24 09:51:59, tommi wrote: > nit: = {0}; Done. Does this array init-to-zero on stack work for all platforms and compilers we use? https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:417: for (uint32 i = 0; i < kNumFramesToTest; ++i) On 2014/09/24 09:51:59, tommi wrote: > {} Done. https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:419: ASSERT_EQ(0, sink_data[i * kOutputChannels + c]); On 2014/09/24 09:51:59, tommi wrote: > EXPECT_EQ? Done. https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:434: for (uint32 i = 0; i < kNumFramesToTest; ++i) On 2014/09/24 09:51:59, tommi wrote: > {} Done. https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider_unittest.cc:436: ASSERT_EQ(kExpectedData[i], sink_data[i * kOutputChannels + c]); On 2014/09/24 09:51:59, tommi wrote: > EXPECT_EQ? Done. https://codereview.chromium.org/499233003/diff/200001/content/renderer/speech... File content/renderer/speech_recognition_dispatcher.cc (right): https://codereview.chromium.org/499233003/diff/200001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:33: next_id_(1) { } On 2014/09/24 09:52:00, tommi wrote: > {} Done. https://codereview.chromium.org/499233003/diff/200001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:39: audio_source_provider_.reset(); On 2014/09/24 09:52:00, tommi wrote: > on which thread does this execute? Same as constructor would be my best guess. Actually looks like dead code - either legacy or a future thing. This is never called according to code search.
https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_source_provider.h (right): https://codereview.chromium.org/499233003/diff/200001/content/renderer/media/... content/renderer/media/speech_recognition_audio_source_provider.h:27: // SpeechRecognitionAudioSourceProvider works as an audio sink to the On 2014/09/24 09:51:59, tommi wrote: > Is 'source provider' a good name? The reason I'm wondering is because usually > things are chained together like: src->track->sink, so if this is an audio sink, > it feels odd to have it be called a "source provider". That being said, I > haven't looked at the implementation so perhaps this will make sense to me in a > bit. From the webspeech's perspective, this class is a source provider. From the audio track's perspective, this class is a audio sink. I don't have strong opinion here, that says, I am fine to rename it to SpeechRecognitionAudioSink.
On 2014/09/24 09:04:38, burnik wrote: > https://codereview.chromium.org/499233003/diff/200001/content/renderer/speech... > content/renderer/speech_recognition_dispatcher.h:68: // Called By > |audio_source_provider_|. > On 2014/09/23 23:31:39, jamesr wrote: > > not a very useful comment either and not really true, since we're doing the > bind > > call audio_source_provider_ doesn't link against this. > > > > You should give this a different name (so it doesn't look like an IPC handler) > > and put a blank line between this and the ipc handlers > > How does this sound? > > // Callback for SpeechRecognitionAudioSourceProvider::OnStoppedCB. > void AudioTrackStopped(); This is not a useful comment. Name the function for what it does and add a comment if what the function *does* is not obvious. Comments about how the function is used are out of place and almost certainly will go stale immediately. Say why, not what.
On 2014/09/24 18:40:25, jamesr wrote: > On 2014/09/24 09:04:38, burnik wrote: > > > https://codereview.chromium.org/499233003/diff/200001/content/renderer/speech... > > content/renderer/speech_recognition_dispatcher.h:68: // Called By > > |audio_source_provider_|. > > On 2014/09/23 23:31:39, jamesr wrote: > > > not a very useful comment either and not really true, since we're doing the > > bind > > > call audio_source_provider_ doesn't link against this. > > > > > > You should give this a different name (so it doesn't look like an IPC > handler) > > > and put a blank line between this and the ipc handlers > > > > How does this sound? > > > > // Callback for SpeechRecognitionAudioSourceProvider::OnStoppedCB. > > void AudioTrackStopped(); > > This is not a useful comment. Name the function for what it does and add a > comment if what the function *does* is not obvious. Comments about how the > function is used are out of place and almost certainly will go stale > immediately. Say why, not what. The thing is, this is like an event raised from the audio_source_provider_ so that's the name's origin. But if you insist, this will be in the next patchset: void ResetAudioSourceProvider(); Essentially that's all that it does for now.
Patchset 10. Renamed the new class and unit test. Nits addressed. Any owners feel like stamping? :)
https://codereview.chromium.org/499233003/diff/220001/content/renderer/speech... File content/renderer/speech_recognition_dispatcher.cc (right): https://codereview.chromium.org/499233003/diff/220001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:79: WebString("Provided audioTrack is not supported. Ignoring track."), Removed "Ignoring track." because of return.
Some nits, lgtm after you address them. https://codereview.chromium.org/499233003/diff/240001/content/common/speech_r... File content/common/speech_recognition_messages.h (right): https://codereview.chromium.org/499233003/diff/240001/content/common/speech_r... content/common/speech_recognition_messages.h:68: // Wheter user has set an audio track as input nit, end with period https://codereview.chromium.org/499233003/diff/240001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_sink.cc (right): https://codereview.chromium.org/499233003/diff/240001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.cc:89: output_params_.sample_rate()); nit, indentation should look: std::ceil(output_params_.frames_per_buffer() * static_cast<double>(input_params_.sample_rate()) / output_params_.sample_rate()); https://codereview.chromium.org/499233003/diff/240001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.cc:104: new media::AudioConverter(input_params, output_params_, false)); AudioConverter has a better performance when the fifo is disabled there. Does it work if you change it to new media::AudioConverter(input_params, output_params_, true)); https://codereview.chromium.org/499233003/diff/240001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_sink.h (right): https://codereview.chromium.org/499233003/diff/240001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.h:40: // TODO(burnik): Add comments about owenrship of params/members. please address this TODO here https://codereview.chromium.org/499233003/diff/240001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.h:42: const media::AudioParameters& params, indentation. https://codereview.chromium.org/499233003/diff/240001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_sink_unittest.cc (right): https://codereview.chromium.org/499233003/diff/240001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink_unittest.cc:19: // Mocked out sockets used for Send/Receive. nit, put all these helper class into anonymous namespace https://codereview.chromium.org/499233003/diff/240001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink_unittest.cc:186: const int kOutputBitsPerSample = 16; move all these variable on top of MockSyncSocket, https://codereview.chromium.org/499233003/diff/240001/content/renderer/speech... File content/renderer/speech_recognition_dispatcher.cc (right): https://codereview.chromium.org/499233003/diff/240001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:290: void SpeechRecognitionDispatcher::ResetAudioSourceProvider() { There is no AudioSourceProvider any more, how about changing the name to OnAudioTrackStopped()
Nits addressed. Waiting for remaining owners to stamp. https://codereview.chromium.org/499233003/diff/240001/content/common/speech_r... File content/common/speech_recognition_messages.h (right): https://codereview.chromium.org/499233003/diff/240001/content/common/speech_r... content/common/speech_recognition_messages.h:68: // Wheter user has set an audio track as input On 2014/09/29 09:28:56, xians1 wrote: > nit, end with period Done. https://codereview.chromium.org/499233003/diff/240001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_sink.cc (right): https://codereview.chromium.org/499233003/diff/240001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.cc:89: output_params_.sample_rate()); On 2014/09/29 09:28:57, xians1 wrote: > nit, indentation should look: > std::ceil(output_params_.frames_per_buffer() * > static_cast<double>(input_params_.sample_rate()) / > output_params_.sample_rate()); Done. https://codereview.chromium.org/499233003/diff/240001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.cc:104: new media::AudioConverter(input_params, output_params_, false)); On 2014/09/29 09:28:56, xians1 wrote: > AudioConverter has a better performance when the fifo is disabled there. > Does it work if you change it to > new media::AudioConverter(input_params, output_params_, true)); No, it doesn't work. https://codereview.chromium.org/499233003/diff/240001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_sink.h (right): https://codereview.chromium.org/499233003/diff/240001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.h:40: // TODO(burnik): Add comments about owenrship of params/members. On 2014/09/29 09:28:57, xians1 wrote: > please address this TODO here Done. https://codereview.chromium.org/499233003/diff/240001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.h:42: const media::AudioParameters& params, On 2014/09/29 09:28:57, xians1 wrote: > indentation. Done. https://codereview.chromium.org/499233003/diff/240001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_sink_unittest.cc (right): https://codereview.chromium.org/499233003/diff/240001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink_unittest.cc:19: // Mocked out sockets used for Send/Receive. On 2014/09/29 09:28:57, xians1 wrote: > nit, put all these helper class into anonymous namespace Done. https://codereview.chromium.org/499233003/diff/240001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink_unittest.cc:186: const int kOutputBitsPerSample = 16; On 2014/09/29 09:28:57, xians1 wrote: > move all these variable on top of MockSyncSocket, Done.
Addressed remaining comment from previous patchset. https://codereview.chromium.org/499233003/diff/240001/content/renderer/speech... File content/renderer/speech_recognition_dispatcher.cc (right): https://codereview.chromium.org/499233003/diff/240001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:290: void SpeechRecognitionDispatcher::ResetAudioSourceProvider() { On 2014/09/29 09:28:57, xians1 wrote: > There is no AudioSourceProvider any more, how about changing the name to > OnAudioTrackStopped() This has been discussed earlier. It would be regarded as an IPC for the reader if named with "On" prefix. Renamed to "ResetAudioSink" - ready for next patchset. Also, this is a bug. Changed to: speech_audio_sink_.reset();
LGTM w/ nits ;-) https://codereview.chromium.org/499233003/diff/260001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_sink.cc (right): https://codereview.chromium.org/499233003/diff/260001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.cc:36: // Buffer index for sync with client is |params.size| on the shared memory. Odd language here as well. Not clear to me what you mean. Can you rewrite? .. is ... on the ... https://codereview.chromium.org/499233003/diff/260001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.cc:72: // Purposely only support tracks from an audio device. Dissallow WebAudio. Dissallow WebAudio? Does it mean that it is not supported or what? https://codereview.chromium.org/499233003/diff/260001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.cc:83: // We need detach the thread here because it will be a new capture thread nit, 'need to' or must detach perhaps. https://codereview.chromium.org/499233003/diff/260001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.cc:95: static const int kNumberOfBuffersInFifo = 2; How was 2 chosen. What would happen if it was 20 instead? https://codereview.chromium.org/499233003/diff/260001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_sink.h (right): https://codereview.chromium.org/499233003/diff/260001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.h:32: // WebSpeechRecognizer increments the shared buffer index to synchronize. Can you clarify? What is synchronized and how does it work? https://codereview.chromium.org/499233003/diff/260001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.h:48: /* Socket ownership is passed to here. */ 'to here' sounds odd. Can you rewrite?
Comments + bugfix
Updated CL description and comments. https://codereview.chromium.org/499233003/diff/260001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_sink.cc (right): https://codereview.chromium.org/499233003/diff/260001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.cc:36: // Buffer index for sync with client is |params.size| on the shared memory. On 2014/09/29 10:38:41, henrika wrote: > Odd language here as well. Not clear to me what you mean. Can you rewrite? .. is > ... on the ... Done. // Peer's buffer index is accessed via |params.size| in shared memory. https://codereview.chromium.org/499233003/diff/260001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.cc:42: // Client must manage his own counter and reset it. s/client/peer/g https://codereview.chromium.org/499233003/diff/260001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.cc:72: // Purposely only support tracks from an audio device. Dissallow WebAudio. On 2014/09/29 10:38:41, henrika wrote: > Dissallow WebAudio? Does it mean that it is not supported or what? No. Just dissallowed as documented (abuse mitigation). https://codereview.chromium.org/499233003/diff/260001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.cc:83: // We need detach the thread here because it will be a new capture thread On 2014/09/29 10:38:41, henrika wrote: > nit, 'need to' or must detach perhaps. Detach the thread here because it will be a new capture thread ... https://codereview.chromium.org/499233003/diff/260001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.cc:95: static const int kNumberOfBuffersInFifo = 2; On 2014/09/29 10:38:41, henrika wrote: > How was 2 chosen. What would happen if it was 20 instead? The constant 2 here is a minimum integer number of buffers which can allow delays. This in fact means that if we need to accumulate 100 ms of data before releasing, we can actually store 200ms at any moment. so this is a 100% overhead. If it were 20, we would use up more heap than we need - e.g. for desired 100 ms for output, we could accumulate 2s of audio input. That would amount to 1900% overhead. This amount of delay would indicate a serious issue with performance on the browser process and it wouldn't make sense to do anything other than stop gathering audio input since it isn't going anywhere. For such a large delay, the remote API would also disconnect the upstream connection, killing the SR session. This case is handled in the OnData callback. https://codereview.chromium.org/499233003/diff/260001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_sink.h (right): https://codereview.chromium.org/499233003/diff/260001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.h:32: // WebSpeechRecognizer increments the shared buffer index to synchronize. On 2014/09/29 10:38:41, henrika wrote: > Can you clarify? What is synchronized and how does it work? The buffer indices are synchronized. Detailed in design doc. http://goo.gl/9Ot3PC https://codereview.chromium.org/499233003/diff/260001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.h:48: /* Socket ownership is passed to here. */ On 2014/09/29 10:38:41, henrika wrote: > 'to here' sounds odd. Can you rewrite? Done: *Socket ownership is transferred.*
lgtm with nits https://codereview.chromium.org/499233003/diff/280001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_sink.h (right): https://codereview.chromium.org/499233003/diff/280001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.h:39: SpeechRecognitionAudioSink(/* ExtraData reference is copied from track. */ those comments are rather redundant https://codereview.chromium.org/499233003/diff/280001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_sink_unittest.cc (right): https://codereview.chromium.org/499233003/diff/280001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink_unittest.cc:66: virtual size_t Send(const void* buffer, size_t length) OVERRIDE; nit. please add a comment like // base::SyncSocket implementation before this line https://codereview.chromium.org/499233003/diff/280001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink_unittest.cc:78: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/499233003/diff/280001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink_unittest.cc:184: }; disallow copy/assign https://codereview.chromium.org/499233003/diff/280001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink_unittest.cc:192: SpeechRecognitionAudioSinkTest() { } should have a virtual dtor https://codereview.chromium.org/499233003/diff/280001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink_unittest.cc:328: scoped_ptr<SpeechRecognitionAudioSink> speech_audio_sink_; members should be private https://codereview.chromium.org/499233003/diff/280001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink_unittest.cc:338: }; disallow copy and assign
One open questions for tommi/xians/jochen before next patchset. https://codereview.chromium.org/499233003/diff/280001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_sink.h (right): https://codereview.chromium.org/499233003/diff/280001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.h:39: SpeechRecognitionAudioSink(/* ExtraData reference is copied from track. */ On 2014/09/30 08:23:58, jochen (slow for reviews) wrote: > those comments are rather redundant Tommi requested to document the ownership of the params. Since now using a scoped_ptr indicating the passing, maybe they really are redundant. tommi, xians, jochen: Do you think I should remove them? https://codereview.chromium.org/499233003/diff/280001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_sink_unittest.cc (right): https://codereview.chromium.org/499233003/diff/280001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink_unittest.cc:66: virtual size_t Send(const void* buffer, size_t length) OVERRIDE; On 2014/09/30 08:23:59, jochen (slow for reviews) wrote: > nit. please add a comment like // base::SyncSocket implementation before this > line Done. https://codereview.chromium.org/499233003/diff/280001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink_unittest.cc:78: }; On 2014/09/30 08:23:59, jochen (slow for reviews) wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/499233003/diff/280001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink_unittest.cc:184: }; On 2014/09/30 08:23:59, jochen (slow for reviews) wrote: > disallow copy/assign Done. https://codereview.chromium.org/499233003/diff/280001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink_unittest.cc:192: SpeechRecognitionAudioSinkTest() { } On 2014/09/30 08:23:58, jochen (slow for reviews) wrote: > should have a virtual dtor Done. https://codereview.chromium.org/499233003/diff/280001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink_unittest.cc:328: scoped_ptr<SpeechRecognitionAudioSink> speech_audio_sink_; On 2014/09/30 08:23:58, jochen (slow for reviews) wrote: > members should be private Done. https://codereview.chromium.org/499233003/diff/280001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink_unittest.cc:338: }; On 2014/09/30 08:23:58, jochen (slow for reviews) wrote: > disallow copy and assign Done.
Quick follow up on the previous question. https://codereview.chromium.org/499233003/diff/280001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_sink.h (right): https://codereview.chromium.org/499233003/diff/280001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.h:39: SpeechRecognitionAudioSink(/* ExtraData reference is copied from track. */ On 2014/09/30 09:57:46, burnik wrote: > On 2014/09/30 08:23:58, jochen (slow for reviews) wrote: > > those comments are rather redundant > > Tommi requested to document the ownership of the params. > Since now using a scoped_ptr indicating the passing, maybe they really are > redundant. > > tommi, xians, jochen: > Do you think I should remove them? Added comment at top of ctor and removed all /* ... */: // Socket ownership is transferred to the class via constructor.
I think that the parameter types make the ownership clear enough.
agree with jochen re ownership documentation. The cl looks good but I'd like to chat a little bit about the pointer below (that I think is redundant but iirc you had some other reasons for it) https://codereview.chromium.org/499233003/diff/300001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_sink.cc (right): https://codereview.chromium.org/499233003/diff/300001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.cc:40: peer_buffer_index_ = &(buffer->params.size); instead of peer_buffer_index_ I'd rather want to use a pointer to the shared memory. You actually already have a pointer to that via shared_memory_, so instead of having multiple pointers to the same thing, what about having a private method like this: media::AudioInputBuffer* audio_input_buffer() { <dcheck correct calling thread> <dcheck shared_memory_ is valid> return static_cast<media::AudioInputBuffer*>(shared_memory_.memory()); } and then wherever you currently use peer_buffer_index_, use |audio_input_buffer()->params.size| instead.
How about it? :) https://codereview.chromium.org/499233003/diff/300001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_sink.cc (right): https://codereview.chromium.org/499233003/diff/300001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.cc:40: peer_buffer_index_ = &(buffer->params.size); On 2014/10/06 20:30:33, tommi wrote: > instead of peer_buffer_index_ I'd rather want to use a pointer to the shared > memory. > > You actually already have a pointer to that via shared_memory_, so instead of > having multiple pointers to the same thing, what about having a private method > like this: > > media::AudioInputBuffer* audio_input_buffer() { > <dcheck correct calling thread> > <dcheck shared_memory_ is valid> > return static_cast<media::AudioInputBuffer*>(shared_memory_.memory()); > } > > and then wherever you currently use peer_buffer_index_, use > |audio_input_buffer()->params.size| instead. What about: http://crrev.com/526113002 struct AudioSharedBuffer { uint32 buffer_index; int8 audio[1]; }; media::AudioSharedBuffer* audio_shared_buffer() { <dcheck correct calling thread> <dcheck shared_memory_ is valid> return static_cast<media::AudioSharedBuffer*>(shared_memory_.memory()); } and then use: |audio_shared_buffer()->buffer_index| ? Sounds much cleaner than params.size. Also usable in the browser CL and I believe in a few other places.
https://codereview.chromium.org/499233003/diff/300001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_sink.cc (right): https://codereview.chromium.org/499233003/diff/300001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.cc:40: peer_buffer_index_ = &(buffer->params.size); On 2014/10/07 09:01:09, burnik wrote: > On 2014/10/06 20:30:33, tommi wrote: > > instead of peer_buffer_index_ I'd rather want to use a pointer to the shared > > memory. > > > > You actually already have a pointer to that via shared_memory_, so instead of > > having multiple pointers to the same thing, what about having a private method > > like this: > > > > media::AudioInputBuffer* audio_input_buffer() { > > <dcheck correct calling thread> > > <dcheck shared_memory_ is valid> > > return static_cast<media::AudioInputBuffer*>(shared_memory_.memory()); > > } > > > > and then wherever you currently use peer_buffer_index_, use > > |audio_input_buffer()->params.size| instead. > > What about: > > http://crrev.com/526113002 > > struct AudioSharedBuffer { > uint32 buffer_index; > int8 audio[1]; > }; > > media::AudioSharedBuffer* audio_shared_buffer() { > <dcheck correct calling thread> > <dcheck shared_memory_ is valid> > return static_cast<media::AudioSharedBuffer*>(shared_memory_.memory()); > } > > and then use: > |audio_shared_buffer()->buffer_index| > > ? > > Sounds much cleaner than params.size. Also usable in the browser CL and I > believe in a few other places. > I don't think you should make this CL more complicated given that the reviewers on that thread are not convinced by the approach. I will suggest you do what Tommi proposed.
Removed the peer_buffer_index_ , using audio_input_buffer()->params.size. Also a bit of refactoring. https://codereview.chromium.org/499233003/diff/300001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_sink.cc (right): https://codereview.chromium.org/499233003/diff/300001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.cc:40: peer_buffer_index_ = &(buffer->params.size); On 2014/10/07 09:46:18, xians1 wrote: > On 2014/10/07 09:01:09, burnik wrote: > > On 2014/10/06 20:30:33, tommi wrote: > > > instead of peer_buffer_index_ I'd rather want to use a pointer to the shared > > > memory. > > > > > > You actually already have a pointer to that via shared_memory_, so instead > of > > > having multiple pointers to the same thing, what about having a private > method > > > like this: > > > > > > media::AudioInputBuffer* audio_input_buffer() { > > > <dcheck correct calling thread> > > > <dcheck shared_memory_ is valid> > > > return static_cast<media::AudioInputBuffer*>(shared_memory_.memory()); > > > } > > > > > > and then wherever you currently use peer_buffer_index_, use > > > |audio_input_buffer()->params.size| instead. > > > > What about: > > > > http://crrev.com/526113002 > > > > struct AudioSharedBuffer { > > uint32 buffer_index; > > int8 audio[1]; > > }; > > > > media::AudioSharedBuffer* audio_shared_buffer() { > > <dcheck correct calling thread> > > <dcheck shared_memory_ is valid> > > return static_cast<media::AudioSharedBuffer*>(shared_memory_.memory()); > > } > > > > and then use: > > |audio_shared_buffer()->buffer_index| > > > > ? > > > > Sounds much cleaner than params.size. Also usable in the browser CL and I > > believe in a few other places. > > > > I don't think you should make this CL more complicated given that the reviewers > on that thread are not convinced by the approach. > > I will suggest you do what Tommi proposed. Done.
https://codereview.chromium.org/499233003/diff/320001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_sink.h (right): https://codereview.chromium.org/499233003/diff/320001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.h:65: media::AudioInputBuffer* audio_input_buffer() const; nit, I am afraid hacker_style is not appropriate for this method since it is not one line of code in its implementation. Probably GetAudioInputBuffer() is better? Tommi, what do you think?
https://codereview.chromium.org/499233003/diff/320001/content/renderer/media/... File content/renderer/media/speech_recognition_audio_sink.h (right): https://codereview.chromium.org/499233003/diff/320001/content/renderer/media/... content/renderer/media/speech_recognition_audio_sink.h:65: media::AudioInputBuffer* audio_input_buffer() const; On 2014/10/07 15:21:12, xians1 wrote: > nit, I am afraid hacker_style is not appropriate for this method since it is not > one line of code in its implementation. > > Probably GetAudioInputBuffer() is better? > > Tommi, what do you think? Done.
lgtm
thanks! lgtm
Add ENABLE_WEBRTC flag checks
https://codereview.chromium.org/499233003/diff/360001/content/renderer/speech... File content/renderer/speech_recognition_dispatcher.h (right): https://codereview.chromium.org/499233003/diff/360001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.h:52: blink::WebSpeechRecognizerClient*) override; not lgtm, you should never use override for cross-repository implementations and the blink:: interface is in the Blink repository. if you do, changing the interface becomes nearly impossible since you can't atomically update the interface and implementations in the same patch.
Remove override for blink impl
Good point, can't say why anyone had put it there earlier... https://codereview.chromium.org/499233003/diff/360001/content/renderer/speech... File content/renderer/speech_recognition_dispatcher.h (right): https://codereview.chromium.org/499233003/diff/360001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.h:52: blink::WebSpeechRecognizerClient*) override; On 2014/10/08 19:14:11, jamesr wrote: > not lgtm, you should never use override for cross-repository implementations and > the blink:: interface is in the Blink repository. if you do, changing the > interface becomes nearly impossible since you can't atomically update the > interface and implementations in the same patch. Done.
Good point, can't say why anyone had put it there earlier...
Rebase on master - merge fix
https://codereview.chromium.org/499233003/diff/400001/content/content_rendere... File content/content_renderer.gypi (right): https://codereview.chromium.org/499233003/diff/400001/content/content_rendere... content/content_renderer.gypi:629: 'renderer/media/speech_recognition_audio_sink.cc', have you excluded these classed when enable_webrtc == 0 ? https://codereview.chromium.org/499233003/diff/400001/content/renderer/speech... File content/renderer/speech_recognition_dispatcher.cc (right): https://codereview.chromium.org/499233003/diff/400001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:58: IPC_MESSAGE_HANDLER(SpeechRecognitionMsg_SharedAudioBusReady, Is SharedAudioBusReady a suitable name here? I think this msg is triggered by the speech recognizer after it sets up a audio stream to receive data from the track. I will suggest a name like AudioStreamReady or AudioReceiverReady https://codereview.chromium.org/499233003/diff/400001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:109: // fall back to default input when the track is not allowed nit, s/fall/Fall/g, and end with period. https://codereview.chromium.org/499233003/diff/400001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:259: speech_audio_sink_.reset(); call ResetAudioSink() instead
s/OnSharedAudioBusReady/OnAudioReceiverReady/ + nits
Nits done. https://codereview.chromium.org/499233003/diff/400001/content/content_rendere... File content/content_renderer.gypi (right): https://codereview.chromium.org/499233003/diff/400001/content/content_rendere... content/content_renderer.gypi:629: 'renderer/media/speech_recognition_audio_sink.cc', On 2014/10/09 12:19:02, xians1 wrote: > have you excluded these classed when enable_webrtc == 0 ? Yes. https://codereview.chromium.org/499233003/diff/400001/content/renderer/speech... File content/renderer/speech_recognition_dispatcher.cc (right): https://codereview.chromium.org/499233003/diff/400001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:58: IPC_MESSAGE_HANDLER(SpeechRecognitionMsg_SharedAudioBusReady, On 2014/10/09 12:19:02, xians1 wrote: > Is SharedAudioBusReady a suitable name here? > I think this msg is triggered by the speech recognizer after it sets up a audio > stream to receive data from the track. > > I will suggest a name like > AudioStreamReady or AudioReceiverReady Done. https://codereview.chromium.org/499233003/diff/400001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:109: // fall back to default input when the track is not allowed On 2014/10/09 12:19:02, xians1 wrote: > nit, s/fall/Fall/g, and end with period. Done. https://codereview.chromium.org/499233003/diff/400001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:259: speech_audio_sink_.reset(); On 2014/10/09 12:19:02, xians1 wrote: > call ResetAudioSink() instead Done.
lgtm again.
content/renderer lgtm https://codereview.chromium.org/499233003/diff/420001/content/renderer/speech... File content/renderer/speech_recognition_dispatcher.cc (right): https://codereview.chromium.org/499233003/diff/420001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:10: #if defined(ENABLE_WEBRTC) put the #ifdef block below the rest of the includes. see http://www.chromium.org/developers/coding-style#TOC-Platform-specific-code https://codereview.chromium.org/499233003/diff/420001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:263: // Create socket here and pass ownership to the |speech_audio_sink_|. this comment describes "what" but it doesn't say "why". It's pretty clear what this code is doing from reading it, so this comment isn't useful, but describing why it's important to create the socket here might be useful
The CQ bit was checked by burnik@chromium.org
The CQ bit was unchecked by burnik@chromium.org
Nits done. Preland checks.
Good point on the comment. Thx. :) https://codereview.chromium.org/499233003/diff/420001/content/renderer/speech... File content/renderer/speech_recognition_dispatcher.cc (right): https://codereview.chromium.org/499233003/diff/420001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:10: #if defined(ENABLE_WEBRTC) On 2014/10/09 17:10:28, jamesr wrote: > put the #ifdef block below the rest of the includes. see > http://www.chromium.org/developers/coding-style#TOC-Platform-specific-code Done. https://codereview.chromium.org/499233003/diff/420001/content/renderer/speech... content/renderer/speech_recognition_dispatcher.cc:263: // Create socket here and pass ownership to the |speech_audio_sink_|. On 2014/10/09 17:10:28, jamesr wrote: > this comment describes "what" but it doesn't say "why". It's pretty clear what > this code is doing from reading it, so this comment isn't useful, but describing > why it's important to create the socket here might be useful Done.
The CQ bit was checked by burnik@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/499233003/480001
Message was sent while issue was closed.
Committed patchset #21 (id:480001)
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/2eeb46675144086a100ce6497f4b8ac5e56fcdb1 Cr-Commit-Position: refs/heads/master@{#298981}
Message was sent while issue was closed.
Shared memory initialization in unit test - bugfix
Remove suppression for unit test
burnik@chromium.org changed reviewers: - henrika@chromium.org, jamesr@chromium.org, jochen@chromium.org, kenrb@chromium.org, tommi@chromium.org
Please review the fix.
Patchset #23 (id:730001) has been deleted
Patchset #22 (id:670001) has been deleted |
