Closed
Bug 1051148
Opened 10 years ago
Closed 9 years ago
Implementation of Webspeech API & PocketSphinx Integration
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
2.2 S14 (12june)
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: anatal, Assigned: kdavis)
References
(Blocks 3 open bugs)
Details
(Whiteboard: [webspeechapi][systemsfe])
User Story
Implement Web Speech API in Firefox OS baseline according to W3C standards
Attachments
(2 files, 19 obsolete files)
33.18 KB,
patch
|
kdavis
:
review+
|
Details | Diff | Splinter Review |
1.60 KB,
patch
|
kdavis
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release) Build ID: 20140716183446
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Comment 1•10 years ago
|
||
This patch enables WebSpeechAPI with a PocketsphinxService for use with grammars. Few notes: 1 - Code is multiplatform and tests and builds was made in all platforms (Mac, Linux, B2G, Fennec) less Windows that needs a fix 2 - The patch includes all pocketsphinx sources and models with moz.build and packaging/installer configuration 3 - This patch does not auto set Web Speech API preferences that still should be changed by the user 4 - The decode is made in-memory and on a thread
Attachment #8470537 -
Flags: review?(bugs)
Comment 2•10 years ago
|
||
Comment on attachment 8470537 [details] [diff] [review] Patch to enable offline WebSpeech API with a PocketSphinxService You need to split the patch to reviewable parts.
Attachment #8470537 -
Flags: review?(bugs) → review-
Reporter | ||
Comment 3•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #2) > Comment on attachment 8470537 [details] [diff] [review] > Patch to enable offline WebSpeech API with a PocketSphinxService > > You need to split the patch to reviewable parts. ggp said me that I need to do only one commit. How do you want this to be split ? One patch per bug or only two: one for pocketsphinx sources and models, and another for PocketSphinxSpeechRecognitionService?
Flags: needinfo?(bugs)
Comment 4•10 years ago
|
||
I'm not going review all pocketsphinx source codes, only the code you have written. (pocketsphinx certainly needs security audit and some fuzz testing before we expose the functionality to the web.)
Flags: needinfo?(bugs)
Comment 5•10 years ago
|
||
Also, is the license of pocketsphinx such that we can distribute it with Gecko?
Comment 6•10 years ago
|
||
(I think it is, but better to ask gerv)
Reporter | ||
Comment 7•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #6) > (I think it is, but better to ask gerv) Yes
Reporter | ||
Comment 8•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #4) > I'm not going review all pocketsphinx source codes, only the code you have > written. > (pocketsphinx certainly needs security audit and some fuzz testing before we > expose the functionality > to the web.) Ok
Reporter | ||
Comment 9•10 years ago
|
||
(In reply to Andre Natal from comment #7) > (In reply to Olli Pettay [:smaug] from comment #6) > > (I think it is, but better to ask gerv) > > Yes @Sandip has the license and is addressing this.
Flags: needinfo?(skamat)
Comment 10•10 years ago
|
||
Reviewing it with Legal this week. (cc: Geoff and Gerv)
Flags: needinfo?(skamat)
Comment 11•10 years ago
|
||
There is no problem with the license of PocketSphinx. Please make sure a copy of the COPYING file makes it into whichever directory you import the code into. Gerv
Comment 12•10 years ago
|
||
(In reply to Gervase Markham [:gerv] from comment #11) > There is no problem with the license of PocketSphinx. Please make sure a > copy of the COPYING file makes it into whichever directory you import the > code into. > > Gerv Thanks Gerv. Andre - can you pls take care of this?
Flags: needinfo?(anatal)
Reporter | ||
Comment 13•10 years ago
|
||
(In reply to Sandip Kamat from comment #12) > (In reply to Gervase Markham [:gerv] from comment #11) > > There is no problem with the license of PocketSphinx. Please make sure a > > copy of the COPYING file makes it into whichever directory you import the > > code into. > > > > Gerv > > Thanks Gerv. Andre - can you pls take care of this? Yes, no problem.
Reporter | ||
Updated•10 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Comment 14•10 years ago
|
||
Brief benchmark on Flame > START LISTENING User 3%, System 4%, IOW 0%, IRQ 0% User 1 + Nice 20 + Sys 27 + Idle 523 + IOW 0 + IRQ 0 + SIRQ 0 = 571 PID TID PR CPU% S VSS RSS PCY UID Thread Proc 1597 1643 0 4% S 140868K 70452K root opensl_rec_thre /system/b2g/plugin-container 1597 1627 0 2% S 140868K 70452K root AudioGUM /system/b2g/plugin-container 1597 1640 0 2% S 140868K 70452K root MediaStreamGrph /system/b2g/plugin-container 1562 1562 1 2% R 1288K 620K root top top 96 96 1 1% S 0K 0K root ksmd 1597 1597 0 0% S 140868K 70452K root Browser /system/b2g/plugin-container 1597 1629 1 0% S 140868K 70452K root ProcessThread /system/b2g/plugin-container 1597 1642 1 0% S 140868K 70452K root AudioRecord /system/b2g/plugin-container 279 1641 0 0% S 21696K 6720K fg media AudioIn_A /system/bin/mediaserver 1574 1574 0 0% D 0K 0K root kworker/u:3 > EXACTLY POINT OF RECOGNITION User 15%, System 7%, IOW 0%, IRQ 0% User 30 + Nice 59 + Sys 44 + Idle 438 + IOW 4 + IRQ 0 + SIRQ 1 = 576 PID TID PR CPU% S VSS RSS PCY UID Thread Proc 1597 1597 1 6% S 140284K 70456K root Browser /system/b2g/plugin-container 1597 1643 1 4% S 140284K 70456K root opensl_rec_thre /system/b2g/plugin-container 287 287 0 3% S 188440K 83148K root b2g /system/b2g/b2g 1597 1627 0 2% S 140284K 70456K root AudioGUM /system/b2g/plugin-container 287 712 0 2% S 188440K 83148K root Compositor /system/b2g/b2g 1597 1640 1 1% S 140284K 70456K root MediaStreamGrph /system/b2g/plugin-container 1562 1562 0 1% R 1288K 620K root top top 96 96 1 1% S 0K 0K root ksmd 1597 1629 0 0% S 140284K 70456K root ProcessThread /system/b2g/plugin-container 287 1600 1 0% S 188440K 83148K root BufMgrParent#15 /system/b2g/b2g > STOP LISTENING User 10%, System 5%, IOW 0%, IRQ 0% User 6 + Nice 57 + Sys 30 + Idle 478 + IOW 1 + IRQ 0 + SIRQ 2 = 574 PID TID PR CPU% S VSS RSS PCY UID Thread Proc 1597 1627 0 5% S 140868K 70544K root AudioGUM /system/b2g/plugin-container 1597 1643 1 5% S 140868K 70544K root opensl_rec_thre /system/b2g/plugin-container 1597 1597 1 3% S 140868K 70544K root Browser /system/b2g/plugin-container 1597 1640 1 2% S 140868K 70544K root MediaStreamGrph /system/b2g/plugin-container 1562 1562 0 2% R 1288K 620K root top top 96 96 0 1% S 0K 0K root ksmd 287 712 1 0% S 189024K 83148K root Compositor /system/b2g/b2g 1597 1629 1 0% S 140868K 70544K root ProcessThread /system/b2g/plugin-container 1597 1642 1 0% S 140868K 70544K root AudioRecord /system/b2g/plugin-container 279 1641 1 0% S 21696K 6720K fg media AudioIn_A /system/bin/mediaserver
Reporter | ||
Comment 15•10 years ago
|
||
OK
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(anatal)
Reporter | ||
Comment 16•10 years ago
|
||
The patch was split in two and here is only the new code while bug 1051146 have the build system patch.
Attachment #8470537 -
Attachment is obsolete: true
Attachment #8474364 -
Flags: review?(bugs)
Comment 17•10 years ago
|
||
Comment on attachment 8474364 [details] [diff] [review] This patch creates a SpeechRecognitionService using PocketSphinx as a backend decoder to Web Speech API ggp, could you take the first round of reviews here. (random comments, remove printfs, new code should use gecko coding style https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style)
Attachment #8474364 -
Flags: review?(bugs) → review?(ggoncalves)
Comment 19•10 years ago
|
||
Comment on attachment 8474364 [details] [diff] [review] This patch creates a SpeechRecognitionService using PocketSphinx as a backend decoder to Web Speech API Andre and I agreed today on splitting the patches for ease of reviewing -- basically one patch for the scaffolding changes that had to be made in the current code, and another patch adding the service itself. Let's cancel the review for now and have another look when the patches are split, but in the meantime, Andre, please make sure you address :smaug's comments about coding style and removing the printf's.
Attachment #8474364 -
Flags: review?(ggoncalves)
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → anatal
Reporter | ||
Comment 20•10 years ago
|
||
This patch covers only the include of PocketSphinxSpeechRecognitionService.
Attachment #8474364 -
Attachment is obsolete: true
Attachment #8481643 -
Flags: review?(ggoncalves)
Reporter | ||
Comment 21•10 years ago
|
||
Attachment #8481643 -
Attachment is obsolete: true
Attachment #8481643 -
Flags: review?(ggoncalves)
Attachment #8481705 -
Flags: review?(ggoncalves)
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Comment 22•10 years ago
|
||
Attachment #8481705 -
Attachment is obsolete: true
Attachment #8481705 -
Flags: review?(ggoncalves)
Attachment #8489735 -
Flags: review?(ggoncalves)
Reporter | ||
Updated•10 years ago
|
Summary: Create SpeechRecognitionService for Pocketsphinx and integrate with the decoder to enable offline WebSpeechAPI → Development of PocketSphinxSpeechRecognitionService to use pocketsphinx and enable Webspeech API
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Comment 23•10 years ago
|
||
Attachment #8489735 -
Attachment is obsolete: true
Attachment #8489735 -
Flags: review?(ggoncalves)
Attachment #8489750 -
Flags: review?(ggoncalves)
Reporter | ||
Comment 24•10 years ago
|
||
Attachment #8489750 -
Attachment is obsolete: true
Attachment #8489750 -
Flags: review?(ggoncalves)
Attachment #8493507 -
Flags: review?(ggoncalves)
Reporter | ||
Comment 25•10 years ago
|
||
Attachment #8493507 -
Attachment is obsolete: true
Attachment #8493507 -
Flags: review?(ggoncalves)
Attachment #8496617 -
Flags: review?(ggoncalves)
Comment 26•10 years ago
|
||
If you ggp are busy with other stuff, feel free to move this to my review queue. (though, can't promise very fast review.)
Comment 27•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #26) > If you ggp are busy with other stuff, feel free to move this to my review > queue. > (though, can't promise very fast review.) I'll have time to do a first pass tomorrow. As with the other patches, I'll make sure all the obvious issues and style nits are fixed before moving it to you :)
Comment 28•10 years ago
|
||
Comment on attachment 8496617 [details] [diff] [review] Development of PocketSphinxSpeechRecognitionService to use pocketsphinx and enable Webspeech API Review of attachment 8496617 [details] [diff] [review]: ----------------------------------------------------------------- I've only taken a cursory look at PocketSphinxSpeechRecognitionService.{h,cpp} to find style issues, but the other changes look good. The one worrying thing I've found is the usage of mGram in SpeechGrammarList to compensate for the fact that we haven't implemented SpeechGrammarList and SpeechGrammar yet. As much as I hate do delay this patch, I think we should file a separate bug and implement these first. It shouldn't be too hard, as a SpeechGrammarList basically wraps an array of SpeechGrammar objects. Andre, would you be available for working on SpeechGrammar/SpeechGrammarList? ::: content/media/webspeech/recognition/PocketSphinxSpeechRecognitionService.cpp @@ +94,5 @@ > + rv = ps_start_utt(mPs, "goforward"); > + rv = ps_process_raw(mPs, &mAudiovector[0], mAudiovector.size(), FALSE, FALSE); > + > + rv = ps_end_utt(mPs); > + if (rv >= 0) if (rv >= 0) {, here and in the rest of the file. @@ +132,5 @@ > + nsString aStringDictPath; // dict folder > + > + NS_GetSpecialDirectory(NS_GRE_DIR, getter_AddRefs(tmpFile)); > + #if defined(XP_WIN) // for some reason, on windows NS_GRE_DIR is not bin root, but bin/browser > + tmpFile->AppendRelativePath(NS_LITERAL_STRING("..") ); Extra whitespace at the end of the line. Please watch out for other occurrences of this in the same method. ::: content/media/webspeech/recognition/PocketSphinxSpeechRecognitionService.h @@ +45,5 @@ > + cmd_ln_t *config; > + nsCOMPtr<nsIThread> decoderthread; > + bool decodersane; // flag to verify if decoder is sane > + bool grammarsane; // flag to verify if grammar is sane > + std::vector<int16_t> audiovector; Please fix these declarations according to our coding style. For example, ps_decoder_t* mPSHandle; nsCOMPtr<nsIThread> mDecoderThread; and so on. Also, what does it mean for a decoder or a grammar to be sane? Please rename the member variables if you can make the meaning more explicit, or explain in a comment. ::: content/media/webspeech/recognition/SpeechGrammarList.cpp @@ +89,4 @@ > const Optional<float>& aWeight, > ErrorResult& aRv) > { > + mGram = ToNewUTF8String(aString); Hmm, I feel like we should finish the implementation of SpeechGrammar and SpeechGrammarList instead of resorting to stuff like this. If we had them properly implemented, then inside ValidateAndSetGrammarList you could just call SpeechGrammarList::Item to get a SpeechGrammar, then call SpeechGrammar::GetSrc to get the source as a string. ::: content/media/webspeech/recognition/SpeechGrammarList.h @@ +52,4 @@ > > nsCOMPtr<nsISpeechRecognitionService> mRecognitionService; > > + const char * mGram; I guess you don't need this; please see my comment on SpeechGrammarList.cpp. Please remember to also remove the blank line you added. ::: content/media/webspeech/recognition/SpeechRecognition.cpp @@ +21,5 @@ > #include "nsIObserverService.h" > #include "nsServiceManagerUtils.h" > > +#include "nsWeakPtr.h" > +#include "nsIWeakReferenceUtils.h" I guess you don't need this, correct? Please remember to also remove the blank line you added. ::: content/media/webspeech/recognition/moz.build @@ -19,4 @@ > 'SpeechRecognitionResult.h', > 'SpeechRecognitionResultList.h', > 'SpeechStreamListener.h', > - 'test/FakeSpeechRecognitionService.h', No need to remove this. @@ -33,4 @@ > 'SpeechRecognitionResult.cpp', > 'SpeechRecognitionResultList.cpp', > 'SpeechStreamListener.cpp', > - 'test/FakeSpeechRecognitionService.cpp', No need to remove this either. @@ +38,5 @@ > > LOCAL_INCLUDES += [ > '/dom/base', > + '/media/pocketsphinx', > + '/media/sphinxbase', Please remove the extra whitespace at the end of the line. ::: layout/build/nsLayoutModule.cpp @@ +96,4 @@ > #include "mozilla/Services.h" > > #ifdef MOZ_WEBSPEECH > +#include "mozilla/dom/PocketSphinxSpeechRecognitionService.h" Please don't replace the lines referring to FakeSpeechRecognitionService, but rather add yours after them. This goes for all changes in this file.
Attachment #8496617 -
Flags: review?(ggoncalves)
Reporter | ||
Comment 29•10 years ago
|
||
Comment on attachment 8496617 [details] [diff] [review] Development of PocketSphinxSpeechRecognitionService to use pocketsphinx and enable Webspeech API Review of attachment 8496617 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webspeech/recognition/SpeechGrammarList.cpp @@ +89,4 @@ > const Optional<float>& aWeight, > ErrorResult& aRv) > { > + mGram = ToNewUTF8String(aString); Makes sense, but well, actually w3c specification it is not well suited for grammar recogntion. In this case for example, SpeechGrammar's src attribute does not expect the grammar string, but an URI that points to that grammar. So you confirm that it is OK to fill this attribute with the grammar string instead the URI? https://dvcs.w3.org/hg/speech-api/raw-file/tip/speechapi.html#speechreco-speechgrammar src attribute The required src attribute is the URI for the grammar. Note some services may support builtin grammars that can be specified using a builtin URI scheme.
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(ggoncalves)
Comment 30•10 years ago
|
||
(In reply to Andre Natal from comment #29) > Note some services may support builtin grammars that can be specified using a builtin > URI scheme. I think this snippet answers your question, actually. We should be free to define builtin URI schemes referrring to the PocketSphinx grammars we support. Note that implementing SpeechGrammar/SpeechGrammarList doesn't really depend on specifying such schemes -- a SpeechGrammar will just hold an arbitrary string as its src, and it's up to the recognition service to interpret that string. So once we have these objects implemented, we can bikeshed on this bug about what URI scheme we can use to expose the grammars we'll ship (and we probably want to look at what other implementations are doing in this regard, too).
Flags: needinfo?(ggoncalves)
Reporter | ||
Comment 31•10 years ago
|
||
(In reply to Guilherme Gonçalves [:ggp] from comment #30) > (In reply to Andre Natal from comment #29) > > Note some services may support builtin grammars that can be specified using a builtin > > URI scheme. > > I think this snippet answers your question, actually. We should be free to > define builtin URI schemes referrring to the PocketSphinx grammars we > support. > > Note that implementing SpeechGrammar/SpeechGrammarList doesn't really depend > on specifying such schemes -- a SpeechGrammar will just hold an arbitrary > string as its src, and it's up to the recognition service to interpret that > string. So once we have these objects implemented, we can bikeshed on this > bug about what URI scheme we can use to expose the grammars we'll ship (and > we probably want to look at what other implementations are doing in this > regard, too). Thanks. I am working on it.
Reporter | ||
Comment 32•10 years ago
|
||
Attachment #8496617 -
Attachment is obsolete: true
Attachment #8510685 -
Flags: review?(ggoncalves)
Attachment #8510685 -
Flags: review?(bugs)
Reporter | ||
Comment 33•10 years ago
|
||
(In reply to Guilherme Gonçalves [:ggp] from comment #30) > (In reply to Andre Natal from comment #29) > > Note some services may support builtin grammars that can be specified using a builtin > > URI scheme. > > I think this snippet answers your question, actually. We should be free to > define builtin URI schemes referrring to the PocketSphinx grammars we > support. > > Note that implementing SpeechGrammar/SpeechGrammarList doesn't really depend > on specifying such schemes -- a SpeechGrammar will just hold an arbitrary > string as its src, and it's up to the recognition service to interpret that > string. So once we have these objects implemented, we can bikeshed on this > bug about what URI scheme we can use to expose the grammars we'll ship (and > we probably want to look at what other implementations are doing in this > regard, too). I did the fixed that you asked for and created another bug about SpeechGrammar/SpeechGrammarList approach and also submit the patch
Comment 34•10 years ago
|
||
Comment on attachment 8510685 [details] [diff] [review] Development of PocketSphinxSpeechRecognitionService to use pocketsphinx and enable Webspeech API As I said on IRC, we shouldn't enable pocketsphinx just if MOZ_WEBSPEECH is set. We need something similar what pico does for tts. And perhaps put PocketSphinx related files to content/media/webspeech/recognition/pocketsphinx/ >+ DecodeResultTask( const nsString& hypstring , WeakPtr<dom::SpeechRecognition> recognition) aHypstring, aRecognition... Please use aFoo naming for arguments, same issue also elsewhere in the patch >+ NS_IMETHOD Run() { { should be in the next line >+public: >+ DecodeTask(WeakPtr<dom::SpeechRecognition> recogntion , std::vector<int16_t> audiovector , ps_decoder_t * ps) extra space before * >+ NS_IMETHOD Run() { { should be on its own line >+ >+ >+ char const *hyp, *uttid; * goes with the type, char const* hyp; char const* uttid; (But do you really want char const* or const char* ?) >+ int rv; >+ int32 score; >+ nsString hypoValue; >+ >+ rv = ps_start_utt(mPs, "goforward"); >+ rv = ps_process_raw(mPs, &mAudiovector[0], mAudiovector.size(), FALSE, FALSE); >+ >+ rv = ps_end_utt(mPs); >+ if (rv >= 0) { I don't understand rv value usage here. You assign to rv several times, but check the value only once. >+private: >+ WeakPtr<dom::SpeechRecognition> mRecognition; >+ ps_decoder_t * mPs; Extra space before*. mPs needs also comment about what keeps the object alive. >+ std::vector<int16_t> mAudiovector; Any particular reason to use std::vector here? Why not nsTArray<int16_t> and call SetCapacity before the mAudioVector.push_back calls >+ PocketSphinxSpeechRecognitionService::PocketSphinxSpeechRecognitionService() >+ { >+ mSpeexState = nullptr; >+ >+ // get root folder >+ nsCOMPtr<nsIFile> tmpFile; >+ nsString aStringAMPath; // am folder >+ nsString aStringDictPath; // dict folder nsAutoString for local string variables. >+ NS_GetSpecialDirectory(NS_GRE_DIR, getter_AddRefs(tmpFile)); >+ #if defined(XP_WIN) // for some reason, on windows NS_GRE_DIR is not bin root, but bin/browser >+ tmpFile->AppendRelativePath(NS_LITERAL_STRING("..") ); >+ #endif >+ tmpFile->AppendRelativePath(NS_LITERAL_STRING("models") ); >+ tmpFile->AppendRelativePath(NS_LITERAL_STRING("en-us-semi") ); >+ tmpFile->GetPath(aStringAMPath); >+ >+ NS_GetSpecialDirectory(NS_GRE_DIR, getter_AddRefs(tmpFile)); >+ #if defined(XP_WIN) // for some reason, on windows NS_GRE_DIR is not bin root, but bin/browser >+ tmpFile->AppendRelativePath(NS_LITERAL_STRING("..") ); Does something else in the platform need to use this kind of hack? >+ if (mPSConfig == NULL) { nullptr >+ PocketSphinxSpeechRecognitionService::~PocketSphinxSpeechRecognitionService() >+ { >+ if (mPSConfig) >+ free(mPSConfig); Always {} with if if (mPSConfig) { free(mPSConfig); } >+ PocketSphinxSpeechRecognitionService::ProcessAudioSegment(AudioSegment* aAudioSegment, int32_t aSampleRate) >+ { >+ if (!mSpeexState) { >+ mSpeexState = speex_resampler_init(1, aSampleRate, 16000, SPEEX_RESAMPLER_QUALITY_MAX , nullptr); 2 spaces for indentation. Same issue also elsewhere. And you have some odd extra spaces between the parameters >+ } >+ else { } else {
Attachment #8510685 -
Flags: review?(bugs) → review-
Updated•10 years ago
|
Attachment #8510685 -
Flags: review?(ggoncalves)
Assignee | ||
Comment 35•9 years ago
|
||
Part 1 of 2: Patch that introduces WebSpeech API implementation This is the part 1 of 2 for this bug. This patch introduces the Pocketsphinx/Sphinxbase implementation of the WebSpeech API. This implementation depends upon all of the patches for Bug 1051146. The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=d932008c76cd
Attachment #8606749 -
Flags: review?(bugs)
Assignee | ||
Comment 36•9 years ago
|
||
Part 2 of 2: Patch that turns off the WebSpeech API implementation using flags This is the part 2 of 2 for this bug. This patch turns off the WebSpeech API implementation using flags. This implementation depends upon all of the patches for Bug 1051146. The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=cae7e9ed4c66
Attachment #8510685 -
Attachment is obsolete: true
Attachment #8606767 -
Flags: review?(bugs)
Assignee | ||
Comment 37•9 years ago
|
||
The patches above will have to be revised when the flags suggested in Bug 1051146, comment 50 are added.
Assignee | ||
Comment 38•9 years ago
|
||
Part 2 of 2: Patch that turns off the WebSpeech API implementation using flags This is the part 2 of 2 for this bug. This patch turns off the WebSpeech API implementation using flags. This patch fixes a typo in the previous patch. Also, this implementation depends upon all of the patches for Bug 1051146. The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffea7548ae17
Attachment #8606767 -
Attachment is obsolete: true
Attachment #8606767 -
Flags: review?(bugs)
Attachment #8606916 -
Flags: review?(bugs)
Assignee | ||
Comment 39•9 years ago
|
||
Part 2 of 2: Patch that turns off the WebSpeech API implementation using various flags This is the part 2 of 2 for this bug. This patch turns off the WebSpeech API implementation using various flags. In addition this patch fixes some tests that did not respect the MOZ_WEBSPEECH flag. Also, this patch depends upon all of the patches for Bug 1051146. The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=6186a5fa2359
Attachment #8606916 -
Attachment is obsolete: true
Attachment #8606916 -
Flags: review?(bugs)
Attachment #8607113 -
Flags: review?(bugs)
Comment 40•9 years ago
|
||
Comment on attachment 8606749 [details] [diff] [review] Part 1 of 2: Patch that introduces WebSpeech API implementation So this has lots of unrelated changes to Synthesis, and that all partially makes the patch size massive. Please include only the part which implements the recognition service. (feel free to file a separate bug to fix whatever issues you see in Synthesis code) Btw, since this all makes recognizer run on child process (I like that approach, safer), do we need to worry about sandboxing? *sphinx tries to open files, and if process is sandboxed, can it?
Attachment #8606749 -
Flags: review?(bugs) → review-
Comment 41•9 years ago
|
||
Comment on attachment 8607113 [details] [diff] [review] Part 2 of 2: Patch that turns off the WebSpeech API implementation using various flags I think we should enable the media.webspeech.recognition.enable/media.webspeech.service.default only on nightlies first. there is NIGHTLY_BUILD flag for it. That way we don't accidentally end up exposing still in-development code. test_interfaces.html has become more strict and tries to test what we actually ship, so trying to live without pref() checks. Wouldn't nightly: true, b2g: true work?
Attachment #8607113 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 42•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #40) > Comment on attachment 8606749 [details] [diff] [review] > Part 1 of 2: Patch that introduces WebSpeech API implementation > > So this has lots of unrelated changes to Synthesis, and that all partially > makes > the patch size massive. > Please include only the part which implements the recognition service. > (feel free to file a separate bug to fix whatever issues you see in > Synthesis code) OK. I will not submit my fixes for the Synthesis code. I have filed bugs against most of the issues I have fixed in the Synthesis code, but will not submit. Here are the associated bugs: * Bug 1167538 * Bug 1167539 * Bug 1167540 * Bug 1167541 * Bug 1167542 * Bug 1167543
Assignee | ||
Comment 43•9 years ago
|
||
Part 1 of 2: Patch that introduces WebSpeech API implementation This is the part 1 of 2 for this bug. This patch introduces the Pocketsphinx/Sphinxbase implementation of the WebSpeech API. This implementation depends upon all of the patches for Bug 1051146. The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=880674e0a840
Attachment #8606749 -
Attachment is obsolete: true
Attachment #8607113 -
Attachment is obsolete: true
Attachment #8610065 -
Flags: review?(bugs)
Assignee | ||
Comment 44•9 years ago
|
||
Part 2 of 2: Patch that turns off the WebSpeech API implementation using flags This is the part 2 of 2 for this bug. This patch turns off the WebSpeech API implementation using flags. This implementation depends upon all of the patches for Bug 1051146. The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=37f1f8a35dfe
Attachment #8610066 -
Flags: review?(bugs)
Comment 45•9 years ago
|
||
Comment on attachment 8610066 [details] [diff] [review] Part 2 of 2: Patch that turns off the WebSpeech API implementation using flags ah, I guess we can do it this way. One thing to check is that whether lots of code makes bug 1073399 kind of cases worse.
Attachment #8610066 -
Flags: review?(bugs) → review+
Comment 46•9 years ago
|
||
Comment on attachment 8610065 [details] [diff] [review] Part 1 of 2: Patch that introduces WebSpeech API implementation >+ mPSConfig = cmd_ln_init(NULL, ps_args(), TRUE, "-hmm", >+ ToNewUTF8String(aStringAMPath), // acoustic model >+ "-dict", ToNewUTF8String(aStringDictPath), NULL); >+ if (mPSConfig == NULL) { nullptr, not NULL. Also elsewhere. >+PocketSphinxSpeechRecognitionService::~PocketSphinxSpeechRecognitionService() >+{ >+ if (mPSConfig) >+ free(mPSConfig); >+ if (mPSHandle) >+ free(mPSHandle); Always {} with if. Also elsewhere >+ >+ NS_ConvertUTF16toUTF8 utf8Str(grammar); >+ const nsACString& utf8AStr = utf8Str; >+ int result = ps_set_jsgf_string(mPSHandle, "name", >+ PromiseFlatCString(utf8AStr).get()); You could just do NS_ConvertUTF16toUTF8(grammar).get() >+NS_IMETHODIMP >+PocketSphinxSpeechRecognitionService::Observe(nsISupports* aSubject, >+ const char* aTopic, >+ const char16_t* aData) >+{ >+ MOZ_ASSERT(mRecognition->mTestConfig.mFakeRecognitionService, >+ "Got request to fake recognition service event, " >+ "but " TEST_PREFERENCE_FAKE_RECOGNITION_SERVICE " is not set"); >+ >+ if (!strcmp(aTopic, SPEECH_RECOGNITION_TEST_END_TOPIC)) { >+ nsCOMPtr<nsIObserverService> obs = services::GetObserverService(); >+ obs->RemoveObserver(this, SPEECH_RECOGNITION_TEST_EVENT_REQUEST_TOPIC); >+ obs->RemoveObserver(this, SPEECH_RECOGNITION_TEST_END_TOPIC); >+ >+ return NS_OK; >+ } >+ >+ const nsDependentString eventName = nsDependentString(aData); >+ >+ if (eventName.EqualsLiteral("EVENT_RECOGNITIONSERVICE_ERROR")) { >+ mRecognition->DispatchError( >+ SpeechRecognition::EVENT_RECOGNITIONSERVICE_ERROR, >+ SpeechRecognitionErrorCode::Network, // TODO different codes? >+ NS_LITERAL_STRING("RECOGNITIONSERVICE_ERROR test event")); >+ >+ } else if (eventName.EqualsLiteral("EVENT_RECOGNITIONSERVICE_FINAL_RESULT")) { >+ nsRefPtr<SpeechEvent> event = new SpeechEvent( >+ mRecognition, SpeechRecognition::EVENT_RECOGNITIONSERVICE_FINAL_RESULT); >+ >+ event->mRecognitionResultList = BuildMockResultList(); >+ NS_DispatchToMainThread(event); >+ } >+ >+ return NS_OK; >+} >+ >+SpeechRecognitionResultList* >+PocketSphinxSpeechRecognitionService::BuildMockResultList() >+{ >+ SpeechRecognitionResultList* resultList = >+ new SpeechRecognitionResultList(mRecognition); >+ SpeechRecognitionResult* result = new SpeechRecognitionResult(mRecognition); >+ SpeechRecognitionAlternative* alternative = >+ new SpeechRecognitionAlternative(mRecognition); >+ >+ alternative->mTranscript = NS_LITERAL_STRING("Mock final result"); >+ alternative->mConfidence = 0.0f; >+ >+ result->mItems.AppendElement(alternative); >+ resultList->mItems.AppendElement(result); >+ >+ return resultList; >+} Please use 4 spaces for indentation. But I'm now here lost with the "mock" setup. Why BuildMockResultList? >+ /** >+ * Builds a mock SpeechRecognitionResultList >+ */ >+ dom::SpeechRecognitionResultList *BuildMockResultList(); * goes with the type >+ >+ /** Speex state */ >+ SpeexResamplerState *mSpeexState; ditto. SpeexResamplerState* mSpeexState; >+ >+ /** Pocksphix decoder */ >+ ps_decoder_t *mPSHandle; ditto >+ >+ /** Sphinxbase parsed command line arguments */ >+ cmd_ln_t *mPSConfig; ditto >+ /** Flag to verify if decoder was created */ >+ bool ISDecoderCreated; >+ >+ /** Flag to verify if grammar was compiled */ >+ bool ISGrammarCompiled; member variables should be name mFoo >+ >+ /** Audio data */ >+ std::vector<int16_t> mAudioVector; I'd prefer using nsTArray, of if we know some expected size for the array, perhaps even nsAutoTArray (that might reduce memory churn quite a bit). >+++ b/dom/media/webspeech/recognition/SpeechGrammar.h >@@ -49,6 +49,8 @@ private: > ~SpeechGrammar(); > > nsCOMPtr<nsISupports> mParent; >+ >+ nsString mGram; Could you call this mSrc; >+++ b/dom/media/webspeech/recognition/SpeechGrammarList.h >@@ -56,6 +56,8 @@ private: > ~SpeechGrammarList(); > > nsCOMPtr<nsISupports> mParent; >+ >+ nsTArray<nsRefPtr<SpeechGrammar> > mItems; drop space between > and >. These days all the compilers support >>, in the old days > > had to be used. >@@ -535,7 +535,9 @@ SpeechRecognition::StartRecording(DOMMediaStream* aDOMStream) > // doesn't get Destroy()'ed > mDOMStream = aDOMStream; > >- NS_ENSURE_STATE(mDOMStream->GetStream()); >+ if (NS_WARN_IF(!mDOMStream->GetStream())) { >+ return NS_ERROR_UNEXPECTED; >+ } > mSpeechListener = new SpeechStreamListener(this); > mDOMStream->GetStream()->AddListener(mSpeechListener); > >@@ -698,11 +700,15 @@ SpeechRecognition::Start(const Optional<NonNull<DOMMediaStream>>& aStream, Error > } > > mRecognitionService = GetSpeechRecognitionService(); >- NS_ENSURE_TRUE_VOID(mRecognitionService); >+ if (NS_WARN_IF(!mRecognitionService)) { >+ return; >+ } > > nsresult rv; > rv = mRecognitionService->Initialize(this); >- NS_ENSURE_SUCCESS_VOID(rv); >+ if (NS_WARN_IF(NS_FAILED(rv))) { >+ return; >+ } I prefer use of NS_ENSURE_* macros over if (NS_WARN_IF()) > SpeechRecognitionAlternative::SpeechRecognitionAlternative(SpeechRecognition* aParent) >- : mTranscript(NS_LITERAL_STRING("")) >+ : mTranscript(EmptyString()) The right fix would be to drop explicit construction here. Strings are empty by default > interface SpeechRecognitionResult { > readonly attribute unsigned long length; > getter SpeechRecognitionAlternative item(unsigned long index); >- readonly attribute boolean final; >+ readonly attribute boolean isFinal; Why this change? The spec has interface SpeechRecognitionResult { readonly attribute unsigned long length; getter SpeechRecognitionAlternative item(in unsigned long index); readonly attribute boolean final; }; please undo, or explain.
Attachment #8610065 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 47•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #46) > Comment on attachment 8610065 [details] [diff] [review] > Part 1 of 2: Patch that introduces WebSpeech API implementation > > >+ mPSConfig = cmd_ln_init(NULL, ps_args(), TRUE, "-hmm", > >+ ToNewUTF8String(aStringAMPath), // acoustic model > >+ "-dict", ToNewUTF8String(aStringDictPath), NULL); > >+ if (mPSConfig == NULL) { > nullptr, not NULL. Also elsewhere. OK. I'll make these changes.
Assignee | ||
Comment 48•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #46) > Comment on attachment 8610065 [details] [diff] [review] > Part 1 of 2: Patch that introduces WebSpeech API implementation > > >+PocketSphinxSpeechRecognitionService::~PocketSphinxSpeechRecognitionService() > >+{ > >+ if (mPSConfig) > >+ free(mPSConfig); > >+ if (mPSHandle) > >+ free(mPSHandle); > Always {} with if. Also elsewhere OK. I'll make these changes.
Assignee | ||
Comment 49•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #46) > Comment on attachment 8610065 [details] [diff] [review] > Part 1 of 2: Patch that introduces WebSpeech API implementation > > > >+ > >+ NS_ConvertUTF16toUTF8 utf8Str(grammar); > >+ const nsACString& utf8AStr = utf8Str; > >+ int result = ps_set_jsgf_string(mPSHandle, "name", > >+ PromiseFlatCString(utf8AStr).get()); > You could just do > NS_ConvertUTF16toUTF8(grammar).get() OK. I can change this.
Assignee | ||
Comment 50•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #46) > Comment on attachment 8610065 [details] [diff] [review] > Part 1 of 2: Patch that introduces WebSpeech API implementation > >+NS_IMETHODIMP > >+PocketSphinxSpeechRecognitionService::Observe(nsISupports* aSubject, > >+ const char* aTopic, > >+ const char16_t* aData) > >+{ > >+ MOZ_ASSERT(mRecognition->mTestConfig.mFakeRecognitionService, > >+ "Got request to fake recognition service event, " > >+ "but " TEST_PREFERENCE_FAKE_RECOGNITION_SERVICE " is not set"); > >+ > >+ if (!strcmp(aTopic, SPEECH_RECOGNITION_TEST_END_TOPIC)) { > >+ nsCOMPtr<nsIObserverService> obs = services::GetObserverService(); > >+ obs->RemoveObserver(this, SPEECH_RECOGNITION_TEST_EVENT_REQUEST_TOPIC); > >+ obs->RemoveObserver(this, SPEECH_RECOGNITION_TEST_END_TOPIC); > >+ > >+ return NS_OK; > >+ } > >+ > >+ const nsDependentString eventName = nsDependentString(aData); > >+ > >+ if (eventName.EqualsLiteral("EVENT_RECOGNITIONSERVICE_ERROR")) { > >+ mRecognition->DispatchError( > >+ SpeechRecognition::EVENT_RECOGNITIONSERVICE_ERROR, > >+ SpeechRecognitionErrorCode::Network, // TODO different codes? > >+ NS_LITERAL_STRING("RECOGNITIONSERVICE_ERROR test event")); > >+ > >+ } else if (eventName.EqualsLiteral("EVENT_RECOGNITIONSERVICE_FINAL_RESULT")) { > >+ nsRefPtr<SpeechEvent> event = new SpeechEvent( > >+ mRecognition, SpeechRecognition::EVENT_RECOGNITIONSERVICE_FINAL_RESULT); > >+ > >+ event->mRecognitionResultList = BuildMockResultList(); > >+ NS_DispatchToMainThread(event); > >+ } > >+ > >+ return NS_OK; > >+} > >+ > >+SpeechRecognitionResultList* > >+PocketSphinxSpeechRecognitionService::BuildMockResultList() > >+{ > >+ SpeechRecognitionResultList* resultList = > >+ new SpeechRecognitionResultList(mRecognition); > >+ SpeechRecognitionResult* result = new SpeechRecognitionResult(mRecognition); > >+ SpeechRecognitionAlternative* alternative = > >+ new SpeechRecognitionAlternative(mRecognition); > >+ > >+ alternative->mTranscript = NS_LITERAL_STRING("Mock final result"); > >+ alternative->mConfidence = 0.0f; > >+ > >+ result->mItems.AppendElement(alternative); > >+ resultList->mItems.AppendElement(result); > >+ > >+ return resultList; > >+} > Please use 4 spaces for indentation. OK. I'll change this.
Assignee | ||
Comment 51•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #46) > Comment on attachment 8610065 [details] [diff] [review] > Part 1 of 2: Patch that introduces WebSpeech API implementation > > >+ /** > >+ * Builds a mock SpeechRecognitionResultList > >+ */ > >+ dom::SpeechRecognitionResultList *BuildMockResultList(); > * goes with the type OK. I'll change here and other places too.
Assignee | ||
Comment 52•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #46) > Comment on attachment 8610065 [details] [diff] [review] > Part 1 of 2: Patch that introduces WebSpeech API implementation > > >+ /** Flag to verify if decoder was created */ > >+ bool ISDecoderCreated; > >+ > >+ /** Flag to verify if grammar was compiled */ > >+ bool ISGrammarCompiled; > > member variables should be name mFoo > Ok will change this.
Assignee | ||
Comment 53•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #46) > Comment on attachment 8610065 [details] [diff] [review] > Part 1 of 2: Patch that introduces WebSpeech API implementation > > >+ > >+ /** Audio data */ > >+ std::vector<int16_t> mAudioVector; > I'd prefer using nsTArray, of if we know some expected size for the array, > perhaps even nsAutoTArray (that might reduce memory churn quite a bit). > The size of this is not known at compile time. In particular, its size is dependent upon the size of the AudioSegment which isn't known at compile time as it's dependent upon what the user says. (See the implementation of the method PocketSphinxSpeechRecognitionService::ProcessAudioSegment). That being said, I don't see the advantage. Maybe there is something I am missing?
Assignee | ||
Comment 54•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #46) > Comment on attachment 8610065 [details] [diff] [review] > Part 1 of 2: Patch that introduces WebSpeech API implementation > > >+++ b/dom/media/webspeech/recognition/SpeechGrammar.h > >@@ -49,6 +49,8 @@ private: > > ~SpeechGrammar(); > > > > nsCOMPtr<nsISupports> mParent; > >+ > >+ nsString mGram; > Could you call this mSrc; OK. I'll make this change.
Assignee | ||
Comment 55•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #46) > Comment on attachment 8610065 [details] [diff] [review] > Part 1 of 2: Patch that introduces WebSpeech API implementation > > >+++ b/dom/media/webspeech/recognition/SpeechGrammarList.h > >@@ -56,6 +56,8 @@ private: > > ~SpeechGrammarList(); > > > > nsCOMPtr<nsISupports> mParent; > >+ > >+ nsTArray<nsRefPtr<SpeechGrammar> > mItems; > drop space between > and >. These days all the compilers support >>, in the > old days > > > had to be used. OK. I'll make this change.
Assignee | ||
Comment 56•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #46) > Comment on attachment 8610065 [details] [diff] [review] > Part 1 of 2: Patch that introduces WebSpeech API implementation > > >@@ -535,7 +535,9 @@ SpeechRecognition::StartRecording(DOMMediaStream* aDOMStream) > > // doesn't get Destroy()'ed > > mDOMStream = aDOMStream; > > > >- NS_ENSURE_STATE(mDOMStream->GetStream()); > >+ if (NS_WARN_IF(!mDOMStream->GetStream())) { > >+ return NS_ERROR_UNEXPECTED; > >+ } > > mSpeechListener = new SpeechStreamListener(this); > > mDOMStream->GetStream()->AddListener(mSpeechListener); > > > >@@ -698,11 +700,15 @@ SpeechRecognition::Start(const Optional<NonNull<DOMMediaStream>>& aStream, Error > > } > > > > mRecognitionService = GetSpeechRecognitionService(); > >- NS_ENSURE_TRUE_VOID(mRecognitionService); > >+ if (NS_WARN_IF(!mRecognitionService)) { > >+ return; > >+ } > > > > nsresult rv; > > rv = mRecognitionService->Initialize(this); > >- NS_ENSURE_SUCCESS_VOID(rv); > >+ if (NS_WARN_IF(NS_FAILED(rv))) { > >+ return; > >+ } > I prefer use of NS_ENSURE_* macros over if (NS_WARN_IF()) > The Mozilla coding standard[1] mandates use of NS_WARN_IF in new code. I quote "Previously the NS_ENSURE_* macros were used for this purpose, but those macros hide return statements and should not be used in new code." So, I'd like to keep it as it is. [1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Use_the_nice_macros
Assignee | ||
Comment 57•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #46) > Comment on attachment 8610065 [details] [diff] [review] > Part 1 of 2: Patch that introduces WebSpeech API implementation > > >+ mPSConfig = cmd_ln_init(NULL, ps_args(), TRUE, "-hmm", > >+ ToNewUTF8String(aStringAMPath), // acoustic model > >+ "-dict", ToNewUTF8String(aStringDictPath), NULL); > >+ if (mPSConfig == NULL) { > nullptr, not NULL. Also elsewhere. > > > >+PocketSphinxSpeechRecognitionService::~PocketSphinxSpeechRecognitionService() > >+{ > >+ if (mPSConfig) > >+ free(mPSConfig); > >+ if (mPSHandle) > >+ free(mPSHandle); > Always {} with if. Also elsewhere > > >+ > >+ NS_ConvertUTF16toUTF8 utf8Str(grammar); > >+ const nsACString& utf8AStr = utf8Str; > >+ int result = ps_set_jsgf_string(mPSHandle, "name", > >+ PromiseFlatCString(utf8AStr).get()); > You could just do > NS_ConvertUTF16toUTF8(grammar).get() > > >+NS_IMETHODIMP > >+PocketSphinxSpeechRecognitionService::Observe(nsISupports* aSubject, > >+ const char* aTopic, > >+ const char16_t* aData) > >+{ > >+ MOZ_ASSERT(mRecognition->mTestConfig.mFakeRecognitionService, > >+ "Got request to fake recognition service event, " > >+ "but " TEST_PREFERENCE_FAKE_RECOGNITION_SERVICE " is not set"); > >+ > >+ if (!strcmp(aTopic, SPEECH_RECOGNITION_TEST_END_TOPIC)) { > >+ nsCOMPtr<nsIObserverService> obs = services::GetObserverService(); > >+ obs->RemoveObserver(this, SPEECH_RECOGNITION_TEST_EVENT_REQUEST_TOPIC); > >+ obs->RemoveObserver(this, SPEECH_RECOGNITION_TEST_END_TOPIC); > >+ > >+ return NS_OK; > >+ } > >+ > >+ const nsDependentString eventName = nsDependentString(aData); > >+ > >+ if (eventName.EqualsLiteral("EVENT_RECOGNITIONSERVICE_ERROR")) { > >+ mRecognition->DispatchError( > >+ SpeechRecognition::EVENT_RECOGNITIONSERVICE_ERROR, > >+ SpeechRecognitionErrorCode::Network, // TODO different codes? > >+ NS_LITERAL_STRING("RECOGNITIONSERVICE_ERROR test event")); > >+ > >+ } else if (eventName.EqualsLiteral("EVENT_RECOGNITIONSERVICE_FINAL_RESULT")) { > >+ nsRefPtr<SpeechEvent> event = new SpeechEvent( > >+ mRecognition, SpeechRecognition::EVENT_RECOGNITIONSERVICE_FINAL_RESULT); > >+ > >+ event->mRecognitionResultList = BuildMockResultList(); > >+ NS_DispatchToMainThread(event); > >+ } > >+ > >+ return NS_OK; > >+} > >+ > >+SpeechRecognitionResultList* > >+PocketSphinxSpeechRecognitionService::BuildMockResultList() > >+{ > >+ SpeechRecognitionResultList* resultList = > >+ new SpeechRecognitionResultList(mRecognition); > >+ SpeechRecognitionResult* result = new SpeechRecognitionResult(mRecognition); > >+ SpeechRecognitionAlternative* alternative = > >+ new SpeechRecognitionAlternative(mRecognition); > >+ > >+ alternative->mTranscript = NS_LITERAL_STRING("Mock final result"); > >+ alternative->mConfidence = 0.0f; > >+ > >+ result->mItems.AppendElement(alternative); > >+ resultList->mItems.AppendElement(result); > >+ > >+ return resultList; > >+} > Please use 4 spaces for indentation. > But I'm now here lost with the "mock" setup. Why BuildMockResultList? > > > >+ /** > >+ * Builds a mock SpeechRecognitionResultList > >+ */ > >+ dom::SpeechRecognitionResultList *BuildMockResultList(); > * goes with the type > > >+ > >+ /** Speex state */ > >+ SpeexResamplerState *mSpeexState; > ditto. SpeexResamplerState* mSpeexState; > > >+ > >+ /** Pocksphix decoder */ > >+ ps_decoder_t *mPSHandle; > ditto > > >+ > >+ /** Sphinxbase parsed command line arguments */ > >+ cmd_ln_t *mPSConfig; > > ditto > >+ /** Flag to verify if decoder was created */ > >+ bool ISDecoderCreated; > >+ > >+ /** Flag to verify if grammar was compiled */ > >+ bool ISGrammarCompiled; > > member variables should be name mFoo > > >+ > >+ /** Audio data */ > >+ std::vector<int16_t> mAudioVector; > I'd prefer using nsTArray, of if we know some expected size for the array, > perhaps even nsAutoTArray (that might reduce memory churn quite a bit). > > > > >+++ b/dom/media/webspeech/recognition/SpeechGrammar.h > >@@ -49,6 +49,8 @@ private: > > ~SpeechGrammar(); > > > > nsCOMPtr<nsISupports> mParent; > >+ > >+ nsString mGram; > Could you call this mSrc; > > > > > >+++ b/dom/media/webspeech/recognition/SpeechGrammarList.h > >@@ -56,6 +56,8 @@ private: > > ~SpeechGrammarList(); > > > > nsCOMPtr<nsISupports> mParent; > >+ > >+ nsTArray<nsRefPtr<SpeechGrammar> > mItems; > drop space between > and >. These days all the compilers support >>, in the > old days > > > had to be used. > > > > >@@ -535,7 +535,9 @@ SpeechRecognition::StartRecording(DOMMediaStream* aDOMStream) > > // doesn't get Destroy()'ed > > mDOMStream = aDOMStream; > > > >- NS_ENSURE_STATE(mDOMStream->GetStream()); > >+ if (NS_WARN_IF(!mDOMStream->GetStream())) { > >+ return NS_ERROR_UNEXPECTED; > >+ } > > mSpeechListener = new SpeechStreamListener(this); > > mDOMStream->GetStream()->AddListener(mSpeechListener); > > > >@@ -698,11 +700,15 @@ SpeechRecognition::Start(const Optional<NonNull<DOMMediaStream>>& aStream, Error > > } > > > > mRecognitionService = GetSpeechRecognitionService(); > >- NS_ENSURE_TRUE_VOID(mRecognitionService); > >+ if (NS_WARN_IF(!mRecognitionService)) { > >+ return; > >+ } > > > > nsresult rv; > > rv = mRecognitionService->Initialize(this); > >- NS_ENSURE_SUCCESS_VOID(rv); > >+ if (NS_WARN_IF(NS_FAILED(rv))) { > >+ return; > >+ } > I prefer use of NS_ENSURE_* macros over if (NS_WARN_IF()) > > > > > > SpeechRecognitionAlternative::SpeechRecognitionAlternative(SpeechRecognition* aParent) > >- : mTranscript(NS_LITERAL_STRING("")) > >+ : mTranscript(EmptyString()) > The right fix would be to drop explicit construction here. Strings are empty > by default > s/NS_LITERAL_STRING("")/EmptyString()/g doesn't usually make such changes. But, I can remove the explicit member construction here.
Assignee | ||
Comment 58•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #46) > Comment on attachment 8610065 [details] [diff] [review] > Part 1 of 2: Patch that introduces WebSpeech API implementation > > > > interface SpeechRecognitionResult { > > readonly attribute unsigned long length; > > getter SpeechRecognitionAlternative item(unsigned long index); > >- readonly attribute boolean final; > >+ readonly attribute boolean isFinal; > Why this change? > The spec has > interface SpeechRecognitionResult { > readonly attribute unsigned long length; > getter SpeechRecognitionAlternative item(in unsigned long index); > readonly attribute boolean final; > }; > please undo, or explain. There are two versions of the spec the 19 October 2012 version [3] and the 6 June 2014 version [4]. The speech synthesis portion of the API targets the newer version of the spec; thus, the recognition portion of the API should also target this newer version of the spec. The spec you are referring to is the older version of the spec [1]. The newer version of the spec [2] changed this member name from final to isFinal. Thus I have also changed this member name to adhere to the newer spec. [1] https://dvcs.w3.org/hg/speech-api/raw-file/tip/speechapi.html#dfn-final [2] https://dvcs.w3.org/hg/speech-api/raw-file/tip/webspeechapi.html#dfn-isFinal [3] https://dvcs.w3.org/hg/speech-api/raw-file/tip/speechapi.html [4] https://dvcs.w3.org/hg/speech-api/raw-file/tip/webspeechapi.html
Assignee | ||
Comment 59•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #46) > Comment on attachment 8610065 [details] [diff] [review] > Part 1 of 2: Patch that introduces WebSpeech API implementation > > >+NS_IMETHODIMP > >+PocketSphinxSpeechRecognitionService::Observe(nsISupports* aSubject, > >+ const char* aTopic, > >+ const char16_t* aData) > >+{ > >+ MOZ_ASSERT(mRecognition->mTestConfig.mFakeRecognitionService, > >+ "Got request to fake recognition service event, " > >+ "but " TEST_PREFERENCE_FAKE_RECOGNITION_SERVICE " is not set"); > >+ > >+ if (!strcmp(aTopic, SPEECH_RECOGNITION_TEST_END_TOPIC)) { > >+ nsCOMPtr<nsIObserverService> obs = services::GetObserverService(); > >+ obs->RemoveObserver(this, SPEECH_RECOGNITION_TEST_EVENT_REQUEST_TOPIC); > >+ obs->RemoveObserver(this, SPEECH_RECOGNITION_TEST_END_TOPIC); > >+ > >+ return NS_OK; > >+ } > >+ > >+ const nsDependentString eventName = nsDependentString(aData); > >+ > >+ if (eventName.EqualsLiteral("EVENT_RECOGNITIONSERVICE_ERROR")) { > >+ mRecognition->DispatchError( > >+ SpeechRecognition::EVENT_RECOGNITIONSERVICE_ERROR, > >+ SpeechRecognitionErrorCode::Network, // TODO different codes? > >+ NS_LITERAL_STRING("RECOGNITIONSERVICE_ERROR test event")); > >+ > >+ } else if (eventName.EqualsLiteral("EVENT_RECOGNITIONSERVICE_FINAL_RESULT")) { > >+ nsRefPtr<SpeechEvent> event = new SpeechEvent( > >+ mRecognition, SpeechRecognition::EVENT_RECOGNITIONSERVICE_FINAL_RESULT); > >+ > >+ event->mRecognitionResultList = BuildMockResultList(); > >+ NS_DispatchToMainThread(event); > >+ } > >+ > >+ return NS_OK; > >+} > >+ > >+SpeechRecognitionResultList* > >+PocketSphinxSpeechRecognitionService::BuildMockResultList() > >+{ > >+ SpeechRecognitionResultList* resultList = > >+ new SpeechRecognitionResultList(mRecognition); > >+ SpeechRecognitionResult* result = new SpeechRecognitionResult(mRecognition); > >+ SpeechRecognitionAlternative* alternative = > >+ new SpeechRecognitionAlternative(mRecognition); > >+ > >+ alternative->mTranscript = NS_LITERAL_STRING("Mock final result"); > >+ alternative->mConfidence = 0.0f; > >+ > >+ result->mItems.AppendElement(alternative); > >+ resultList->mItems.AppendElement(result); > >+ > >+ return resultList; > >+} > Please use 4 spaces for indentation. I've a question.... The Mozilla Coding Convention [1] states of indentation: "Two spaces per logic level, or four spaces in Python code." My reading of this is that indentation should be two spaces. Also, the code examples given by the Mozilla Coding Convention use two spaces for indentation. So it seems as if two spaces is the Mozilla coding convention. Is this not so? [1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Indentation
Comment 60•9 years ago
|
||
(In reply to kdavis from comment #59) > > >+ return resultList; > > >+} > > Please use 4 spaces for indentation. > > I've a question.... > > The Mozilla Coding Convention [1] states of indentation: > > "Two spaces per logic level, or four spaces in Python code." > > My reading of this is that indentation should be two spaces. YES. What did I write. 2 space. 2 2 2. I must have missing "don't"
Comment 61•9 years ago
|
||
(In reply to kdavis from comment #58) > (In reply to Olli Pettay [:smaug] from comment #46) > > Comment on attachment 8610065 [details] [diff] [review] > > Part 1 of 2: Patch that introduces WebSpeech API implementation > > > > > > > interface SpeechRecognitionResult { > > > readonly attribute unsigned long length; > > > getter SpeechRecognitionAlternative item(unsigned long index); > > >- readonly attribute boolean final; > > >+ readonly attribute boolean isFinal; > > Why this change? > > The spec has > > interface SpeechRecognitionResult { > > readonly attribute unsigned long length; > > getter SpeechRecognitionAlternative item(in unsigned long index); > > readonly attribute boolean final; > > }; > > please undo, or explain. > > There are two versions of the spec the 19 October 2012 version [3] and the > 6 June 2014 version [4]. The speech synthesis portion of the API targets > the newer version of the spec; thus, the recognition portion of the API > should also target this newer version of the spec. > > The spec you are referring to is the older version of the spec [1]. The newer > version of the spec [2] changed this member name from final to isFinal. Thus > I have also changed this member name to adhere to the newer spec. > > [1] https://dvcs.w3.org/hg/speech-api/raw-file/tip/speechapi.html#dfn-final > [2] > https://dvcs.w3.org/hg/speech-api/raw-file/tip/webspeechapi.html#dfn-isFinal > [3] https://dvcs.w3.org/hg/speech-api/raw-file/tip/speechapi.html > [4] https://dvcs.w3.org/hg/speech-api/raw-file/tip/webspeechapi.html I see. It is possible that looked at https://dvcs.w3.org/hg/speech-api/raw-file/tip/speechapi.html last night. "tip" in the url is very misleading.
Comment 62•9 years ago
|
||
(In reply to kdavis from comment #56) > > The Mozilla coding standard[1] mandates use of NS_WARN_IF in new code. I > quote > > "Previously the NS_ENSURE_* macros were used for this purpose, but those > macros > hide return statements and should not be used in new code." > Hmm, who added that to the coding style page. It is not agreed. But fine.
Comment 63•9 years ago
|
||
(In reply to kdavis from comment #53) > > That being said, I don't see the advantage. Maybe there is something I am > missing? If we can estimate what the needed size is, using nsAutoTArray would reduce malloc/free calls possibly significantly. And nsTArray vs std::vector: we know how nsTArray behaves from performance point of view. We can't have similar guarantees for vector. See also https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code#C.2B.2B_and_Mozilla_standard_libraries
Assignee | ||
Comment 64•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #63) > (In reply to kdavis from comment #53) > If we can estimate what the needed size is, using nsAutoTArray would reduce > malloc/free calls possibly significantly. And nsTArray vs std::vector: we > know how nsTArray behaves from performance point of view. We can't have > similar guarantees for vector. > See also > https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code#C.2B. > 2B_and_Mozilla_standard_libraries We can't estimate the needed size as we don't know what the user will say at compile time. So, the initial capacity of a nsAutoTArray, or for that matter std::vector, is not going to help us much. Also, I'm not sure what you mean by... > we know how nsTArray behaves from performance point of view. We can't > have similar guarantees for vector The C++ standard makes performance guarantees for its containers. For example for std::vector see sections 23.3.6.2-23.3.6.5 of the current working draft [1] of the C++ standard. The complexity of each method and constructor is specified. However, I can use a nsTArray, but I don't think the stated reasons justify its use. [1] http://open-std.org/JTC1/SC22/WG21/docs/papers/2015/n4431.pdf
Comment 65•9 years ago
|
||
(In reply to kdavis from comment #64) > We can't estimate the needed size as we don't know what the user will say > at compile time. So, the initial capacity of a nsAutoTArray, or for that > matter std::vector, is not going to help us much. Well, I was thinking if we could estimate how much some shorter speech commands would usually take memory and just use nsAutoTArray at least that size. But perhaps not worth. > > we know how nsTArray behaves from performance point of view. We can't > > have similar guarantees for vector > > The C++ standard makes performance guarantees for its containers. It does? Does it guarantee in which way the buffer inside vector is increased?
Assignee | ||
Comment 66•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #65) > (In reply to kdavis from comment #64) > > > The C++ standard makes performance guarantees for its containers. > It does? Does it guarantee in which way the buffer inside vector is > increased? The listed complexities of 23.3.6.2-23.3.6.5 are for the most part time complexities not space complexities. Space complexities are indeed under specified by the standard. However, time complexities are, more-or-less, fully specified. Be that as it may, I'll use nsTArray. Thus, the question of std::vector space complexity will not arise.
Assignee | ||
Comment 67•9 years ago
|
||
Comment on attachment 8610066 [details] [diff] [review] Part 2 of 2: Patch that turns off the WebSpeech API implementation using flags Flagging gps for review of build changes
Attachment #8610066 -
Flags: superreview?(gps)
Assignee | ||
Comment 68•9 years ago
|
||
Part 1 of 2: Patch that introduces WebSpeech API implementation This is the part 1 of 2 for this bug. This patch introduces the Pocketsphinx/Sphinxbase implementation of the WebSpeech API. This patch addresses the review suggestions of Comment 46. This implementation depends upon all of the patches for Bug 1051146. The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=68e31ec78735
Attachment #8610065 -
Attachment is obsolete: true
Attachment #8612098 -
Flags: review+
Assignee | ||
Comment 69•9 years ago
|
||
(In reply to kdavis from comment #67) > Comment on attachment 8610066 [details] [diff] [review] > Part 2 of 2: Patch that turns off the WebSpeech API implementation using > flags > > Flagging gps for review of build changes gps is this alright as is?
Flags: needinfo?(gps)
Updated•9 years ago
|
Whiteboard: [webspeechapi] → [webspeechapi][systemsfe]
Target Milestone: --- → 2.2 S14 (12june)
Comment 71•9 years ago
|
||
Comment on attachment 8610066 [details] [diff] [review] Part 2 of 2: Patch that turns off the WebSpeech API implementation using flags Review of attachment 8610066 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure why you are changing configure.in: you need the AC_SUBST bits for this to work. ::: configure.in @@ -5191,5 @@ > -if test -n "$MOZ_WEBSPEECH_POCKETSPHINX"; then > - AC_DEFINE(MOZ_WEBSPEECH_POCKETSPHINX) > -fi > - > -AC_SUBST(MOZ_WEBSPEECH_POCKETSPHINX) Removing MOZ_WEBSPEECH_POCKETSPHINX from AC_SUBST will not expose this variable to moz.build files, causing the check for feature presence in the moz.build added in part 1 to never return true.
Attachment #8610066 -
Flags: superreview?(gps) → superreview-
Updated•9 years ago
|
Flags: needinfo?(gps)
Assignee | ||
Comment 72•9 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #71) > Comment on attachment 8610066 [details] [diff] [review] > Part 2 of 2: Patch that turns off the WebSpeech API implementation using > flags > > Review of attachment 8610066 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm not sure why you are changing configure.in: you need the AC_SUBST bits > for this to work. > > ::: configure.in > @@ -5191,5 @@ > > -if test -n "$MOZ_WEBSPEECH_POCKETSPHINX"; then > > - AC_DEFINE(MOZ_WEBSPEECH_POCKETSPHINX) > > -fi > > - > > -AC_SUBST(MOZ_WEBSPEECH_POCKETSPHINX) > > Removing MOZ_WEBSPEECH_POCKETSPHINX from AC_SUBST will not expose this > variable to moz.build files, causing the check for feature presence in the > moz.build added in part 1 to never return true. I think only the MOZ_ARG_DISABLE_BOOL should be removed so the flag is not on by default. So, I'd think the diff should be something like... dnl ======================================================== dnl = Disable Speech API models dnl ======================================================== -MOZ_ARG_DISABLE_BOOL(webspeechmodels, -[ --disable-webspeechmodels Disable support for HTML Speech API Models], - MOZ_WEBSPEECH_MODELS=, - MOZ_WEBSPEECH_MODELS=1) if test -n "$MOZ_WEBSPEECH_MODELS"; then AC_DEFINE(MOZ_WEBSPEECH_MODELS) fi AC_SUBST(MOZ_WEBSPEECH_MODELS) and a similar thing for the other similar flags. Seem reasonable?
Flags: needinfo?(gps)
Assignee | ||
Comment 73•9 years ago
|
||
Part 2 of 2: Patch that turns off the WebSpeech API implementation using flags This is the part 2 of 2 for this bug. This patch turns off the WebSpeech API implementation using flags. This implementation depends upon all of the patches for Bug 1051146. This patch obsoletes attachment 8612098 [details] [diff] [review], its previous version, and implements the suggestions of the build peer review Comment 71 The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=365759599600
Attachment #8612098 -
Attachment is obsolete: true
Attachment #8614016 -
Flags: review?(gps)
Comment 74•9 years ago
|
||
Comment on attachment 8614016 [details] [diff] [review] Part 2 of 2: Patch that turns off the WebSpeech API implementation using flags Review of attachment 8614016 [details] [diff] [review]: ----------------------------------------------------------------- This looks better. It's kinda weird not having a way to enable a feature via a configure argument. You could switch this to MOZ_ARG_ENABLE_BOOL and make it opt in by default. But this is your feature and I don't care too much to enforce configure flag presence.
Attachment #8614016 -
Flags: review?(gps) → review+
Updated•9 years ago
|
Flags: needinfo?(gps)
Assignee | ||
Comment 75•9 years ago
|
||
Comment on attachment 8612098 [details] [diff] [review] Part 1 of 2: Patch that introduces WebSpeech API implementation Clicked wrong box
Attachment #8612098 -
Attachment is obsolete: false
Assignee | ||
Comment 76•9 years ago
|
||
Comment on attachment 8610066 [details] [diff] [review] Part 2 of 2: Patch that turns off the WebSpeech API implementation using flags Clicked wrong box
Attachment #8610066 -
Attachment is obsolete: true
Keywords: checkin-needed
Comment 77•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a0493fb37db https://hg.mozilla.org/integration/mozilla-inbound/rev/b2fb4270e0a6
Keywords: checkin-needed
Comment 78•9 years ago
|
||
Backed out for the rooting failures identified in bug 1051146 comment 139.
Comment 79•9 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/51b0f06606ad
Comment 80•9 years ago
|
||
"init" needs to become a RootedDictionary<SpeechRecognitionEventInit>.
Assignee | ||
Comment 81•9 years ago
|
||
(In reply to Not doing reviews right now from comment #80) > "init" needs to become a RootedDictionary<SpeechRecognitionEventInit>. Thanks!
Assignee | ||
Comment 82•9 years ago
|
||
Part 1 of 2: Patch that introduces WebSpeech API implementation This is the part 1 of 2 for this bug. This patch introduces the Pocketsphinx/Sphinxbase implementation of the WebSpeech API. Relative to the patch it obsoletes this patch introduces only a single change on a single line, that suggested in Comment 80. To run properly this patch depends upon Bug 1171249, which is currently tagged checkin-needed, and Bug 1171850, a one line change, that is waiting on build peer review. The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e1382c3709d
Attachment #8612098 -
Attachment is obsolete: true
Attachment #8615907 -
Flags: review+
Assignee | ||
Comment 83•9 years ago
|
||
Part 2 of 2: Patch that turns off the WebSpeech API implementation using flags This is the part 2 of 2 for this bug. This patch turns off the WebSpeech API implementation using flags. This implementation depends upon Bug 1171249, which is currently tagged checkin-needed. Relative to the patch it obsoletes this patch make no material changes. It only compensates for the line number changes that are needed as a result of Bug 1171249. The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=372d7aa2b034
Attachment #8614016 -
Attachment is obsolete: true
Attachment #8615913 -
Flags: review+
Attachment #8615907 -
Flags: review+ → review?(bugs)
Attachment #8615913 -
Flags: review+ → review?(bugs)
Attachment #8615913 -
Flags: review?(bugs) → review?(gps)
Assignee | ||
Comment 84•9 years ago
|
||
Comment on attachment 8615907 [details] [diff] [review] Part 1 of 2: Patch that introduces WebSpeech API implementation The diff between the new and old patches is due to the change... SpeechRecognitionEventInit init; to RootedDictionary<SpeechRecognitionEventInit> init(nsContentUtils::RootingCx()); as suggested in Comment 80. The rest of the diff between patches is simply a result of changes in the "index f311ac2" values. ============================================================================ 8c8 < index 3cf84cf..9d2761c4 100644 --- > index 7f150fe..8cf1d76 100644 11c11 < @@ -1003,6 +1003,10 @@ pref("network.proxy.browsing.app_origins", "app://system.gaiamobile.org"); --- > @@ -1001,6 +1001,10 @@ pref("network.proxy.browsing.app_origins", "app://system.gaiamobile.org"); 586c586 < index 2001e2e..002612a 100644 --- > index 0666153..fbb9c44 100644 610,616c610 < @@ -472,12 +472,12 @@ SpeechRecognition::NotifyFinalResult(SpeechEvent* aEvent) < { < ResetAndEnd(); < < - SpeechRecognitionEventInit init; < + RootedDictionary<SpeechRecognitionEventInit> init(nsContentUtils::RootingCx()); < init.mBubbles = true; --- > @@ -477,7 +477,7 @@ SpeechRecognition::NotifyFinalResult(SpeechEvent* aEvent) 731c725 < index 14cb375..8002a2f 100644 --- > index f311ac2..ee3ac68 100644 734c728 < @@ -26,6 +26,11 @@ if CONFIG['MOZ_WEBSPEECH_TEST_BACKEND']: --- > @@ -29,6 +29,11 @@ if CONFIG['MOZ_WEBSPEECH_TEST_BACKEND']: 746c740 < @@ -44,10 +49,21 @@ if CONFIG['MOZ_WEBSPEECH_TEST_BACKEND']: --- > @@ -47,10 +52,21 @@ if CONFIG['MOZ_WEBSPEECH_TEST_BACKEND']:
Assignee | ||
Comment 85•9 years ago
|
||
Comment on attachment 8615907 [details] [diff] [review] Part 1 of 2: Patch that introduces WebSpeech API implementation Even more easily understood is the interdiff between the new and old patches: diff -u b/dom/media/webspeech/recognition/SpeechRecognition.cpp b/dom/media/webspeech/recognition/SpeechRecognition.cpp --- b/dom/media/webspeech/recognition/SpeechRecognition.cpp +++ b/dom/media/webspeech/recognition/SpeechRecognition.cpp @@ -472,7 +472,7 @@ { ResetAndEnd(); - RootedDictionary<SpeechRecognitionEventInit> init(nsContentUtils::RootingCx()); + SpeechRecognitionEventInit init; init.mBubbles = true; init.mCancelable = false; // init.mResultIndex = 0;
Assignee | ||
Comment 86•9 years ago
|
||
Comment on attachment 8615913 [details] [diff] [review] Part 2 of 2: Patch that turns off the WebSpeech API implementation using flags The interdiff between the new and old patches here is simple it's the empty file.
Comment 87•9 years ago
|
||
Comment on attachment 8615913 [details] [diff] [review] Part 2 of 2: Patch that turns off the WebSpeech API implementation using flags Review of attachment 8615913 [details] [diff] [review]: ----------------------------------------------------------------- Haven't I reviewed this already?
Attachment #8615913 -
Flags: review?(gps) → review+
Assignee | ||
Comment 88•9 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #87) > Comment on attachment 8615913 [details] [diff] [review] > Part 2 of 2: Patch that turns off the WebSpeech API implementation using > flags > > Review of attachment 8615913 [details] [diff] [review]: > ----------------------------------------------------------------- > > Haven't I reviewed this already? Yeah. The diff here shifts line numbers of the changes, other that that it's the same. The line numbers had to shift as a result of Bug 1171249. Gregor just wanted me run it by everyone again.
Comment 89•9 years ago
|
||
(In reply to kdavis from comment #88) > (In reply to Gregory Szorc [:gps] from comment #87) > > Comment on attachment 8615913 [details] [diff] [review] > > Part 2 of 2: Patch that turns off the WebSpeech API implementation using > > flags > > > > Review of attachment 8615913 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Haven't I reviewed this already? > > Yeah. The diff here shifts line numbers of the changes, other that that > it's the same. The line numbers had to shift as a result of Bug 1171249. > > Gregor just wanted me run it by everyone again. Oh you don't have to ask for review if only the line numbers change. I was talking about the other patch where you actually made a code change.
Assignee | ||
Comment 90•9 years ago
|
||
(In reply to Gregor Wagner [:gwagner] from comment #89) > (In reply to kdavis from comment #88) > > (In reply to Gregory Szorc [:gps] from comment #87) > > > Comment on attachment 8615913 [details] [diff] [review] > > > Part 2 of 2: Patch that turns off the WebSpeech API implementation using > > > flags > > > > > > Review of attachment 8615913 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > Haven't I reviewed this already? > > > > Yeah. The diff here shifts line numbers of the changes, other that that > > it's the same. The line numbers had to shift as a result of Bug 1171249. > > > > Gregor just wanted me run it by everyone again. > > Oh you don't have to ask for review if only the line numbers change. I was > talking about the other patch where you actually made a code change. I was just looking at the line that causes the problem SpeechRecognition.cpp:475 https://github.com/mozilla/gecko-dev/blob/master/dom/media/webspeech/recognition/SpeechRecognition.cpp#L475 and, strangely enough, my original patch doesn't change that line at all. My patch just left the line as was. Maybe some new commit hook was added between when SpeechRecognition.cpp:475 was originally written and now? Either way, the new patch should fix it.
Updated•9 years ago
|
Attachment #8615907 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 91•9 years ago
|
||
Part 1 of 2: Patch that introduces WebSpeech API implementation This is the part 1 of 2 for this bug. This patch introduces the Pocketsphinx/Sphinxbase implementation of the WebSpeech API. Relative to the patch it obsoletes this patch makes *no* changes other than compensating for line number changes that have occurred in the intervening time period. To run properly this patch depends upon Bug 1172925, which is currently waiting on build peer review. The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b35b8d10b7d
Attachment #8615907 -
Attachment is obsolete: true
Attachment #8617745 -
Flags: review+
Assignee | ||
Comment 92•9 years ago
|
||
Part 2 of 2: Patch that turns off the WebSpeech API implementation using flags This is the part 2 of 2 for this bug. This patch turns off the WebSpeech API implementation using flags. This implementation depends upon Bug 1172925, which is currently waiting on build peer review. Relative to the patch it obsoletes this patch make *no* changes other than compensating for line number changes that have occurred in the intervening time period. The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=e64cefa5f4ca
Attachment #8615913 -
Attachment is obsolete: true
Attachment #8617747 -
Flags: review+
Keywords: checkin-needed
Comment 93•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b901a0609156 https://hg.mozilla.org/integration/mozilla-inbound/rev/28701c2045b3
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b901a0609156 https://hg.mozilla.org/mozilla-central/rev/28701c2045b3
Updated•9 years ago
|
User Story: (updated)
Updated•9 years ago
|
Summary: Development of PocketSphinxSpeechRecognitionService to use pocketsphinx and enable Webspeech API → Implementation of Webspeech API & PocketSphinx Integration
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•