Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category Music playback
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by Stephane Doyon - 2006-10-25
Last edited by Thomas Martitz - 2011-06-05

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

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.

Closed by  Thomas Martitz
2011-06-05 11:29
Reason for closing:  Accepted
Additional comments about closing:  

r14517

Stephane Doyon commented on 2006-11-03 00:22

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

Stephane Doyon commented on 2006-11-03 00:25

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.

Daniel Dalton commented on 2007-08-23 11:02

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.

Nils Wallménius commented on 2007-08-26 09:00

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?

Michael Sevakis commented on 2007-08-28 00:56

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.

Michael Sevakis commented on 2007-08-28 01:09

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.

Nils Wallménius commented on 2007-08-29 15:12

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

Stephane Doyon commented on 2007-09-14 03:35

Testing, sorry for the inconvenience.

Stephane Doyon commented on 2007-09-14 03:37

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.

Stephane Doyon commented on 2007-10-23 04:08

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.

Stephane Doyon commented on 2007-10-23 04:57

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?

Stephane Doyon commented on 2007-11-27 04:55

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

Stephane Doyon commented on 2007-11-27 05:03

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

Stephane Doyon commented on 2007-11-28 02:54

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

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing