hadware / voxpopuli

Python wrapper for Espeak and Mbrola, for simple local TTS
MIT License
28 stars 16 forks source link

Initial support for Windows #3

Closed klvbdmh closed 7 years ago

klvbdmh commented 7 years ago

References: #2

Only _str_to_phonems functionality for now.

hadware commented 7 years ago

Alright, fine with me :)

Will you be updating the other class methods (_str_to_audio and phomems_to_audio) so they match the the new way of setting up arguments you introduced? (much appreciated btw)

klvbdmh commented 7 years ago

Yes I will. I only did one class method just to see if the format is fine and if there are no regressions on Linux. Now that I know how you fixed my code, I'll apply those fixes in other methods.

One question: why is it necessary to wrap phonem_synth_args in ' '.join? subprocess.run() accepts a list as its first argument and I use it like so in my scripts on various Linux servers I manage. Is there some sort of regression with a list that doesn't happen with a single string?

hadware commented 7 years ago

Yes I will. I only did one class method just to see if the format is fine and if there are no regressions on Linux. Now that I know how you fixed my code, I'll apply those fixes in other methods.

Good. Very good.

One question: why is it necessary to wrap phonem_synth_args in ' '.join?

I genuinely have no idea. I thought so as well, and I checked and double checked, but when I pass the args as a list, the run command doesn't return anything. I'm suspecting it might be because of this:

If args is a sequence, the first item specifies the command string, and any additional items will be treated as additional arguments to the shell itself.

(extracted from http://stackoverflow.com/a/15109975/1327143 which itself was extracted from the doc)

and here, the first argument is a MALLOC_CHECK=0 which might screw things up a bit.

klvbdmh commented 7 years ago

BTW I figured out how to get mbrola.exe binary - it's inside a DOS archive that has to be downloaded separately. I updated the installation instructions in my fork. I also successfully piped espeak and mbrola together to create a playable .wav file. I have to iron out a few details and I'll push the updates tomorrow.

I thought so as well, and I checked and double checked, but when I pass the args as a list, the run command doesn't return anything

What happens when you omit shell=True?

hadware commented 7 years ago

I get

FileNotFoundError: [Errno 2] No such file or directory: 'MALLOC_CHECK_=0'

I mean, I know passing a string isn't standard practice, but for this particular case I think we can ignore it. It's good to know that you've cracked the mbrola.exe thing. I'll try creating more unittests in the meantime to prevent any regressions on linux.

klvbdmh commented 7 years ago

All right. String it is.

Any preferences about max line length? I've noticed you're using 120, which is fine by me.

When you're writing more tests, can you add TestPhonemsToAudio? Right now we have Str -> Audio and Str -> Phonems.

hadware commented 7 years ago

Max line length? You mean, lines in the code? If that's what you're talking about, unless i've made a mistake, i'm trying as hard as I can to conform to PEP8. Else, must be a mistake.

I'll indeed write this missing unittest, thanks for pointing that out.

klvbdmh commented 7 years ago

Yes, line length in the code. PEP8 recommends 80, but 120 is a sensible option too; it's what I use in my projects.

hadware commented 7 years ago

Oh, I'm confirming to PEP8 in all cases, since Pycharm's automatic PEP8 checker immediatly screams at me when I don't respect it.