[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH] Add an output module for RHVoice.
From: |
Anatol Kamynin |
Subject: |
[PATCH] Add an output module for RHVoice. |
Date: |
Tue, 14 Feb 2012 00:07:13 +0400 |
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.