Rockbox

Tasklist

FS#8918 - Voice multiple thumbnails and talk race fixes

Attached to Project: Rockbox
Opened by Stephane Doyon (sdoyon) - Sunday, 20 April 2008, 20:42 GMT
Last edited by Stephane Doyon (sdoyon) - Tuesday, 15 July 2008, 14:24 GMT
Task Type Patches
Category User Interface
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

I've been stuck on this stuff for a while and I'd like to move
forward. This patch reverts a previous commit so a review is called for.

Starting a fresh task to reduce confusion. This supersedes  FS#6239 .

I need the ability to enqueue more than one .talk clip, aka thumbnail, at
a time. Needed for proper voicing of the bookmark selection menu, and
eventually for the playlist viewer and to speak full paths in the id3
viewer.

Also I see some juggling of talk queue indicies in talk.c that don't look
thread safe and that are begging for a mutex.

When speaking a thumbnail, the current implementation just grabs the
thumbnail buffer and uses it, not caring whether it might have been in
use already by a previously queued thumbnail in the process of being
played out. It mostly works simply because we always shutup before
enqueuing a thumbnail.

Fixing this and allowing to enqueue multiple thumbnails requires to
properly track when the thunbnail buffer is used, and when we are done
with it, in particular when shutting up.

The first thing this patch does is to make the shutup operation
synchronous again. This means reversing r15872 by preglow: "Attempt at
fixing the statusbar showing up late in some screens and circumstances."

I do two things to try to mitigate the original problem (what I've been
able to find out about it anyway...):
-mp3_play_stop() does nothing until the audio thread is ready. This
should eliminate initial delays when initializing audio.
-the shutup operation is made a noop when no voice has been queued,
avoiding having to wait on the VOICE_STOP message being processed. In
particular it does nothing if voice is not being used.

Hopefully this solves the visual UI lag issues, but as a blind user I
can't say I've really confirmed this.

Having shutup synchronous simplifies things. At least in the case of
thumbnails, I need to be able to wait for shutup to have completed and
for the buffer to be freed. If shutup is not synchronous, I'll need to
setup event notification in talk.c to synchronize the threads. That can
be done, I had some code for it in  FS#6239 , but it means more code and
more complexity.

Given that there are still some stability and buffering issues with voice
and with pcm, such as B#8066 and the inconsistent pcm beep issue among
others, I would much prefer the shutup function to be deterministic and
simpler: it helps debugging in general to know that it has really stopped
when talk_shutup() returns.

This patch allows loading multiple thumbnails back-to-back in the one
thumbnail buffer. That buffer is currently 64KB, and that is usually
sufficient to accomodate two .talk clips. I keep track of when the
buffer is used and how much data is in it.

Finally, i put in a mutex to prevent race conditions in handling the talk
queue indices and the thumbnail buffer state.
This task depends upon

Closed by  Stephane Doyon (sdoyon)
Tuesday, 15 July 2008, 14:24 GMT
Reason for closing:  Accepted
Comment by Michael Sevakis (MikeS) - Thursday, 24 April 2008, 21:22 GMT
As far as I can tell, things look good here. Mostly I'm able to evaluate the voice thread aspect. Perhaps "queue_lock" should be renamed "talk_queue_lock" so the name never conflicts with potential kernel.c calls?

A little GCC hint (sorry if you knew already): "({...})" is cleaner alternative "do {...} while(0)" that also allows returning a value. Example: ({ int _x = ...; ...; _x; })
Comment by Michael Sevakis (MikeS) - Thursday, 24 April 2008, 21:52 GMT
I found one problem. mutex_init is called from reset_state which isn't a one-time init. the mutex must be initialized before any multithreads access to it and never more than one time or scheduler data structures can be corrupted. a one-time talk_startup call (or some other similar thing) guaranteed before anything has access to voice code is needed.
Comment by Michael Sevakis (MikeS) - Thursday, 24 April 2008, 22:01 GMT
One more thing (sorry :), I suggest using SHAREDBSS_ATTR or IBSS_ATTR on any kernel object unconditionally that while a particular object itself may not be dual core accessed to block between them, priority inheritance chains can be regardless of where the code runs. At this point it only applies to mutexes and queues with queue_send enabled it avoids crazy bugs later and isn't costly.
Comment by Stephane Doyon (sdoyon) - Sunday, 27 April 2008, 03:47 GMT
MikeS wrote:

>As far as I can tell, things look good here.

Thanks for taking the time.

>Mostly I'm able to evaluate the
>voice thread aspect.

That's why I asked of course.

>Perhaps "queue_lock" should be renamed
>"talk_queue_lock" so the name never conflicts with potential kernel.c calls?

Sure. Might be wise.

>A little GCC hint (sorry if you knew already): "({...})" is cleaner
>alternative "do {...} while(0)" that also allows returning a value. Example:

I knew of this but had only used it as an expression. I hadn't thought it
could return void, and that it was allowed to be empty. I have to wonder
why I never saw an empty ({ }), or an ({ ... }) for a non-expression in
other projects, while I saw lots of do { ... } while(0). Off the top of
my head I can't think why ({ }) wouldn't work just as well, and it's true
that it's more readable.

>I found one problem. mutex_init is called from reset_state which isn't a
>one-time init. the mutex must be initialized before any multithreads access
>to it and never more than one time or scheduler data structures can be
>corrupted.

Oh thanks for catching this.

>a one-time talk_startup call (or some other similar thing)
>guaranteed before anything has access to voice code is needed.

OK, I'm putting it in talk_init(), guarded by !talk_initialized. That
ought to do it.

>One more thing (sorry :), I suggest using SHAREDBSS_ATTR or IBSS_ATTR on any
>kernel object unconditionally that while a particular object itself may not
>be dual core accessed to block between them, priority inheritance chains can
>be regardless of where the code runs. At this point it only applies to
>mutexes and queues with queue_send enabled it avoids crazy bugs later and
>isn't costly.

Thanks.

There is one spot where thumbnail_buf_used is read without having to take
the mutex. I had made it volatile for that reason but I guess I'll add
SHAREDBSS_ATTR too. Hope this is right, I won't claim to understand this
100%.

Shouldn't SHAREDBSS_ATTR be included inside a struct definition for
mutexes in kernel.h, instead of added by hand on each declaration?

Anyway many thanks again. I'll be glad to finally get this done and
be able to commit the patches depending on this.

Here's the new patch.

Loading...