Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added --speex-* options to configure. #703

Closed
wants to merge 1 commit into from

Conversation

giuliopaci
Copy link
Contributor

This pull request adds three options (--speex-root, --speex-libdir, --speex-includedir) that can be used to locate speex library. In my setup speex lib are in /usr/lib/x86_64-linux-gnu, while include files are in /usr/include:
./configure --speex-libdir=/usr/lib/x86_64-linux-gnu --speex-root=/usr lets me use the system library instead of a custom one.

@@ -70,7 +70,7 @@ unset MKLLIBDIR

function usage {
echo 'Usage: ./configure [--static|--shared] [--threaded-atlas={yes|no}] [--atlas-root=ATLASROOT] [--fst-root=FSTROOT]
[--openblas-root=OPENBLASROOOT] [--clapack-root=CLAPACKROOT] [--mkl-root=MKLROOT] [--mkl-libdir=MKLLIBDIR]
[--openblas-root=OPENBLASROOOT] [--clapack-root=CLAPACKROOT] [--mkl-root=MKLROOT] [--mkl-libdir=MKLLIBDIR] [--speex-root=SPEEXROOT] [--speex-libdir=SPEEXLIBDIR] [--speex-includedir=SPEEXINCLUDEDIR]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please break this line, it's very long.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and please do it using commit --amend, or rebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just updated the pull request with your suggestion.

@danpovey
Copy link
Contributor

I assume you need speex for something specific- it's only needed in 'compress-uncompress-speex' which is only useful if you want to recognize data compressed with speex. Is that the case? If you don't need speex, it's possible no-one else is really using this feature either, so I wouldn't want to clutter the configure script.

@jtrmal
Copy link
Contributor

jtrmal commented Apr 15, 2016

I'm actually not really fond of speex support being part of the kaldi libs
-- I don't see any argument why we should include speex and not sph or mp3
or any other format. I understand that we should keep it now mainly for
continuity reasons, but besides that? Not 100% sure but I think sox can
convert speex to wav as well...
y.

On Thu, Apr 14, 2016 at 9:47 PM, Daniel Povey notifications@github.com
wrote:

I assume you need speex for something specific- it's only needed in
'compress-uncompress-speex' which is only useful if you want to recognize
data compressed with speex. Is that the case? If you don't need speex, it's
possible no-one else is really using this feature either, so I wouldn't
want to clutter the configure script.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#703 (comment)

@danpovey
Copy link
Contributor

It's totally optional-- it's only there for the sake of the
compress-uncompress-speex utility, which has a rather specific purpose to
simulate speex compression in wav files. Other than that we don't need
it. Unless Giulio needs it for that purpose, I may actually not accept
this PR, because almost no-one will likely be requiring speex support.

On Thu, Apr 14, 2016 at 6:54 PM, jtrmal notifications@github.com wrote:

I'm actually not really fond of speex support being part of the kaldi libs
-- I don't see any argument why we should include speex and not sph or mp3
or any other format. I understand that we should keep it now mainly for
continuity reasons, but besides that? Not 100% sure but I think sox can
convert speex to wav as well...
y.

On Thu, Apr 14, 2016 at 9:47 PM, Daniel Povey notifications@github.com
wrote:

I assume you need speex for something specific- it's only needed in
'compress-uncompress-speex' which is only useful if you want to recognize
data compressed with speex. Is that the case? If you don't need speex,
it's
possible no-one else is really using this feature either, so I wouldn't
want to clutter the configure script.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#703 (comment)


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#703 (comment)

@giuliopaci
Copy link
Contributor Author

I do not have such a use case yet, but I can have it. I just noticed the message reporting that speex was not detected and the fact that it was not possible at all to use the system library.
Even if I am not using it personally, I do not see the point in not accepting this pull request if the speex section of the configure is going to stay there: why preferring a solution that is only partially working if there is one that solves a more general case?

@jtrmal: I do not know if sox handles speex files, but ffmpeg handles them for sure (both encoding and decoding).

@danpovey
Copy link
Contributor

Let me leave it up there a bit while I think about it.
I'm OK to have the options in principle, but I'm concerned that the speex
stuff is too 'visible' right now- it gives the impression that it's much
more important than it actually is. (We don't use it for the things you
might expect us to use it for).
Dan

On Thu, Apr 14, 2016 at 7:10 PM, Giulio Paci notifications@github.com
wrote:

I do not have such a use case yet, but I can have it. I just noticed the
message reporting that speex was not detected and the fact that it was not
possible at all to use the system library.
Even if I am not using it personally, I do not see the point in not
accepting this pull request if the speex section of the configure is going
to stay there: why preferring a solution that is only partially working if
there is one that solves a more general case?

@jtrmal https://github.com/jtrmal: I do not know if sox handles speex
files, but ffmpeg handles them for sure (both encoding and decoding).


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#703 (comment)

@giuliopaci giuliopaci force-pushed the master branch 2 times, most recently from fea3058 to 812dba1 Compare April 21, 2016 09:49
@giuliopaci giuliopaci force-pushed the master branch 2 times, most recently from 8ea2a5d to 01308a7 Compare April 28, 2016 18:11
@giuliopaci
Copy link
Contributor Author

Hi,
did you thought about this pull request?
Giulio

@danpovey
Copy link
Contributor

danpovey commented May 4, 2016

I just created pr #758, which addresses the issue in a different way.

The underlying issue is that the message that configure printed made you think that linking against the speex library was something that was possibly important, and that you needed, which was a waste of your time, because you didn't need it. I'd prefer for it to languish in obscurity, because it's only needed by a very specific tool that most people don't want to use.

@giuliopaci
Copy link
Contributor Author

giuliopaci commented May 4, 2016

While I understand that this feature is probably not for me, still I do not understand while keeping this feature in the code if it is not useful or, if it is useful to anybody why not to allow them to use their precompiled version of the library.

I do not think the new wording would have keep me away from trying to compile with speex library. I have that library as part of my typical environment and if any system is reporting that it will have one more feature with it, it seems natural to me to enable that feature (unless the message is saying it is harmful, or clearly make me understand why I will never need it).

I am not sure I understand the reasoning behind advertising that a feature is not used while, at the same time, trying to convince it is not important and yet do not give a proper support to use precompiled libraries for those willing to enable that feature.

@danpovey
Copy link
Contributor

danpovey commented May 4, 2016

Sorry, I think the harm in this would outweigh the benefit on balance-> closing the PR.

@danpovey danpovey closed this May 4, 2016
@giuliopaci
Copy link
Contributor Author

This is my last attempt for this, then I will give up. What about if the commit is made so that only lines from 465 to the end are used? The default behaviour of the script will be the usual one, but still it will be possible to specify correct parameters for compilation with precompiled speex library, by setting SPEEXROOT and SPEEXLIBDIR environment variables.

@danpovey
Copy link
Contributor

danpovey commented May 5, 2016

I would be OK with everything except the additional stuff in the usage
message.

On Wed, May 4, 2016 at 8:36 PM, Giulio Paci notifications@github.com
wrote:

This is my last attempt for this, then I will give up. What about if the
commit is made so that only lines from 465 to the end are used? The default
behaviour of the script will be the usual one, but still it will be
possible to specify correct parameters for compilation with precompiled
speex library, by setting SPEEXROOT and SPEEXLIBDIR environment variables.


You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub
#703 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants