Closed Bug 1051146 Opened 10 years ago Closed 10 years ago

Integrate pocketsphinx sources, and its own moz.build on build system

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox41 fixed)

Status:
RESOLVED FIXED
Milestone:
2.2 S14 (12june)
Tracking Flags:
Tracking Status
firefox41 --- fixed

People

(Reporter: anatal, Assigned: kdavis)

References

Details

(Whiteboard: [webspeechapi][systemsfe])

Attachments

(8 files, 24 obsolete files)

Part 1 of 7: Introduces the B2G specific build flags, initially enabled
2.76 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Part 4 of 7: Introduces build system changes for the MOZ_WEBSPEECH_TEST_BACKEND flag
4.20 KB, patch
smaug
: review+
gps
: review+
eeejay
: feedback+
Details | Diff | Splinter Review
Part 5 of 7: Introduces new English model with no build integration
151 bytes, text/plain
smaug
: review+
Details
Part 2 of 7: Introduces the new Pocketsphinx and Sphinxbase code with no build integration
2.28 MB, patch
smaug
: review+
Details | Diff | Splinter Review
Part 7 of 7: Introduces update scripts for Pocketsphinx and Sphinxbase code
2.42 KB, patch
kdavis
: review+
Details | Diff | Splinter Review
Part 3 of 7: Introduces build system changes for the new Pocketsphinx and Sphinxbase code
5.66 KB, patch
kdavis
: review+
Details | Diff | Splinter Review
Part 6 of 7: Introduces build integration of new English model
1.19 KB, patch
gps
: review+
smaug
: feedback+
Details | Diff | Splinter Review
Disable on Windows to work around bustage
881 bytes, patch
RyanVM
: 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
Blocks: 1049931
OS: Mac OS X → All
Hardware: x86 → All
Blocks: 1051148
No longer blocks: 1049931
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Integrate pocketsphinx sources and write moz.build → Integrate pocketsphinx sources, models, its own moz.build, and package system for all platforms
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Multi-platform Build system patch (Fennec , OSX, Linux, Windows and B2G) to integrate pocketsphinx speech decoder into Firefox and enable Web Speech API 1 - Contains all pocketsphinx sources and models. 2 - Contains the modifications on building and package system for all supported platforms (Fennec , OSX, Linux, Windows and B2G).
Assignee: nobody → anatal
Jonathan, I just finished the build patch and tested on all supported platforms. Can you please indicate the right build peer to review it? Thanks Andre
Flags: needinfo?(jgriffin)
Comment on attachment 8474282 [details] [diff] [review] Multi-platform Build system patch (Fennec , OSX, Linux, Windows and B2G) to integrate pocketsphinx speech decoder into Firefox and enable Web Speech API Can you review this or suggest a more appropriate reviewer?
Attachment #8474282 - Flags: review?(mh+mozilla)
Flags: needinfo?(jgriffin)
Blocks: 1049931
No longer blocks: 1051148
Status: REOPENED → NEW
Depends on: 1051123
Can you separate this in two patches? one with the pocketsphinx code import, and one with the relevant m-c stuff. A few notes from a quick glance at the diffstat: - Don't create a new top-level directory for models - Do we really want to ship 13MB of additional data for this, as opposed to downloading it when required? Has this been discussed with the relevant product managers for desktop, mobile and b2g? - What's the plan for other languages? are we going to have 13MB of data per language (or more)?
(In reply to Mike Hommey [:glandium] from comment #4) > Can you separate this in two patches? one with the pocketsphinx code import, > and one with the relevant m-c stuff. > Ok, will do it > A few notes from a quick glance at the diffstat: > - Don't create a new top-level directory for models Before start to code I discussed this with Ted Mielczarek. Also, including it on root folder was the only way that I found to load it multi-platform using GRE folder. If this is not the correct way, please then indicate how to do it. > - Do we really want to ship 13MB of additional data for this, as opposed to > downloading it when required? Has this been discussed with the relevant > product managers for desktop, mobile and b2g? I agree that for downloadable applications this can be an issue (btw, the english models compacted ends with 9mb), but maybe not for b2g devices. But anyway, I also believe that downloading and installing models can be a better approach, since a user can speak more than one language and require other models. Also, models can be adapted and auto-retrained with user voice on server for improvement. But on other hand, people want to save data, mainly on developed countries. So this is a tradeoff. I talk with Sandip Kamat almost everyday (PM for b2g), and already talked with a lot of people, but I am not sure if one of them was PM for other platforms. Maybe Sandip can help. > - What's the plan for other languages? are we going to have 13MB of data per > language (or more)? Currently there was only conversations to include Spanish, but for new models we need to capture user voice, classify it, and train the models. This is time consuming and need continuous and specialized work. But on the end, yes, the models can get this large. Pocketsphinx offers a number of models as a starting point, but to know its accuracy, we need to test each them with respective native speakers. http://sourceforge.net/projects/cmusphinx/files/Acoustic%20and%20Language%20Models/
Flags: needinfo?(ted)
Flags: needinfo?(skamat)
I mean: "But on other hand, people want to save data, mainly on developing countries."
My current thought for existing markets of FxOS is to preload English and Spanish "language packs" by default and add other languages on demand. 2 things to note: 1. OEMs can decide to change this config for their markets/regions 2. They can also preload another (or replace existing) language packs We should have a table indicating memory footprints for them.
Flags: needinfo?(skamat)
So @glandium, what do I do regards model folder location and shipping method? The end of my work is fully dependent of this decision. Well, anyway I will attach at least the pocketsphinx sources integration.
(In reply to Andre Natal from comment #5) > > A few notes from a quick glance at the diffstat: > > - Don't create a new top-level directory for models > > Before start to code I discussed this with Ted Mielczarek. Also, including > it on root folder was the only way that I found to load it multi-platform > using GRE folder. If this is not the correct way, please then indicate how > to do it. Putting the models into dist/bin/models is fine, but you don't need to have $topsrcdir/models to achieve that. You can put that directory anywhere and the same INSTALL_TARGETS bits will work. Apologies if I miscommunicated that.
Flags: needinfo?(ted)
Thanks. I understand that in staging it is right. So what is the correct place to put when installed, instead $topsrcdir/models?
Flags: needinfo?(ted)
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(ted)
Flags: needinfo?(mh+mozilla)
After discussion with Jonas Sicking, it was decided the user will need to download the models for his language instead embed them on build.
Flags: needinfo?(jonas)
Comment on attachment 8474282 [details] [diff] [review] Multi-platform Build system patch (Fennec , OSX, Linux, Windows and B2G) to integrate pocketsphinx speech decoder into Firefox and enable Web Speech API Resetting flag until something reviewable appears (cf. comments 4 and 11)
Attachment #8474282 - Flags: review?(mh+mozilla)
Summary: Integrate pocketsphinx sources, models, its own moz.build, and package system for all platforms → Integrate pocketsphinx sources, and its own moz.build
Component: DOM → Build Config
Blocks: 1051118
No longer blocks: 1049931
Depends on: 1051148
Depends on: 1065911
No longer depends on: 1051148
Blocks: 1049931
No longer blocks: 1051118
Depends on: 1051607
Flags: needinfo?(jonas)
:glandium This patch covers only the addition of pocketsphinx sources and proper build system configuration to compile it.
Attachment #8474282 - Attachment is obsolete: true
Attachment #8488968 - Flags: review?(mh+mozilla)
Summary: Integrate pocketsphinx sources, and its own moz.build → Integrate pocketsphinx sources, and its own moz.build on build system
No longer depends on: 1051607
No longer depends on: 1065911
Blocks: 1067689
No longer blocks: 1049931
Comment on attachment 8488968 [details] [diff] [review] Integrate pocketsphinx sources, and its own moz.build on build system Review of attachment 8488968 [details] [diff] [review]: ----------------------------------------------------------------- You'll need to update toolkit/content/license.html, and you'll need to find someone to review the code dump. Random thought: importing yet another (maybe not up-to-date) dtoa doesn't sound like a great idea. It would be better to add a wrapper around one we have in the tree already, whichever is better. ::: content/media/webspeech/moz.build @@ -4,5 @@ > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > DIRS = ['synth'] > - > -if CONFIG['MOZ_WEBSPEECH']: The option to disable webspeech still exists. Please leave this here, and add a if CONFIG['MOZ_WEBSPEECH'] in the other moz.builds. Or, if we really want web speech to be enabled everywhere, remove all MOZ_WEBSPEECH occurrences from the tree. ::: media/pocketsphinx/moz.build @@ +4,5 @@ > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > + > +DIRS += ['../../models/', ] You have plenty of trailing whitespaces in this file, please remove them. ../../models ends up being a top-level directory. Didn't we say we wanted that elsewhere? Also, there's no models/moz.build in the patch, so applying it leads to an unbuildable tree. Also, this should be DIRS += [ '...', ] @@ +13,5 @@ > +] > + > + > +EXPORTS.pocketsphinx += [ > + 'cmdln_macro.h', 4-space indentations, please. @@ +88,5 @@ > +if CONFIG['GKMEDIAS_SHARED_LIBRARY']: > + NO_VISIBILITY_FLAGS = True > + > +CXXFLAGS += [ > + '-nostdinc' 4-space indentation, and a comma at the end of the line. That said, why -nostdinc? This isn't a nice flag to add to windows builds, for instance (clang-cl won't like it), so at the very least it should be conditional, if necessary at all. ::: media/sphinxbase/moz.build @@ +2,5 @@ > +# vim: set filetype=python: > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + Same remarks in this file as in media/pocketsphinx/moz.build: 4-space indents, trailing whitespaces, and missing comma. @@ +125,5 @@ > +CXXFLAGS += [ > + '-nostdinc' > +] > + > +FINAL_LIBRARY = 'xul' With this moz.build (and the one in pocketsphinx), you'll have those libraries built in libxul with symbols exported. Is this really your intent? It seems to me the intent is for them to go in libgkmedias.
Attachment #8488968 - Flags: review?(mh+mozilla) → feedback-
Blocks: 1051148
No longer blocks: 1051148
Blocks: 1051148
No longer blocks: 1067689
Attachment #8488968 - Attachment is obsolete: true
Attachment #8500214 - Flags: review?(stephouillon)
Attachment #8500214 - Flags: review?(mh+mozilla)
Attachment #8500214 - Flags: review?(bugs)
(In reply to Mike Hommey [:glandium] from comment #14) > Comment on attachment 8488968 [details] [diff] [review] > Integrate pocketsphinx sources, and its own moz.build on build system > > Review of attachment 8488968 [details] [diff] [review]: > ----------------------------------------------------------------- > > You'll need to update toolkit/content/license.html, and you'll need to find > someone to review the code dump. Random thought: importing yet another > (maybe not up-to-date) dtoa doesn't sound like a great idea. It would be > better to add a wrapper around one we have in the tree already, whichever is > better. > > ::: content/media/webspeech/moz.build > @@ -4,5 @@ > > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > > > DIRS = ['synth'] > > - > > -if CONFIG['MOZ_WEBSPEECH']: > > The option to disable webspeech still exists. Please leave this here, and > add a if CONFIG['MOZ_WEBSPEECH'] in the other moz.builds. Or, if we really > want web speech to be enabled everywhere, remove all MOZ_WEBSPEECH > occurrences from the tree. > > ::: media/pocketsphinx/moz.build > @@ +4,5 @@ > > +# License, v. 2.0. If a copy of the MPL was not distributed with this > > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > > + > > + > > +DIRS += ['../../models/', ] > > You have plenty of trailing whitespaces in this file, please remove them. > > ../../models ends up being a top-level directory. Didn't we say we wanted > that elsewhere? Also, there's no models/moz.build in the patch, so applying > it leads to an unbuildable tree. > > Also, this should be > DIRS += [ > '...', > ] > > @@ +13,5 @@ > > +] > > + > > + > > +EXPORTS.pocketsphinx += [ > > + 'cmdln_macro.h', > > 4-space indentations, please. > > @@ +88,5 @@ > > +if CONFIG['GKMEDIAS_SHARED_LIBRARY']: > > + NO_VISIBILITY_FLAGS = True > > + > > +CXXFLAGS += [ > > + '-nostdinc' > > 4-space indentation, and a comma at the end of the line. That said, why > -nostdinc? This isn't a nice flag to add to windows builds, for instance > (clang-cl won't like it), so at the very least it should be conditional, if > necessary at all. > > ::: media/sphinxbase/moz.build > @@ +2,5 @@ > > +# vim: set filetype=python: > > +# This Source Code Form is subject to the terms of the Mozilla Public > > +# License, v. 2.0. If a copy of the MPL was not distributed with this > > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > > + > > Same remarks in this file as in media/pocketsphinx/moz.build: 4-space > indents, trailing whitespaces, and missing comma. > > @@ +125,5 @@ > > +CXXFLAGS += [ > > + '-nostdinc' > > +] > > + > > +FINAL_LIBRARY = 'xul' > > With this moz.build (and the one in pocketsphinx), you'll have those > libraries built in libxul with symbols exported. Is this really your intent? > It seems to me the intent is for them to go in libgkmedias. Mike, I addressed all issues that you indicated. About dtoa.c, we fixed it at pocketsphinx project [1] and I updated it in the patch. Please, let me know if now it's ok. [1] https://github.com/cmusphinx/sphinxbase/commit/93d35600dcdf612a17b377b16db434dab30d8137
Attachment #8500214 - Flags: review?(stephouillon)
Flags: sec-review?(stephouillon)
Comment on attachment 8500214 [details] [diff] [review] Integrate pocketsphinx sources, and its own moz.build Review of attachment 8500214 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/build/moz.build @@ +70,5 @@ > '/extensions/cookie', > '/js/xpconnect/loader', > '/js/xpconnect/src', > + '/media/pocketsphinx', > + '/media/sphinxbase', That can probably move under the CONFIG['MOZ_WEBSPEECH'] test. Although, is there any code in layout/build that uses headers from there? ::: media/sphinxbase/moz.build @@ +3,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +EXPORTS.sphinxbase += [ You're adding /media/sphinxbase and /media/pocketsphinx to LOCAL_INCLUDES in various places. Do you actually need the sphinxbase and pocketsphinx headers exported? (it's better not to export them if that can be avoided) ::: toolkit/content/license.html @@ +4072,5 @@ > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + * > + * ==================================================================== > + * > + */ Since both are the same license, it would seem better to add it only once, but better to ask Gerv.
Attachment #8500214 - Flags: review?(mh+mozilla)
Attachment #8500214 - Flags: review?(gerv)
Attachment #8500214 - Flags: feedback+
Comment on attachment 8500214 [details] [diff] [review] Integrate pocketsphinx sources, and its own moz.build Review of attachment 8500214 [details] [diff] [review]: ----------------------------------------------------------------- Yes, only once, please. Gerv
Attachment #8500214 - Flags: review?(gerv) → review-
Attachment #8500214 - Attachment is obsolete: true
Attachment #8500214 - Flags: review?(bugs)
Attachment #8503836 - Flags: review?(mh+mozilla)
Attachment #8503836 - Flags: review?(gerv)
Attachment #8503836 - Flags: review?(bugs)
(In reply to Gervase Markham [:gerv] from comment #18) > Comment on attachment 8500214 [details] [diff] [review] > Integrate pocketsphinx sources, and its own moz.build > > Review of attachment 8500214 [details] [diff] [review]: > ----------------------------------------------------------------- > > Yes, only once, please. > > Gerv Ok, I fixed and submit a new patch
(In reply to Mike Hommey [:glandium] from comment #17) > Comment on attachment 8500214 [details] [diff] [review] > Integrate pocketsphinx sources, and its own moz.build > > Review of attachment 8500214 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/build/moz.build > @@ +70,5 @@ > > '/extensions/cookie', > > '/js/xpconnect/loader', > > '/js/xpconnect/src', > > + '/media/pocketsphinx', > > + '/media/sphinxbase', > > That can probably move under the CONFIG['MOZ_WEBSPEECH'] test. Although, is > there any code in layout/build that uses headers from there? > Ok, I fixed. I tried to remove them and compile, but when I apply the patch in bug #1051148, fails to build. > ::: media/sphinxbase/moz.build > @@ +3,5 @@ > > +# This Source Code Form is subject to the terms of the Mozilla Public > > +# License, v. 2.0. If a copy of the MPL was not distributed with this > > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > > + > > +EXPORTS.sphinxbase += [ > > You're adding /media/sphinxbase and /media/pocketsphinx to LOCAL_INCLUDES in > various places. Do you actually need the sphinxbase and pocketsphinx headers > exported? (it's better not to export them if that can be avoided) > I tried to remove these exports and despite that builds, when I apply the patch in bug #1051148, fails to build. So seems these exports are required when we use pocketsphinx in a SpeechRecogntionService. > ::: toolkit/content/license.html > @@ +4072,5 @@ > > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > > + * > > + * ==================================================================== > > + * > > + */ > > Since both are the same license, it would seem better to add it only once, > but better to ask Gerv. Ok, I fixed this
Flags: sec-review?(stephouillon) → sec-review+
Attachment #8503836 - Flags: review?(mh+mozilla)
Cancelling glandium since he already feedback+
Comment on attachment 8503836 [details] [diff] [review] 0001-Bug-1051146-Integrate-pocketsphinx-sources-and-its-o.patch Review of attachment 8503836 [details] [diff] [review]: ----------------------------------------------------------------- license.html still contains two copies of the licence. Gerv
Attachment #8503836 - Flags: review?(gerv) → review-
Attachment #8503836 - Attachment is obsolete: true
Attachment #8503836 - Flags: review?(bugs)
Attachment #8506277 - Flags: review?(gerv)
Attachment #8506277 - Flags: review?(bugs)
(In reply to Gervase Markham [:gerv] from comment #23) > Comment on attachment 8503836 [details] [diff] [review] > 0001-Bug-1051146-Integrate-pocketsphinx-sources-and-its-o.patch > > Review of attachment 8503836 [details] [diff] [review]: > ----------------------------------------------------------------- > > license.html still contains two copies of the licence. > > Gerv Sorry, just fixed it
Comment on attachment 8506277 [details] [diff] [review] Integrate pocketsphinx sources, and its own moz.build on build system Review of attachment 8506277 [details] [diff] [review]: ----------------------------------------------------------------- You still have two entries in the table of contents. More haste, less speed :-) Gerv
Attachment #8506277 - Flags: review?(gerv) → review-
Attachment #8506277 - Attachment is obsolete: true
Attachment #8506277 - Flags: review?(bugs)
Attachment #8507172 - Flags: review?(gerv)
Attachment #8507172 - Flags: review?(bugs)
(In reply to Gervase Markham [:gerv] from comment #26) > Comment on attachment 8506277 [details] [diff] [review] > Integrate pocketsphinx sources, and its own moz.build on build system > > Review of attachment 8506277 [details] [diff] [review]: > ----------------------------------------------------------------- > > You still have two entries in the table of contents. More haste, less speed > :-) > > Gerv You are right, but I left it intentionally, since sphinxbase and pocketsphinx are different libs, but I kept only pocketsphinx
Comment on attachment 8507172 [details] [diff] [review] Integrate pocketsphinx sources, and its own moz.build on build system Review of attachment 8507172 [details] [diff] [review]: ----------------------------------------------------------------- Remove the trailing blank line in the <pre> with the actual license block. Then r=gerv. Gerv
Attachment #8507172 - Flags: review?(gerv) → review+
Attachment #8507172 - Attachment is obsolete: true
Attachment #8507172 - Flags: review?(bugs)
Attachment #8510694 - Flags: review?(bugs)
Comment on attachment 8510694 [details] [diff] [review] 0001-Bug-1051146-Integrate-pocketsphinx-sources-and-its-o.patch Is it always the case that webspeech requires Sphinx? If not, shouldn't this be behind its own build flag? What's the impact on the final compiled size by adding this code? I understand that the (eventually downloadable) models are the bulk of the size, but if the feature itself is large, I would opt to disable the code at build time until a download and management mechanism is available for Fennec. There's little point in shipping code in multiple releases without the necessary data, particularly if this has more than a few hundred KB of impact. APK size is a huge limiting factor to our success on Android, so platform features that have a large impact on this don't get an automatic pass. Marking sr? until we clarify what the impact will be, and come up with a mitigation if necessary.
Attachment #8510694 - Flags: superreview?(rnewman)
(In reply to Richard Newman [:rnewman] from comment #31) > Comment on attachment 8510694 [details] [diff] [review] > 0001-Bug-1051146-Integrate-pocketsphinx-sources-and-its-o.patch > > Is it always the case that webspeech requires Sphinx? No. We want to be able to just use the test service, and possibly support other backends. (like the ones provided by the platform) > If not, shouldn't this be behind its own build flag? Yes, there should be separate flag for pocketsphinx
(And I've been waiting for other pocketspinx patches to be updated before looking at this one.)
Attachment #8510694 - Flags: review?(bugs)
(Removed this from my review queue while waiting for the other patches to be updated.)
Comment on attachment 8510694 [details] [diff] [review] 0001-Bug-1051146-Integrate-pocketsphinx-sources-and-its-o.patch Likewise, but leaving a flag set as a reminder.
Attachment #8510694 - Flags: superreview?(rnewman) → superreview-
Dear Olli/Andre and all, This is the critical bugs for Voice Input and targeted to be demoed in MWC2015. Please take times and make sure it won't be a blocker forward. Paste the current/latest discussion thread from related email thread Andre: About https://bugzilla.mozilla.org/show_bug.cgi?id=1051146#c11 we do not have patches yet. I don't even know how/where to start this, since involves an UI panel setup, servers and etc... Olli: Maybe we could land some language models to m-c, but not package them by default. Like all of this, models could be behind some build flag which would let people to try out this stuff on their local builds while the implementation is improving. But we need the build flags working. Please update the progress accordingly and move forward to make it done. Thank you so much! Aaron Wu Engineering Project Manager, Mobile Device Mozilla Corporation
I think we should figure out how to get the code landed without the models at first, to unblock this work, letting users turn it on with a pref and download the models manually. We can punt figuring out how to deal with downloading the models to a followup bug.
Whiteboard: [webspeechapi]
Assignee: anatal → kdavis
Depends on: 1162507
This is the part 1 of 5 for this bug. This patch that introduces the B2G specific build flags (all enabled initially): * MOZ_WEBSPEECH - Enables/Disables the STT API * MOZ_WEBSPEECH_MODELS - Enables/Disables the model installations * MOZ_WEBSPEECH_POCKETSPHINX - Compiles/Doesn't Compile Pocketsphinx, Sphinxbase, and relevant XPCOM models The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f8a8598fb48
Attachment #8510694 - Attachment is obsolete: true
Attachment #8604642 - Flags: review?(bugs)
Part 2 of 5: Patch that introduces the old Pocketsphinx Sphinxbase code. This is the part 2 of 5 for this bug. This patch introduces the old Pocketsphinx/Sphinxbase code which has already gone through a security review. The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f8a8598fb48
Attachment #8604729 - Flags: review?(bugs)
Part 3 of 5: Patch that adds the old English model. This is the part 3 of 5 for this bug. This patch adds the old English model. The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a74f4dc3f1d
Attachment #8605108 - Flags: review?(bugs)
Part 4 of 5: Patch that introduces the new Pocketsphinx Sphinxbase code. This is the part 4 of 5 for this bug. This patch introduces the new Pocketsphinx/Sphinxbase code which must go through a security review, see Bug 1162507 The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=e017ebeb660d
Attachment #8605120 - Flags: review?(bugs)
Part 5 of 5: Patch that adds the new English model. This is the part 5 of 5 for this bug. This patch adds the new English model. The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=9385c52980c7 Note: This patch leaves the various B2G Web Speech API flags enabled. They will be disabled after another series of patches which introduce the Web Speech API implementation.
Attachment #8605142 - Flags: review?(bugs)
(In reply to kdavis from comment #41) > Created attachment 8605120 [details] [diff] [review] > Part 4 of 5: Patch that introduces the new Pocketsphinx Sphinxbase > > Part 4 of 5: Patch that introduces the new Pocketsphinx Sphinxbase code. > > This is the part 4 of 5 for this bug. > > This patch introduces the new Pocketsphinx/Sphinxbase code which must go > through a security review, see Bug 1162507 > > The try for this patch is running here > https://treeherder.mozilla.org/#/jobs?repo=try&revision=e017ebeb660d A build error showed up that I didn't see locally. As a result, I need to suppress the error "control reaches end of non-void function." The code for this is currently going through try. I'll post the patch when it is done building.
Part 4 of 5: Patch that introduces the new Pocketsphinx Sphinxbase code. This is the part 4 of 5 for this bug. This patch introduces the new Pocketsphinx/Sphinxbase code which must go through a security review, see Bug 1162507 The attached patch makes the previous "Part 4 of 5" patch obsolete as it fixes a build problem that was exposed on some platforms. The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b49ac2b654c
Attachment #8605120 - Attachment is obsolete: true
Attachment #8605120 - Flags: review?(bugs)
Attachment #8605244 - Flags: review?(bugs)
Comment on attachment 8604642 [details] [diff] [review] Part 1 of 5: Introduces the B2G specific build flags, initially enabled. >+++ b/configure.in >@@ -3863,7 +3863,9 @@ MOZ_MEDIA_NAVIGATOR= > MOZ_OMX_PLUGIN= > MOZ_VPX= > MOZ_VPX_ERROR_CONCEALMENT= >-MOZ_WEBSPEECH=1 >+MOZ_WEBSPEECH= Hmm, so how do we test webspeech API by default? >+if test -n "$MOZ_WEBSPEECH" && test -n "$MOZ_WEBSPEECH_POCKETSPHINX"; then >+ MOZ_WEBSPEECH=1 >+else >+ MOZ_WEBSPEECH= >+fi We should not require MOZ_WEBSPEECH_POCKETSPHINX to enable Web Speech API (or do I now misunderstand something here?)
Attachment #8604642 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #45) > >-MOZ_WEBSPEECH=1 > >+MOZ_WEBSPEECH= > Hmm, so how do we test webspeech API by default? oh, perhaps we explicitly enable MOZ_WEBSPEECH by default for testing
(In reply to Olli Pettay [:smaug] from comment #45) > Comment on attachment 8604642 [details] [diff] [review] > > We should not require MOZ_WEBSPEECH_POCKETSPHINX to enable Web Speech API > (or do I now misunderstand something here?) My thought, I may be wrong, was that having the API without the pocketsphinx implementation, which is currently the only implementation, wouldn't be useful.
But we have all the Speech API tests using the testing backend.
(In reply to Olli Pettay [:smaug] from comment #48) > But we have all the Speech API tests using the testing backend. So your suggestion is that MOZ_WEBSPEECH=1 MOZ_WEBSPEECH_POCKETSPHINX= use the testing backend?
I think so, though, do we want some flag to enable the testing service? Looks like we currently always build the fake service http://mxr.mozilla.org/mozilla-central/source/dom/media/webspeech/recognition/moz.build
(In reply to Olli Pettay [:smaug] from comment #50) > I think so, though, do we want some flag to enable the testing service? I see no reason not to. But, unfortunately, the synth fake service showed up *after* I made this first patch with the build flags.
I'll introduce new flags for the testing service and revise the patches accordingly.
Part 1 of 5: Introduces the B2G specific build flags, initially enabled. This is the part 1 of 5 for this bug. This patch that introduces the B2G specific build flags (all enabled initially): * MOZ_WEBSPEECH - Enables/Disables the STT API * MOZ_WEBSPEECH_MODELS - Enables/Disables the model installations * MOZ_WEBSPEECH_POCKETSPHINX - Compiles/Doesn't Compile Pocketsphinx, Sphinxbase, and relevant XPCOM models * MOZ_WEBSPEECH_TEST_BACKEND - Compiles/Doesn't Compile testing backend The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=c90547b4de76
Attachment #8604642 - Attachment is obsolete: true
Attachment #8607515 - Flags: review?(bugs)
Part 2 of 5: Patch that introduces the old Pocketsphinx Sphinxbase code. This is the part 2 of 5 for this bug. This patch introduces the old Pocketsphinx/Sphinxbase code which has already gone through a security review. The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=f64b599ebc1d
Attachment #8604729 - Attachment is obsolete: true
Attachment #8604729 - Flags: review?(bugs)
Attachment #8607516 - Flags: review?(bugs)
Part 3 of 5: Patch that adds the old English model. This is the part 3 of 5 for this bug. This patch adds the old English model. The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d506d5e60d3
Attachment #8605108 - Attachment is obsolete: true
Attachment #8605108 - Flags: review?(bugs)
Attachment #8607517 - Flags: review?(bugs)
Part 4 of 5: Patch that introduces the new Pocketsphinx Sphinxbase code. This is the part 4 of 5 for this bug. This patch introduces the new Pocketsphinx/Sphinxbase code which must go through a security review, see Bug 1162507 The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ee964e86485
Attachment #8605244 - Attachment is obsolete: true
Attachment #8605244 - Flags: review?(bugs)
Attachment #8607518 - Flags: review?(bugs)
Part 5 of 5: Patch that adds the new English model. This is the part 5 of 5 for this bug. This patch adds the new English model. The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae5f8e257c43 Note: This patch leaves the various B2G Web Speech API flags enabled. They will be disabled after another series of patches which introduce the Web Speech API implementation.
Attachment #8605142 - Attachment is obsolete: true
Attachment #8605142 - Flags: review?(bugs)
Attachment #8607519 - Flags: review?(bugs)
Comment on attachment 8607515 [details] [diff] [review] Part 1 of 5: Introduces the B2G specific build flags, initially enabled So I'm not seeing any webspeech tests being run on tryserver. In mozilla-inbound they are being run as part of mochitest-3
Attachment #8607515 - Flags: review?(bugs) → review-
Comment on attachment 8607516 [details] [diff] [review] Part 2 of 5: Patch that introduces the old Pocketsphinx Sphinxbase code Review of attachment 8607516 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/pocketsphinx/moz.build @@ +3,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +if CONFIG['MOZ_WEBSPEECH_POCKETSPHINX']: I would have expected this to be in the parent moz.build for the sake of clarity; maybe consult a build peer about the right style? ::: media/sphinxbase/moz.build @@ +3,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +if CONFIG['MOZ_WEBSPEECH_POCKETSPHINX']: Same.
Comment on attachment 8607516 [details] [diff] [review] Part 2 of 5: Patch that introduces the old Pocketsphinx Sphinxbase code Do we actually need EXPORTS.pocketsphinx? Or just export pocketsphinx.h or pocketsphinx_export.h', but at least not all the stuff in src/* Same with EXPORTS.sphinxbase build system changes need build peer review. I think gerv should review the change to toolkit/content/license.html (need to get this landed in some form)
Attachment #8607516 - Flags: review?(bugs) → review+
Comment on attachment 8607518 [details] [diff] [review] Part 4 of 5: Patch that introduces the new Pocketsphinx Sphinxbase code I don't understand the comment in media/pocketsphinx/update.sh "and applies any local patches we're carrying." It doesn't apply any patches. Just copies files. And what if either of the directories have different files? Same with media/sphinxbase/update.sh Please don't have those files in this patch, and perhaps then separate patch for those (with explanation why we want those behave as they behave).
Attachment #8607518 - Flags: review?(bugs) → review+
Comment on attachment 8607517 [details] Part 3 of 5: Patch that adds the old English model Oh, these model files are like this. Then no worth to add the old binary file ever. It would just increase repository size. So just the new one, and could you please have a separate patch for build system changes, and then separate patch which includes the new model files. That way build system peer would need to look at only some tiny patches.
Attachment #8607517 - Flags: review?(bugs) → review-
Attachment #8607519 - Flags: review?(bugs) → review-
Why a new patch for models is being reviwed if we already have a + on them in this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1065911
Flags: needinfo?(kdavis)
Flags: needinfo?(bugs)
Why a new patch for models is being reviewed if we already have a + on them in this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1065911
I honestly can't remember what all I've reviewed half a year ago ;)
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #58) > Comment on attachment 8607515 [details] [diff] [review] > Part 1 of 5: Introduces the B2G specific build flags, initially enabled > > So I'm not seeing any webspeech tests being run on tryserver. > In mozilla-inbound they are being run as part of mochitest-3 There are no tests that are run against just the pocketsphinx sources, which are the only things added in this patch. So, is your comment relevant?
Flags: needinfo?(kdavis)
(In reply to Richard Newman [:rnewman] from comment #59) > Comment on attachment 8607516 [details] [diff] [review] > Part 2 of 5: Patch that introduces the old Pocketsphinx Sphinxbase code > > Review of attachment 8607516 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: media/pocketsphinx/moz.build > @@ +3,5 @@ > > +# This Source Code Form is subject to the terms of the Mozilla Public > > +# License, v. 2.0. If a copy of the MPL was not distributed with this > > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > > + > > +if CONFIG['MOZ_WEBSPEECH_POCKETSPHINX']: > > I would have expected this to be in the parent moz.build for the sake of > clarity; maybe consult a build peer about the right style? > > ::: media/sphinxbase/moz.build > @@ +3,5 @@ > > +# This Source Code Form is subject to the terms of the Mozilla Public > > +# License, v. 2.0. If a copy of the MPL was not distributed with this > > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > > + > > +if CONFIG['MOZ_WEBSPEECH_POCKETSPHINX']: > > Same. Which parent moz.build? The directory gecko/media has no moz.build.
(In reply to Olli Pettay [:smaug] from comment #60) > Comment on attachment 8607516 [details] [diff] [review] > Part 2 of 5: Patch that introduces the old Pocketsphinx Sphinxbase code > > Do we actually need EXPORTS.pocketsphinx? Or just export pocketsphinx.h or > pocketsphinx_export.h', but at least not all the stuff in src/* > Same with EXPORTS.sphinxbase > > build system changes need build peer review. > > I think gerv should review the change to toolkit/content/license.html > > > (need to get this landed in some form) I thought the number of exports was a bit excessive, but this is what remained from the original integration and I thought there was a reason for it. Does anyone know why there are so many exports?
(In reply to Olli Pettay [:smaug] from comment #61) > Comment on attachment 8607518 [details] [diff] [review] > Part 4 of 5: Patch that introduces the new Pocketsphinx Sphinxbase code > > I don't understand the comment in media/pocketsphinx/update.sh > "and applies any local patches we're carrying." > It doesn't apply any patches. Just copies files. media/pocketsphinx/update.sh just copies files and applies the set of local patches which is this case is empty. media/sphinxbase/update.sh copies files and applies the set of local patches which is this case is not empty. > And what if either of the directories have different files? > > Same with media/sphinxbase/update.sh As far as I can tell it won't work. The initial structure, from whomever introduced these two libraries in gecko, changes the directory structure of the pocketsphinx library. So, the script needs to move things about and for this need to know specific files. > Please don't have those files in this patch, and perhaps then separate patch > for those (with explanation why we want those behave as they behave). I'll separate them into another patch.
(In reply to kdavis from comment #68) > I thought the number of exports was a bit excessive, but this is what > remained from the original integration and I thought there was a reason for > it. Does anyone know why there are so many exports? Not any reason afaik, I asked the same question in comment 17.
(In reply to Olli Pettay [:smaug] from comment #62) > Comment on attachment 8607517 [details] > Part 3 of 5: Patch that adds the old English model > > ...and could you please have a separate patch for build system changes, > and then separate patch which includes the new model files. That way > build system peer would need to look at only some tiny patches. OK
(In reply to kdavis from comment #69) > (In reply to Olli Pettay [:smaug] from comment #61) > > Comment on attachment 8607518 [details] [diff] [review] > > And what if either of the directories have different files? > > As far as I can tell it won't work. > Looking at the file structure I think I can get it to work with a generic copy that is ignorant of the file names. I will do this in the next revision. However, the effort put into making the update.sh script agnostic of the file names is seems to be defeated by the moz.build's in gecko/media/pocketsphinx and gecko/media/sphinxbase, as they require explicit file names. None-the-less, I'll try to update the update.sh to a generic copy that is ignorant of the file names.
So, as I understand it, the current desired splitting of patches for today is: 1. Patch introducing build flags, same as in previous first patch 2. Patch with new Pocketsphinx/Sphinxbase code 3. Patch with build system changes for new Pocketsphinx/Sphinxbase (Attempt to minimise the exports and place "if CONFIG['MOZ_WEBSPEECH_POCKETSPHINX’]” properly) 4. Patch with new Pocketsphinx English model 5. Patch with build system changes for Pocketsphinx English model 6. Patch with update scripts that are ignorant of Pocketsphinx/Sphinxbase file names Outstanding issues: 1. Where to put the "if CONFIG['MOZ_WEBSPEECH_POCKETSPHINX’]”, as there is no moz.build in gecko/media? 2. Which Pocketsphinx exports are really needed?
(In reply to kdavis from comment #73) > So, as I understand it, the current desired splitting of patches for today > is: > > 1. Patch introducing build flags, same as in previous first patch > 2. Patch with new Pocketsphinx/Sphinxbase code > 3. Patch with build system changes for new Pocketsphinx/Sphinxbase (Attempt > to minimise the exports and place "if CONFIG['MOZ_WEBSPEECH_POCKETSPHINX’]” > properly) > 4. Patch with new Pocketsphinx English model > 5. Patch with build system changes for Pocketsphinx English model > 6. Patch with update scripts that are ignorant of Pocketsphinx/Sphinxbase > file names That's fine for reviewing, but for landing, that seems like it would create broken intermediate state. > Outstanding issues: > 1. Where to put the "if CONFIG['MOZ_WEBSPEECH_POCKETSPHINX’]”, as there is > no moz.build in gecko/media? wherever you add DIRS += ... > 2. Which Pocketsphinx exports are really needed? Likely none. How many directories would need LOCAL_INCLUDES += ['/path/to/pocketsphinx'] if they aren't exported?
(In reply to Mike Hommey [:glandium] from comment #74) > (In reply to kdavis from comment #73) > That's fine for reviewing, but for landing, that seems like it would create > broken intermediate state. smaug says this is what he wants and he is the reviewer. > > Outstanding issues: > > 1. Where to put the "if CONFIG['MOZ_WEBSPEECH_POCKETSPHINX’]”, as there is > > no moz.build in gecko/media? > > wherever you add DIRS+= ... Yes, but where that is was the question. Oh, I just found where this is... config/external/moz.build It's a bit different from most other DIRS+= as it's not in the parent directory. > > 2. Which Pocketsphinx exports are really needed? > > Likely none. How many directories would need LOCAL_INCLUDES += > ['/path/to/pocketsphinx'] if they aren't exported? This is something I will have to see.
(In reply to Olli Pettay [:smaug] from comment #60) > Comment on attachment 8607516 [details] [diff] [review] > I think gerv should review the change to toolkit/content/license.html > > > (need to get this landed in some form) Wasn't his OK implicit in the silence after comment 29?
Flags: needinfo?(gerv)
(In reply to kdavis from comment #66) > (In reply to Olli Pettay [:smaug] from comment #58) > > Comment on attachment 8607515 [details] [diff] [review] > > Part 1 of 5: Introduces the B2G specific build flags, initially enabled > > > > So I'm not seeing any webspeech tests being run on tryserver. > > In mozilla-inbound they are being run as part of mochitest-3 > > There are no tests that are run against just the pocketsphinx sources, which > are the only things added in this patch. So, is your comment relevant? But we have existing tests for the Web Speech SpeechRecognition API, and normally those are run in mochitest-3. I didn't see those tests being run in your tryserver push.
(In reply to Olli Pettay [:smaug] from comment #77) > (In reply to kdavis from comment #66) > > (In reply to Olli Pettay [:smaug] from comment #58) > > > Comment on attachment 8607515 [details] [diff] [review] > > > Part 1 of 5: Introduces the B2G specific build flags, initially enabled > > > > > > So I'm not seeing any webspeech tests being run on tryserver. > > > In mozilla-inbound they are being run as part of mochitest-3 > > > > There are no tests that are run against just the pocketsphinx sources, which > > are the only things added in this patch. So, is your comment relevant? > But we have existing tests for the Web Speech SpeechRecognition API, and > normally those are run > in mochitest-3. I didn't see those tests being run in your tryserver push. M(3) is running there. Are the test only run against a non-B2G target?
(In reply to Mike Hommey [:glandium] from comment #74) > That's fine for reviewing, but for landing, that seems like it would create > broken intermediate state. Which is totally fine, if it makes reviewing significantly easier. (In reply to kdavis from comment #78) > > M(3) is running there. Are the test only run against a non-B2G target? possibly. I checked only linux-opt m-3. I saw webspeech stuff in normal mozilla-inbound mo-3 logs, but not in the tryserver push mo-3 logs.
(In reply to Olli Pettay [:smaug] from comment #79) > > M(3) is running there. Are the test only run against a non-B2G target? > possibly. I checked only linux-opt m-3. I saw webspeech stuff in normal > mozilla-inbound mo-3 logs, but not in the tryserver push mo-3 logs. As all the flags are only on for B2G, any non-B2G target(s) will not run the M(3) webspeech tests or if any non-B2G target(s) runs the M(3) webspeech tests, it will fail as MOZ_WEBSPEECH undefined. I guess the question is: Do B2G target(s) run the M(3) webspeech tests or are the M(3) webspeech tests switched off for B2G? I can look at this.
(In reply to kdavis from comment #80) > (In reply to Olli Pettay [:smaug] from comment #79) > > > M(3) is running there. Are the test only run against a non-B2G target? > > possibly. I checked only linux-opt m-3. I saw webspeech stuff in normal > > mozilla-inbound mo-3 logs, but not in the tryserver push mo-3 logs. > > As all the flags are only on for B2G MOZ_WEBSPEECH shouldn't be for b2g only. We have the API implementation everywhere. That is how we've got the testing so far (only API testing, no backend testing because we've lacked the backend) Makes sense?
(In reply to Olli Pettay [:smaug] from comment #81) > MOZ_WEBSPEECH shouldn't be for b2g only. We have the API implementation > everywhere. > That is how we've got the testing so far (only API testing, no backend > testing because we've lacked the backend) > Makes sense? Make sense, but, as I understood it from our last video conference, we are currently only trying to land on B2G. This measured goal makes landing achievable and limits the number of parties that have to check off on the installation location, library size, model size.... Later we will move beyond B2G. But, we should walk before we run. However, what we could do is land the Pocketsphinx implementation on B2G but keep the dummy implementation on non-B2G platforms? This could be a middle ground.
(In reply to Mike Hommey [:glandium] from comment #70) > (In reply to kdavis from comment #68) > > I thought the number of exports was a bit excessive, but this is what > > remained from the original integration and I thought there was a reason for > > it. Does anyone know why there are so many exports? > > Not any reason afaik, I asked the same question in comment 17. And I answered why in comment 21.
(In reply to kdavis from comment #82) > (In reply to Olli Pettay [:smaug] from comment #81) > > MOZ_WEBSPEECH shouldn't be for b2g only. We have the API implementation > > everywhere. > > That is how we've got the testing so far (only API testing, no backend > > testing because we've lacked the backend) > > Makes sense? > > Make sense, but, as I understood it from our last video conference, we are > currently only trying to land on B2G. Sure, but I certainly was expecting that we wouldn't break the existing setup, where the API is being tested on desktop. The *sphinx backend would be b2g only. > However, what we could do is land the Pocketsphinx implementation on B2G but > keep the dummy implementation on non-B2G platforms? This could be a middle > ground. Yes, that is what I've assumed all the time. Sorry if I wasn't clear before.
(In reply to Olli Pettay [:smaug] from comment #84) > The *sphinx backend would be b2g only. OK Sphinx backend for B2G fake backend for non-B2G.
(In reply to anatal from comment #83) > (In reply to Mike Hommey [:glandium] from comment #70) > > (In reply to kdavis from comment #68) > > > I thought the number of exports was a bit excessive, but this is what > > > remained from the original integration and I thought there was a reason for > > > it. Does anyone know why there are so many exports? > > > > Not any reason afaik, I asked the same question in comment 17. > > And I answered why in comment 21. Andre did you try to remove all exports? My guess would be that you can remove all except pocketsphinx.h. But, I need to test this guess.
(In reply to kdavis from comment #86) > My guess would be that you can remove all except pocketsphinx.h. But, I need > to test this guess. And/or test with all of the removed, and paste the resulting errors. (In reply to Olli Pettay [:smaug] from comment #79) > (In reply to Mike Hommey [:glandium] from comment #74) > > That's fine for reviewing, but for landing, that seems like it would create > > broken intermediate state. > Which is totally fine, if it makes reviewing significantly easier. I was implying that it doesn't have to land in the form in which it was reviewed.
(In reply to kdavis from comment #73) > So, as I understand it, the current desired splitting of patches for today > is: > > 1. Patch introducing build flags, same as in previous first patch > 2. Patch with new Pocketsphinx/Sphinxbase code > 3. Patch with build system changes for new Pocketsphinx/Sphinxbase (Attempt > to minimise the exports and place "if CONFIG['MOZ_WEBSPEECH_POCKETSPHINX’]” > properly) > 4. Patch with new Pocketsphinx English model > 5. Patch with build system changes for Pocketsphinx English model > 6. Patch with update scripts that are ignorant of Pocketsphinx/Sphinxbase > file names Actually, I will split patch 3 into two patches 3a. Patch with build system changes for new Pocketsphinx/Sphinxbase 3b. Patch with build system changes for the flag MOZ_WEBSPEECH_TEST_BACKEND as these are relatively orthogonal concerns.
Comment on attachment 8607516 [details] [diff] [review] Part 2 of 5: Patch that introduces the old Pocketsphinx Sphinxbase code This patch seems to add an entry to the TOC of license.html (out of alphabetical order - please fix that) but no corresponding license block. In general, if a bug has this many comments, you need to NI me to get my attention. Gerv
Flags: needinfo?(gerv)
Attachment #8607516 - Flags: review-
(In reply to Gervase Markham [:gerv] from comment #89) > Comment on attachment 8607516 [details] [diff] [review] > Part 2 of 5: Patch that introduces the old Pocketsphinx Sphinxbase code > > This patch seems to add an entry to the TOC of license.html (out of > alphabetical order - please fix that) but no corresponding license block. > > In general, if a bug has this many comments, you need to NI me to get my > attention. > > Gerv There will be a new round of patches coming and I'll NI you on the relevant patch.
Part 1 of 7: Introduces the B2G specific build flags, initially enabled This is the part 1 of 7 for this bug. This patch that introduces the B2G specific build flags (all enabled initially): * MOZ_WEBSPEECH - Enables/Disables the STT API * MOZ_WEBSPEECH_MODELS - Enables/Disables the model installations * MOZ_WEBSPEECH_POCKETSPHINX - Compiles/Doesn't Compile Pocketsphinx, Sphinxbase, and relevant XPCOM models * MOZ_WEBSPEECH_TEST_BACKEND - Compiles/Doesn't Compile testing backend The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=37c27d42e927
Attachment #8607515 - Attachment is obsolete: true
Attachment #8607516 - Attachment is obsolete: true
Attachment #8607517 - Attachment is obsolete: true
Attachment #8607518 - Attachment is obsolete: true
Attachment #8607519 - Attachment is obsolete: true
Attachment #8608731 - Flags: review?(bugs)
Part 2 of 7: Introduces the new Pocketsphinx and Sphinxbase code with no build integration This is the part 2 of 7 for this bug. This patch that introduces the new Pocketsphinx and Sphinxbase code with no build integration The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=c03199d3b75b
Flags: needinfo?(gerv)
Attachment #8608733 - Flags: review?(bugs)
Part 3 of 7: Introduces build system changes for the new Pocketsphinx and Sphinxbase code This is the part 3 of 7 for this bug. This patch that introduces the build system changes for the new Pocketsphinx and Sphinxbase code The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=a51100bec84e
Attachment #8608736 - Flags: review?(bugs)
Part 4 of 7: Introduces build system changes for the MOZ_WEBSPEECH_TEST_BACKEND flag This is the part 4 of 7 for this bug. This patch that introduces the build system changes for the MOZ_WEBSPEECH_TEST_BACKEND flag The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=01336871eb2c
Flags: needinfo?(eitan)
Attachment #8608737 - Flags: review?(bugs)
Part 5 of 7: Introduces new English model with no build integration This is the part 5 of 7 for this bug. This patch that introduces the new English model with no build integration The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7d5414b6de2
Attachment #8608740 - Flags: review?(bugs)
Comment on attachment 8608733 [details] [diff] [review] Part 2 of 7: Introduces the new Pocketsphinx and Sphinxbase code with no build integration + <p>This license applies files in directory + <span class="path">media/pocketsphinx/ and media/sphinxbase</span>.</p> _the_ director_ies_. Also, please make trailing slashes consistent. +/* ==================================================================== + * Copyright (c) 1999-2014 Carnegie Mellon University. All rights Please don't include comment characters or ASCII <hr>s. Gerv
Flags: needinfo?(gerv)
Part 6 of 7: Introduces build integration of new English model This is the part 6 of 7 for this bug. This patch that introduces the build integration of new English model The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=49c967bb9df3
Attachment #8608741 - Flags: review?(bugs)
Part 7 of 7: Introduces update scripts for Pocketsphinx and Sphinxbase code This is the part 7 of 7 for this bug. This patch that introduces the update scripts for Pocketsphinx and Sphinxbase code The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=d41aed8240ac
Attachment #8608743 - Flags: review?(bugs)
Part 2 of 7: Introduces the new Pocketsphinx and Sphinxbase code with no build integration This is the part 2 of 7 for this bug. This patch that introduces the new Pocketsphinx and Sphinxbase code with no build integration and fixes license formatting from previous patch. The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5069298f53e
Attachment #8608733 - Attachment is obsolete: true
Attachment #8608733 - Flags: review?(bugs)
Flags: needinfo?(gerv)
Attachment #8608770 - Flags: review?(bugs)
Comment on attachment 8608770 [details] [diff] [review] Part 2 of 7: Introduces the new Pocketsphinx and Sphinxbase code with no build integration + <p>This license applies files in the directories + <span class="path">media/pocketsphinx and media/sphinxbase</span>.</p> -> <p>This license applies to files in the directories <span class="path">media/pocketsphinx/</span> and <span class="path">media/sphinxbase/</span>.</p> Gerv
Flags: needinfo?(gerv)
Attachment #8608770 - Flags: review+
Comment on attachment 8608731 [details] [diff] [review] Part 1 of 7: Introduces the B2G specific build flags, initially enabled Review of attachment 8608731 [details] [diff] [review]: ----------------------------------------------------------------- I like the idea of MOZ_WEBSPEECH_TEST_BACKEND, but I don't understand how it is used. It would be great if it is always enabled for continuous integration tests, but then disabled for actual release builds. I don't know if there is a precedent for that.. From what I could see in this patch, it is always enabled. ::: b2g/confvars.sh @@ +32,5 @@ > > +MOZ_WEBSPEECH=1 > +MOZ_WEBSPEECH_MODELS=1 > +MOZ_WEBSPEECH_POCKETSPHINX=1 > +MOZ_WEBSPEECH_TEST_BACKEND=1 Why does TEST_BACKEND need to be explicitly enabled here? ::: configure.in @@ +3850,5 @@ > MOZ_VPX_ERROR_CONCEALMENT= > MOZ_WEBSPEECH=1 > +MOZ_WEBSPEECH_MODELS= > +MOZ_WEBSPEECH_POCKETSPHINX= > +MOZ_WEBSPEECH_TEST_BACKEND=1 If it is enabled here.. @@ +5139,5 @@ > +dnl ======================================================== > +MOZ_ARG_DISABLE_BOOL(webspeechtestbackend, > +[ --disable-webspeechtestbackend Disable support for HTML Speech API Test Backend], > + MOZ_WEBSPEECH_TEST_BACKEND=, > + MOZ_WEBSPEECH_TEST_BACKEND=1) Are we going to somehow disable the test backend in release builds?
Flags: needinfo?(eitan)
(In reply to Eitan Isaacson [:eeejay] from comment #101) > Comment on attachment 8608731 [details] [diff] [review] > Part 1 of 7: Introduces the B2G specific build flags, initially enabled > > Review of attachment 8608731 [details] [diff] [review]: > ----------------------------------------------------------------- > > I like the idea of MOZ_WEBSPEECH_TEST_BACKEND, but I don't understand how it > is used. It's used to turn off/on the test backend, and it is used in patch 4 of 7. Can you please take a look at that patch? I NI'ed you on it to see if it is used properly in your code. > It would be great if it is always enabled for continuous > integration tests, but then disabled for actual release builds. For now it's simply going to be on for CI, as in the current nightly. I also assume it should be off for release. > From what I could see in this patch, it is always enabled. > > ::: b2g/confvars.sh > @@ +32,5 @@ > > > > +MOZ_WEBSPEECH=1 > > +MOZ_WEBSPEECH_MODELS=1 > > +MOZ_WEBSPEECH_POCKETSPHINX=1 > > +MOZ_WEBSPEECH_TEST_BACKEND=1 > > Why does TEST_BACKEND need to be explicitly enabled here? > > ::: configure.in > @@ +3850,5 @@ > > MOZ_VPX_ERROR_CONCEALMENT= > > MOZ_WEBSPEECH=1 > > +MOZ_WEBSPEECH_MODELS= > > +MOZ_WEBSPEECH_POCKETSPHINX= > > +MOZ_WEBSPEECH_TEST_BACKEND=1 > > If it is enabled here.. > > @@ +5139,5 @@ > > +dnl ======================================================== > > +MOZ_ARG_DISABLE_BOOL(webspeechtestbackend, > > +[ --disable-webspeechtestbackend Disable support for HTML Speech API Test Backend], > > + MOZ_WEBSPEECH_TEST_BACKEND=, > > + MOZ_WEBSPEECH_TEST_BACKEND=1) It doesn't need to be enabled "twice" as it were, but this is consistent with other webspeech flags. I can remove this if it's desired. > Are we going to somehow disable the test backend in release builds? I think it would make sense, but we can wait on the decision for now.
Flags: needinfo?(eitan)
(In reply to Gervase Markham [:gerv] from comment #100) > Comment on attachment 8608770 [details] [diff] [review] > Part 2 of 7: Introduces the new Pocketsphinx and Sphinxbase code with no > build integration > > + <p>This license applies files in the directories > + <span class="path">media/pocketsphinx and media/sphinxbase</span>.</p> > > -> > > <p>This license applies to files in the directories <span > class="path">media/pocketsphinx/</span> and <span > class="path">media/sphinxbase/</span>.</p> > > Gerv I will put this in, but note the trailing "/"'s of the directory names are inconsistent with, for example, <p>This license applies to certain files in the directories <span class="path">intl/icu</span> and <span class="path">js/src/vm</span>. </p> and other directory names in this file.
Flags: needinfo?(gerv)
Part 2 of 7: Introduces the new Pocketsphinx and Sphinxbase code with no build integration This is the part 2 of 7 for this bug. This patch that introduces the new Pocketsphinx and Sphinxbase code with no build integration. This patch also adds in the suggestions of Comment 100 to the previous patch. The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=fec91aa9e244
Attachment #8608770 - Attachment is obsolete: true
Attachment #8608770 - Flags: review?(bugs)
Attachment #8609191 - Flags: review?(bugs)
(In reply to kdavis from comment #103) > I will put this in, but note the trailing "/"'s of the directory names are > inconsistent with, for example, > > <p>This license applies to certain files in the directories > <span class="path">intl/icu</span> and <span > class="path">js/src/vm</span>. > </p> > > and other directory names in this file. I just wanted them consistent with each other :-) Both slashes added or both removed are both fine with me. Gerv
Flags: needinfo?(gerv)
Attachment #8608740 - Flags: review?(bugs) → review+
Comment on attachment 8608741 [details] [diff] [review] Part 6 of 7: Introduces build integration of new English model I don't really know if we need Makefile.in here, or could moz.build work. And build system changes need build peer review.
Attachment #8608741 - Flags: review?(bugs) → feedback+
Comment on attachment 8608743 [details] [diff] [review] Part 7 of 7: Introduces update scripts for Pocketsphinx and Sphinxbase code "applies any local patches we're carrying." sounds a bit odd to me. This is just doing copying from place x to place y, and doesn't apply any patches anywhere. So perhaps drop the comment.
Attachment #8608743 - Flags: review?(bugs) → review+
Comment on attachment 8608731 [details] [diff] [review] Part 1 of 7: Introduces the B2G specific build flags, initially enabled It is a bit silly to review this since most of the patch is being changed in a different bug. You might want to just merge this and that other one and upload the patch in this bug.
Attachment #8608731 - Flags: review?(bugs) → review+
Comment on attachment 8608736 [details] [diff] [review] Part 3 of 7: Introduces build system changes for the new Pocketsphinx and Sphinxbase code Something wrong here. Why about:license#pocketsphinx">Pocketsphinx License is there twice?
Attachment #8608736 - Flags: review?(bugs) → review-
Attachment #8608737 - Flags: review?(bugs) → review+
Attachment #8609191 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #109) > Comment on attachment 8608736 [details] [diff] [review] > Part 3 of 7: Introduces build system changes for the new Pocketsphinx and > Sphinxbase code > > Something wrong here. Why about:license#pocketsphinx">Pocketsphinx License > is there twice? Good catch. I think it's left from a typo in adding in Gervase's changes. I'll remove the first one.
(In reply to Olli Pettay [:smaug] from comment #107) > Comment on attachment 8608743 [details] [diff] [review] > Part 7 of 7: Introduces update scripts for Pocketsphinx and Sphinxbase code > > "applies any local patches we're carrying." sounds a bit odd to me. > This is just doing copying from place x to place y, and doesn't apply any > patches anywhere. So perhaps drop the comment. I can remove the comment
Attachment #8608737 - Flags: feedback+
I'm fine with this change, as long as the synth tests are still enabled and pass..
Flags: needinfo?(eitan)
(In reply to Olli Pettay [:smaug] from comment #106) > Comment on attachment 8608741 [details] [diff] [review] > Part 6 of 7: Introduces build integration of new English model > > I don't really know if we need Makefile.in here, or could moz.build work. As you canceled the review, do you have a constructive suggestion as to what to change?
(In reply to Eitan Isaacson [:eeejay] from comment #112) > I'm fine with this change, as long as the synth tests are still enabled and > pass.. They are and they do. :-)
Bug 1073399 is one thing to keep in mind.
and/or bug 1093827. kdavis, could you ask b2g folks about those. Do we still have the issue that any new code may cause updates to fail? (OTA hasn't worked for ages on my Flame - I do shallow_flash manually)
Part 3 of 7: Introduces build system changes for the new Pocketsphinx and Sphinxbase code This is the part 3 of 7 for this bug. This fixes a minor "mergeo" in the last patch. This patch that introduces the build system changes for the new Pocketsphinx and Sphinxbase code. This fixes a minor "mergeo" in the last patch. The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=a51100bec84e
Attachment #8608736 - Attachment is obsolete: true
Attachment #8610475 - Flags: review+
Part 7 of 7: Introduces update scripts for Pocketsphinx and Sphinxbase code This is the part 7 of 7 for this bug. This patch changes a comment in the previous patch. This patch that introduces the update scripts for Pocketsphinx and Sphinxbase code. This patch changes a comment in the previous patch. The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=d41aed8240ac
Attachment #8608743 - Attachment is obsolete: true
Attachment #8610478 - Flags: review+
Comment on attachment 8610475 [details] [diff] [review] Part 3 of 7: Introduces build system changes for the new Pocketsphinx and Sphinxbase code Flagging gps for review of build changes
Attachment #8610475 - Flags: superreview?(gps)
Comment on attachment 8608737 [details] [diff] [review] Part 4 of 7: Introduces build system changes for the MOZ_WEBSPEECH_TEST_BACKEND flag Flagging gps for review of build changes
Attachment #8608737 - Flags: superreview?(gps)
Comment on attachment 8608741 [details] [diff] [review] Part 6 of 7: Introduces build integration of new English model Flagging gps for review of build changes
Attachment #8608741 - Flags: superreview?(gps)
Comment on attachment 8610478 [details] [diff] [review] Part 7 of 7: Introduces update scripts for Pocketsphinx and Sphinxbase code Flagging gps for review of build changes
Attachment #8610478 - Flags: superreview?(gps)
Comment on attachment 8610475 [details] [diff] [review] Part 3 of 7: Introduces build system changes for the new Pocketsphinx and Sphinxbase code Review of attachment 8610475 [details] [diff] [review]: ----------------------------------------------------------------- This isn't a superreview, merely a build peer review. ::: media/sphinxbase/moz.build @@ +68,5 @@ > +# Suppress warnings in third-party code. > +if CONFIG['GNU_CC']: > + CFLAGS += [ > + '-Wno-sign-compare', > + '-D HAVE_CONFIG_H', This should be "DEFINES['HAVE_CONFIG_H'] = True"
Attachment #8610475 - Flags: superreview?(gps) → review+
Attachment #8608737 - Flags: superreview?(gps) → review+
Comment on attachment 8608741 [details] [diff] [review] Part 6 of 7: Introduces build integration of new English model Review of attachment 8608741 [details] [diff] [review]: ----------------------------------------------------------------- Is there no dom/media/webspeech/recognition/models/moz.build file? If not, this won't work because we require a moz.build file (whereas Makefile.in files are optional). If there is no dom/media/webspeech/recognition/models/moz.build file, please don't create one: just roll the contents of the Makefile.in into dom/media/webspeech/recognition/Makefile.in.
Attachment #8608741 - Flags: superreview?(gps)
Comment on attachment 8610478 [details] [diff] [review] Part 7 of 7: Introduces update scripts for Pocketsphinx and Sphinxbase code Review of attachment 8610478 [details] [diff] [review]: ----------------------------------------------------------------- You don't need my review on this.
Attachment #8610478 - Flags: superreview?(gps)
(In reply to Gregory Szorc [:gps] from comment #124) > Comment on attachment 8608741 [details] [diff] [review] > Part 6 of 7: Introduces build integration of new English model > > Review of attachment 8608741 [details] [diff] [review]: > ----------------------------------------------------------------- > > Is there no dom/media/webspeech/recognition/models/moz.build file? If not, > this won't work because we require a moz.build file (whereas Makefile.in > files are optional). > > If there is no dom/media/webspeech/recognition/models/moz.build file, please > don't create one: just roll the contents of the Makefile.in into > dom/media/webspeech/recognition/Makefile.in. Is there an example on how to do this? No code in gecko that does an "INSTALL_TARGETS+=" does so within a moz.build file.
Flags: needinfo?(gps)
(In reply to Gregory Szorc [:gps] from comment #124) > Comment on attachment 8608741 [details] [diff] [review] > Part 6 of 7: Introduces build integration of new English model > > Review of attachment 8608741 [details] [diff] [review]: > ----------------------------------------------------------------- > > Is there no dom/media/webspeech/recognition/models/moz.build file? If not, > this won't work because we require a moz.build file (whereas Makefile.in > files are optional). > > If there is no dom/media/webspeech/recognition/models/moz.build file, please > don't create one: just roll the contents of the Makefile.in into > dom/media/webspeech/recognition/Makefile.in. As Bug 853594 is still open, it seems the current Makefile.in solution is the preferred solution?
(In reply to kdavis from comment #126) > (In reply to Gregory Szorc [:gps] from comment #124) > > Is there no dom/media/webspeech/recognition/models/moz.build file? If not, > > this won't work because we require a moz.build file (whereas Makefile.in > > files are optional). > > > > If there is no dom/media/webspeech/recognition/models/moz.build file, please > > don't create one: just roll the contents of the Makefile.in into > > dom/media/webspeech/recognition/Makefile.in. > > Is there an example on how to do this? No code in gecko that does an > "INSTALL_TARGETS+=" does so within a moz.build file. You are correct: moz.build does not have support for INSTALL_TARGETS. But I asked to fold the changes into a Makefile.in, not a moz.build file. INSTALL_TARGETS can reference paths in subdirectories. I ask this because every moz.build/Makefile.in file has a small cost associated with it. We try to minimize the number of moz.build/Makefile.in files to make the build system as fast as possible.
Flags: needinfo?(gps)
(In reply to Gregory Szorc [:gps] from comment #128) > (In reply to kdavis from comment #126) > > (In reply to Gregory Szorc [:gps] from comment #124) > > > Is there no dom/media/webspeech/recognition/models/moz.build file? If not, > > > this won't work because we require a moz.build file (whereas Makefile.in > > > files are optional). > > > > > > If there is no dom/media/webspeech/recognition/models/moz.build file, please > > > don't create one: just roll the contents of the Makefile.in into > > > dom/media/webspeech/recognition/Makefile.in. > > > > Is there an example on how to do this? No code in gecko that does an > > "INSTALL_TARGETS+=" does so within a moz.build file. > > You are correct: moz.build does not have support for INSTALL_TARGETS. > > But I asked to fold the changes into a Makefile.in, not a moz.build file. > INSTALL_TARGETS can reference paths in subdirectories. > > I ask this because every moz.build/Makefile.in file has a small cost > associated with it. We try to minimize the number of moz.build/Makefile.in > files to make the build system as fast as possible. My mistake. However, there is no dom/media/webspeech/recognition/Makefile.in there is only a dom/media/webspeech/recognition/moz.build
Please add a dom/media/webspeech/recognition/Makefile.in instead of dom/media/webspeech/recognition/models/Makefile.in.
Part 3 of 7: Introduces build system changes for the new Pocketsphinx and Sphinxbase code This is the part 3 of 7 for this bug. This patch that introduces the build system changes for the new Pocketsphinx and Sphinxbase code. This addresses the build peer review suggestion of Comment 123 The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ac53d4d75b8
Attachment #8610475 - Attachment is obsolete: true
Attachment #8612096 - Flags: review+
Part 6 of 7: Introduces build integration of new English model This is the part 6 of 7 for this bug. This patch that introduces the build integration of new English model. This new patch addresses the build peer review suggestion Comment 124. The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=cff8d344b249
Attachment #8608741 - Attachment is obsolete: true
Attachment #8612291 - Flags: review?(bugs)
Whiteboard: [webspeechapi] → [webspeechapi][systemsfe]
Target Milestone: --- → 2.2 S14 (12june)
Comment on attachment 8612291 [details] [diff] [review] Part 6 of 7: Introduces build integration of new English model This is purely a build system change, so a build peer review would be good. (I don't dare to r+ new Makefile.in files) Why only b2g/installer/package-manifest.in? What if someone wants to test this stuff on desktop and defines MOZ_WEBSPEECH_MODELS locally.
Attachment #8612291 - Flags: review?(bugs) → feedback+
(In reply to Olli Pettay [:smaug] (reviewing overload, no new review requests before June 1, pretty please) from comment #133) > Comment on attachment 8612291 [details] [diff] [review] > Part 6 of 7: Introduces build integration of new English model > > This is purely a build system change, so a build peer review would be good. > (I don't dare to r+ new Makefile.in files) > This change was a result of the build peer review. I thought as you review+ other build changes (See for example Part 4 of 7) you would also review+ this too. If not, Ill assume the build peer review+ is sufficient. > Why only b2g/installer/package-manifest.in? What if someone wants to test > this stuff on desktop and defines MOZ_WEBSPEECH_MODELS locally. As far as I know, we are not supporting desktop in this initial version.
Attachment #8612291 - Flags: review?(gps)
"supporting". Well, don't we still want to make it easy to test this all on desktop?
(In reply to Olli Pettay [:smaug] (reviewing overload, no new review requests before June 1, pretty please) from comment #135) > "supporting". Well, don't we still want to make it easy to test this all on > desktop? I think it wouldn't be too hard to add, but by the same token I'm inclined to stick to our limited set of goals. After Whistler, we can expand to desktop.
Attachment #8612291 - Flags: review?(gps) → review+
Keywords: checkin-needed
Depends on: 1171082
This patch works around the Windows bustage identified in bug 1171082. That said, I never got a chance to push it due to new rooting hazards introduced by these patches. Backing out. https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux64-br-haz/20150603072142/hazards.txt.gz
Attachment #8614772 - Flags: review+
Actually, those failures are from bug 1051148, so I'll backout just that and leave this in.
Comment on attachment 8614772 [details] [diff] [review] Disable on Windows to work around bustage Forgot to mention this got IRC r+ from gps.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #140) > Actually, those failures are from bug 1051148, so I'll backout just that and > leave this in. Just to clarify... *The failure is introduced by Part 2 of 7 and Part 3 of 7 of these patches. *The hazard (unrooted 'init' live across GC call) is from Bug 1051148 The failure is now worked around by Attachment 8614772 [details] [diff] and will be tracked by Bug 1171082; the hazard requires some code change in the patches of Bug 1051148.
Depends on: 1171291
No longer depends on: 1162507
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: