[PATCH] Add an output module for RHVoice.

[Anatol Kamynin]

Hi,
Thanks Trevor for comments.

>> +static GCond *rhv_msg_queueNotEmpty = NULL;
> use  pthread_cond_t and the static initializer.

Do I have to use only pthread-functions?
I have not found the ban on the use of Glebe in the Speech-Dispatcher 
documentation .
In addition, there are output modules that are already using the Glebe, so I 
have reason to think that it is acceptable to Speech-Dispatcher.
If I can choose the API, then I'll use glib.

>> +#define ABORT(
[...]
>> +#undef ABORT
>
> and then you don't use the macro you defined???

Sorry, I skipped this macro, because I moved by the function names .

>> + g_atomic_int_set(&rhv_state, RHV_STATE_STOP);
>
> its an int so C says its the word size for the platform 

Yes, of course. I too read the old books about the C programming language. 
However, the standard has no strong rule for this aspect;  this is only a 
recommendation.

> ...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.

This seems to be right. I think almost the same.
But glib has the atomic functions *only* for the int and pointer.
If the operations with int is so thread-safe as you say, then it seems that the 
Glebe is developed by paranoiac...
See 'GLib Reference Manual':
"You must not directly read integers or pointers concurrently accessed by 
multiple threads, but use the atomic accessor functions instead..."

>> +_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.

Yes, of course.
Is this race really dangerous?
There are two variants:
1. pthread_kantsel starts  firstly, so it terminates the stream function.
2. The stream function ends itself, and pthread_kantsel starts later, therefore 
it fails. 
Do You find this as a implicit threat ?

I plan to remove the call of module_terminate_thread()
from the module_close() function,
and insert pthread_join():
 g_atomic_int_set(&rhv_state, RHV_STATE_CLOSE);
 g_cond_signal(rhv_msg_queueNotEmpty);
 pthread_join(rhv_speak_thread,NULL);

> You should then have both head and tail pointers since
currently each list append means walking the whole list.

Yes, of course.
But this messages list is almost always empty or very short. If the engine 
synthesizes speech quickly, then the list is empty.
If the engine synthesizes speech slowly, the problem with the performance of 
the engine will outbid the cost of moving across the list.

>> +    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 the list is not empty, the appending a message to the end of the list does 
not change his head.
But I plan to make a local copy of the message inside the locked fragment and 
take it as  the function argument.

>> +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.

If we did everything we should, then dentists would be ruined.
Sorry, but the "kind of silly" is too subjective. This mainly reflects the 
state of the speaker, not a state of the topic.
I would prefer more precise arguments than "should" or  "not should" and 
"silly" or "not silly".

>> +void
>> +rhv_msgqueue_pop(void)
>
> pop is a weird name for a function that returns nothing, 

No, not weird. See, for example:
void pthread_cleanup_pop(int execute);
or stl:
    void pop_front();
    void pop_back();

> call it
free_head()?

Thanks, but it hides semantics of this function. This function shifts the 
message queue removing the first entry and assign the next entry as the head.

>> +rhv_msgqueue_push(RHVoice_message msg)
>
>  I'd advise against passing structs by value unless you really need to.

Yes, of course.
But  RHVoice_message  is pointer. Se the next line in this function:
>> + assert(msg != NULL);
or RHVoice.h:
  struct RHVoice_message_s;
  typedef struct RHVoice_message_s *RHVoice_message;

    Anatol.