Rockbox

Tasklist

FS#6239 - Lack of protection against reuse of p_thumbnail: tentative fix

Attached to Project: Rockbox
Opened by Stephane Doyon (sdoyon) - Wednesday, 25 October 2006, 05:37 GMT
Last edited by Thomas Martitz (kugel.) - Sunday, 05 June 2011, 11:29 GMT
Task Type Patches
Category Music playback
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

As far as I can tell, calling talk_file() twice in quick succession has
both instances using the same buffer.

A second call to talk_file() will load the clip in the same p_thumbnail
buffer that contains the previously enqueued voice clip.

tree.c is pretty much the only user of talk_file() and it seems to more
or less avoid this. But I intend to use talk_file() to better describe
bookmark entries, two times per entry, so I need to fix this.

The symptom of this problem that I can hear, is that sometimes the first
clip enqueued with talk_file() is interrupted and the rest of my queued
clips are not spoken. This patch makes it work reliably for me.

My fix is pretty simplistic: talk_file() will simply wait if the buffer
is busy, until it has been spoken (or at least copied to the pcm
buffer). This might introduce a lag in the UI and isn't ideal
obviously. But it seems to work well enough for my purposes, no
perceptible lag somehow in my usage pattern. The correct fix is probably
to have two or three different buffers, but I'm not up to the task of
making that work for Archos.
This task depends upon

Closed by  Thomas Martitz (kugel.)
Sunday, 05 June 2011, 11:29 GMT
Reason for closing:  Accepted
Additional comments about closing:  r14517
Comment by Stephane Doyon (sdoyon) - Friday, 03 November 2006, 00:22 GMT
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
Comment by Stephane Doyon (sdoyon) - Friday, 03 November 2006, 00:25 GMT
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.
Comment by Daniel Dalton (ddalton) - Thursday, 23 August 2007, 11:02 GMT
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.
Comment by Nils Wallménius (nls) - Sunday, 26 August 2007, 09:00 GMT
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?
Comment by Michael Sevakis (MikeS) - Tuesday, 28 August 2007, 00:56 GMT
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.
Comment by Michael Sevakis (MikeS) - Tuesday, 28 August 2007, 01:09 GMT
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.
Comment by Nils Wallménius (nls) - Wednesday, 29 August 2007, 15:12 GMT
I committed the play-flush-voice-fix.diff patch, don't use .talk clips personally so I cant try the other patch
Comment by Stephane Doyon (sdoyon) - Friday, 14 September 2007, 03:35 GMT
Testing, sorry for the inconvenience.
Comment by Stephane Doyon (sdoyon) - Friday, 14 September 2007, 03:37 GMT
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.
Comment by Stephane Doyon (sdoyon) - Tuesday, 23 October 2007, 04:08 GMT
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.
Comment by Stephane Doyon (sdoyon) - Tuesday, 23 October 2007, 04:57 GMT
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?
Comment by Stephane Doyon (sdoyon) - Tuesday, 27 November 2007, 04:55 GMT
The talk-race.diff patch adds a mutex to protect queue_read and
queue_write in talk.c and prevent races with those.
Comment by Stephane Doyon (sdoyon) - Tuesday, 27 November 2007, 05:03 GMT
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().
Comment by Stephane Doyon (sdoyon) - Wednesday, 28 November 2007, 02:54 GMT
Bug fix: an inverted condition in multi-thumbnails when appending silence.

Loading...