[PATCH] Add an output module for RHVoice.

[Trevor Saunders]

On Fri, Feb 03, 2012 at 03:38:41PM +0400, Anatol wrote:
> + * Boston, MA 02110-1301, USA.
> + */
> +
> +
> + #ifdef HAVE_CONFIG_H

extra blank line

> +/* RHVoice header file */
> +#include <RHVoice.h>

isn't that obvious?

> +/* Speech Dispatcher includes. */
> +#include "spd_audio.h"

same

> +#define RHV_NUM_CHANNELS     1
> +#define RHV_BITS_PER_SAMPLE  16

I wonder if static const int works here.

> +             RHV_STATE_UNKNOWN = 0,
> +             RHV_STATE_IDLE,
> +             RHV_STATE_SPEAK,
> +             RHV_STATE_STOP,
> +             RHV_STATE_CLOSE
> +} RHV_STATE;

personally I wouldn't name space this stuff since its private to your
module, but its pretty common style.

> +typedef struct _tRHVMsgEntry

personally I dislike this sort of typedef but again its common style so...

> +const char *RHV_DEFAULT_DATAPATH = "/usr/local/sharead/RHVoice";
> +const char *RHV_SOUND_ICON_FOLDER = "/usr/local/share/RHVoice/icons";
> +const char *RHV_DEFAULT_CONFIGPATH = "/usr/local/etc/Rhvoice";

any reason these can't be static?

> +/* callback function prototype */

its seems obvious.

> +int rhv_callback(const short *samples,int num_samples,const RHVoice_event 
> *events,int num_events,RHVoice_message msg);

make it static?  its longer than 80 chars fwiw.

> +/* message to tts*/
> +GSList *rhv_msg_queue = NULL;
> +
> +/* Thread and process control */
> +volatile gint rhv_state= RHV_STATE_UNKNOWN;

> +static pthread_t rhv_speak_thread;
> +static GCond *rhv_msg_queueNotEmpty = NULL;

use  pthread_cond_t and the static initializer.

> +static GMutex *rhv_msg_queue_mutex = NULL;

same with pthread_mutex.

> +#define ABORT(msg) g_string_append(info, msg); \

defining macros with the same name as system macros / functions seems
like a bad idea to me.

Id also suggest putting macrs like this atleast in braces if not do {
... } while (0);

> +        DBG("FATAL ERROR:", info->str); \
> +     *status_info = info->str; \
> +     g_string_free(info, 0); \

I don't remember how the second arg to g_string_free() works off hand so
I'm not sure if that is actually ok, but assuming macros to be called in
a scope with certain variables is kind of icky.

> +     return -1;

I'd really expect a macro named ABORT() to cause an exit directly.

> +     if (NULL == rhv_msg_queue_mutex)
> +     {

{ on same line
but if you used pthreads you could just not have this runtime allocation
silliness.

> +}
> +#undef ABORT

and then you don't use the macro you defined???

> +     if (g_atomic_int_get(&rhv_state) != RHV_STATE_SPEAK) {
> +             DBG(MODULE_NAME ": STOP called when not in speaking.");
> +             return -1;
> +     }
> +
> +     g_atomic_int_set(&rhv_state, RHV_STATE_STOP);

its an int so C says its the word size for the platform which means you
should be able to assume the move will be atomic, but that said the read
and the write can race however you do this, but I'm not sure yet if
that's an actually issue here.

> +     rhv_list_voices_free(rhv_list_voices);

I'm not sure what I think here, in debug situations its nice to free
this stuff on shutdown.  However in production that's dumb since our
address space is about to go away.  The right answer may be to kill the
module process when the server nolonger needs it, but we need to think
about if the module might be doing something that actually needs cleanup
before implementing that.

> +_rhv_speak(void* nothing)
> +{
> +     while(g_atomic_int_get(&rhv_state) != RHV_STATE_CLOSE) {
> +             g_atomic_int_set(&rhv_state, RHV_STATE_IDLE);

yeah,  I'm pretty sure you race with module_close() here.

> +             g_cond_wait(rhv_msg_queueNotEmpty, mx);
> +             while(rhv_msg_queue  != NULL )  {
> +                     g_atomic_int_set(&rhv_state, RHV_STATE_SPEAK);
> +                     module_report_event_begin();
> +
> +                     if (rhv_msg_queue->data != NULL) {
> +                             g_mutex_lock(rhv_msg_queue_mutex);
> +                             /* Setting voice */
> +                             msg_settings = ((RHVMsgEntry*) 
> rhv_msg_queue->data)->settings;

I think you'd be a lot happier if instead of using a gslist you embed
next pointers in the entries which would save space and allow type
safety.  You should then have both head and tail pointers since
currently each list append means walking the whole list.

> +                             UPDATE_PARAMETER(voice_type, rhv_set_voice);
> +                             UPDATE_STRING_PARAMETER(voice.language, 
> rhv_set_language);
> +                             UPDATE_STRING_PARAMETER(voice.name, 
> rhv_set_synthesis_voice);
> +                             UPDATE_PARAMETER(rate, rhv_set_rate);
> +                             UPDATE_PARAMETER(volume, rhv_set_volume);
> +                             UPDATE_PARAMETER(pitch, rhv_set_pitch);
> +                             UPDATE_PARAMETER(punctuation_mode, 
> rhv_set_punctuation_mode);
> +                             UPDATE_PARAMETER(cap_let_recogn, 
> rhv_set_cap_let_recogn);
> +                             /* UPDATE_PARAMETER(spelling_mode, 
> rhv_set_spelling_mode); */
> +
> +                             g_mutex_unlock(rhv_msg_queue_mutex);

you hold that lock a lot longer than you need to, my suggestion would be
use this lock with the condvar the normal way then if cond_wait() comes
back and the queue is non-empty remove the first element from the list
and drop the lock.

> +                             if (((RHVMsgEntry*) rhv_msg_queue->data)->msg 
> != NULL) {
> +                                     
> RHVoice_speak(((RHVMsgEntry*)rhv_msg_queue->data)->msg);

using the queue when you don't hold the lock scares me.

> +                             }
> +                     }       /* if data */
> +
> +                     if (g_atomic_int_get(&rhv_state) == RHV_STATE_STOP) {
> +                             g_mutex_lock(rhv_msg_queue_mutex);
> +                             rhv_msgqueue_free();
> +                             g_mutex_unlock(rhv_msg_queue_mutex);
> +                             module_report_event_stop();
> +                     } else {
> +                             g_mutex_lock(rhv_msg_queue_mutex);
> +                             rhv_msgqueue_pop();
> +                             g_mutex_unlock(rhv_msg_queue_mutex);
> +                             module_report_event_end();
> +                     }

usually people would only put the one differing line in the if.

> +void
> +rhv_set_rate(signed int rate)
> +{
> +     assert(rate >= -100 && rate <= +100);

assertion seems rather harsh, I'd suggest printing an error and
clamping.

> +rhv_set_volume(signed  int volume)
> +{
> +     assert(volume >= -100 && volume <= +100);

same.

> +rhv_set_voice(SPDVoiceType voice)
> +{
> +     return;
> +}
> +
> +void
> +rhv_set_synthesis_voice(const char *name)
> +{
> +     if (name!=NULL && *name)

we really shouldn't call our own functions in dumb ways like that so the
check is kind of silly.

> +     {
> +             int id=  RHVoice_find_voice(name);
> +             if (id <= 0)
> +             {

{ on same line.

> +void
> +rhv_set_language(const char *lang)
> +{
> +     return;

DBG() saying its not supported?

> +} /* rhv_set_language */

comment is kind of silly.

> +
> +void
> +rhv_set_punctuation_mode(SPDPunctuation pm)
> +{
> +     RHVoice_punctuation_mode  mode = RHVoice_get_punctuation_mode();

you should handle all cases in the switch then this isn't needed.

> +void
> +rhv_handle_events(const RHVoice_event *events,int count)
> +{
> +     int i;
> +     if (NULL == events || !count) return;
> +
> +     for(i=0; i<count && (events+i) != NULL; ++i) {
> +             switch (events[i].type) {
> +                     case     RHVoice_event_mark:
> +                             DBG(MODULE_NAME ": index mark |%s|.", 
> events[i].id.name);
> +                             
> module_report_index_mark((char*)(events[i].id.name));
> +                             break;
> +
> +                     case RHVoice_event_play:
> +                             if (0 >  module_play_file(events[i].id.name)) {
> +                                     DBG(MODULE_NAME ": Unable to play |%s|",
> +                                             events[i].id.name);
> +                             }
> +                             break;
> +
> +                     case RHVoice_event_word_start: break;
> +                     case RHVoice_event_word_end: break;
> +                     case RHVoice_event_sentence_start: break;
> +                     case RHVoice_event_sentence_end: break;

I'd probably use fall through and a single break for all of those cases.

> +             }       /* switch */
> +     } /* for events */
> +} /* rhv_handle_events */
> +
> +SPDVoice **
> +rhv_list_voices_new(void)
> +{
> +     int numVoices = 0, i=0;
> +     SPDVoice **voices = NULL;
> +
> +     numVoices = RHVoice_get_voice_count();
> +     if (numVoices <= 0) return NULL;
> +     voices = g_try_malloc(sizeof(SPDVoice *)*(numVoices+1));

blank line after the if.

btw why do you want a failable malloc here?

> +     if (NULL == voices) return NULL;
> +     for (i=0; i<numVoices;++i) {

space after ';' please.

> +             voices[i] = g_try_malloc(sizeof(SPDVoice));
> +             voices[i]->name = g_strdup(RHVoice_get_voice_name(i+1));
> +             voices[i]->language = g_strdup("ru");
> +             voices[i]->variant= g_strdup("");
> +     } /* for */
> +
> +     /* last pointer must be NULL */
> +     voices[i] = NULL;
> +     return voices;
> +} /* rhv_list_voices_new */
> +
> +void
> +rhv_list_voices_free(SPDVoice **voices)
> +{
> +     if (voices != NULL) {
> +             int i = 0;
> +             for (i=0; voices[i] != NULL;++i) {
> +                     if (voices[i]->name) g_free(voices[i]->name);
> +                     if (voices[i]->language) g_free(voices[i]->language);
> +                     if (voices[i]->variant) g_free(voices[i]->variant);

you should be able to take it as an invariant that a voice has name /
language / variant description strings and not check they aren't null.

> +void
> +rhv_msgqueue_pop(void)

pop is a weird name for a function that returns nothing, call it
free_head()?

> +rhv_msgqueue_push(RHVoice_message msg)

 I'd advise against passing structs by value unless you really need to.

> +{
> +     assert(msg != NULL);
> +     RHVMsgEntry *entry = g_try_malloc(sizeof(RHVMsgEntry));
> +     if (NULL == entry) {

so, that leaks, but you should neer be in this case so you should be
able to just get rid of the check.

thanks for working on this!

Trev

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: 
<http://lists.freebsoft.org/pipermail/speechd/attachments/20120204/d660e171/attachment-0001.pgp>