Rockbox

New version to take into account talk.c rev 1.56: in shutup(), even if
voice clip queue is empty, mark the thumbnail buffer as free. shutup()
will still stop the audio in the PCM buffer and so we won't get a
callback, and so we might miss the thumbnail buffer becoming free

In at least two cases, some code in playback.c will stop and purge voice
output, but without calling the callback, so that talk.c is not aware
that voice has been cancelled. talk.c may then think that there are still
clips in the queue, and will not restart playback when a new clip is
enqueued, and of course will not know that the thumbnail buffer has been
freed, which leads to a nice hang.

The scenario I have encountered involves cond_talk_ids_fq from P#6159,
which forces enqueuing of the next clip even if talk_id is called with
enqueue set to false (used for splashes). I don't know if this is an
issue without cond_talk_ids_fq().

Anyway a simple fix is attached: call shutup() instead of just notifying
the voice thread; it'll do that but clear talk.c's state too.

Ok I think this patch is the right one for this task. Let me know if it isn't.

Anyway updated so it works on the latest svn.

Stephane, now that  FS#6159  has been comitted is it still correct to
use shutup() or should do_shutup() be used as in ddaltons patch?

Basically for SWCODEC instead of posting to the queue directly, do_shutup posts to the voice queue like playback.c already does and then resets the talk queue to empty as well so that the voice thread doesn't pick up any more and stops. Sounds good but man I wan't a "flatter" interface to all this where talk depends on playback not the other way at all. Come to think of it, technically the voice codec is implemented in talk.c for HWCODEC.

One method that should be thread, ISR and multicore safe is to add a blocking variable that, if nonsignaled (whichever value you decide indicates that), makes mp3_callback return 0. After you've set it as such, it's safe to manipulate the queue variables without interference. Resignal it after setting things up and it's ok to call it again. Thing is, HWCODEC can shut off the interrupt but SWCODEC cannot shut off the threads right away. The queue indexes could still be interfered with even after being reset. This blocker would shut off the remainder in the current clip as well.

Oy, scratch that for COP use. A different method would need to be used there since the COP could still be in the callback at the time. For the time being I see no problem with the patch.

I committed the play-flush-voice-fix.diff patch, don't use .talk clips personally so I cant try the other patch

Testing, sorry for the inconvenience.

Having difficulty submitting this comment for some reason...
Hope this doesn't end up in triplicate.

Thanks for committing my play-flush-voice-fix.diff.

Yes, do_shutup() was the right function to use.

Here's an updated multiple-thumbnails.diff.

Admittedly, the problem this fixes is probably only visible when
enqueuing multiple .talk clips. I use that to speak each component of a
full path in the playlist viewer in P#6323. Also in the bookmark
selection screen, I have it say both the directory name and track name,
in P#6240.

There's a vaguely similar issue when spelling out long filenames, in the
above contexts and in the id3 viewer: a long path can easily overflow
the 64 queue entries in talk.c.

The simplistic solution in multiple-thumbnails.diff is to just wait for
the thumbnail buffer to become available. During that time of course we
don't process keys, so it's impossible to interrupt the talking, which
can be pretty annoying.

My first solution to this had been to just add more thumbnail buffers and
voice clip queue entries in talk.c. But that seems like a poor use of
resources.

My current workaround is a hack, the same for both the thumbnail and talk
queue issues: wait, but peek in the button queue and look for a button
press, if there's one then forget what we meant to enqueue and just
return. In the contexts that use this, pretty much any button press leads
to the voice being interrupted anyway. Still, this isn't exactly clean.

The theoretical solution would be to have some thread, perhaps the voice
thread in playback.c, or some other, wait around for the thumbnail buffer
to become available and load the next .talk clip into it, and wait for
the talk queue to drain and continue spelling out the long string into
it. But this seems like a pretty complicated solution to add on top of
something that's already pretty convoluted and a bit brittle. I presume
others would agree given some of the above comments.

I could use some advice as to what to do about this.

jhMikeS, or anyone else: Would something along the lines of the attached
patch be a good way of synchronizing talk.c and playback.c, in particular
for talk.c queue_read?

The idea is to use the callback for playback.c to acknowledge when it has
finished handling the shutup request, at which point we can safely reset
the talk.c queue indicies.

OK second attempt. I do see more races, and a problem with the previous patch.
How about this one.

Can someone clarify what context that talk.c callback runs in?

The talk-race.diff patch adds a mutex to protect queue_read and
queue_write in talk.c and prevent races with those.

The thumbnail-safe.diff patch keeps track of when the thumbnail buffer
is in use and prevents its simultaneous reuse by a subsequent
call to talk_file(). If the buffer is busy then the subsequent
talk_file() request is simply skipped.

The multi-thumbnails.diff patch builds on top of this and allows
queuing up multiple thumbnails in the buffer, one after the other, as
long as buffer space permits. I've kept buffer management simplistic:
the buffer can be reused from position 0 when all buffered clips have
been spoken or when we have done talk_shutup().

Bug fix: an inverted condition in multi-thumbnails when appending silence.