Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#9981 - Tidy up dircache use of audiobuf and safety lock on buffer_alloc

Attached to Project: Rockbox
Opened by Steve Bavin (pondlife) - Wednesday, 04 March 2009, 13:17 GMT+2
Last edited by Steve Bavin (pondlife) - Saturday, 22 October 2011, 11:40 GMT+2
Task Type Patches
Category Operating System/Drivers
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

This patch removes the code in dircache.c that directly modifies 'audiobuf' and replaces it with buffer_alloc() calls.
It also adds a lock to buffer_alloc() to ensure nobody attempts to call it once it's being used for audio.

Probably nothing too radical, but I'd like a dircache expert to take a look at it before I commit.

It would be good if an Archos owner could try it too - the MAS audio setup (especially with talking) might not work in the way I expect.
   buffer_alloc_safety.patch (3.6 KiB) 
 apps/playback.c            |    5 ++++-
 firmware/export/buffer.h   |    1 +
 firmware/common/dircache.c |   13 ++++++-------
 firmware/buffer.c          |   10 ++++++++++
 4 files changed, 21 insertions(+), 8 deletions(-)

This task depends upon

Closed by  Steve Bavin (pondlife)
Saturday, 22 October 2011, 11:40 GMT+2
Reason for closing:  Out of Date
Additional comments about closing:  Yes.
Comment by Steve Bavin (pondlife) - Wednesday, 04 March 2009, 13:18 GMT+2
One other note - the dircache.c variable 'thread_enabled' seems like it should be unneeded. Any thoughts on this?
Comment by Jonathan Gordon (jdgordon) - Monday, 23 March 2009, 00:32 GMT+2
from a very quick look it looks fine, its no question a good idea to stop everything manually fiddling with audio_buf.

I'm not sure I see the point of buffer_lock() though. is there any reason why you couldnt just replace the few lines which fiddle with audio_buf() to just be buf_alloc() calls?
Comment by Steve Bavin (pondlife) - Monday, 23 March 2009, 08:31 GMT+2
buffer_lock() was inspired by some hacking I did trying to improve the timestretch patch. I did a buf_alloc() call after audio had been initialised, yet all appeared to work fine most of the time. With this safety net it would have been obvious I was corrupting things Still, it does add a little binsize, so maybe not needed.
Comment by Jonathan Gordon (jdgordon) - Monday, 23 March 2009, 16:46 GMT+2
well, if its useful screw bin size and keep it.
Comment by Rosso Maltese (asettico) - Friday, 04 September 2009, 12:15 GMT+2
Sync to r22617 and fixed the type mismatch warning in the panicf call.

As reported in  FS#10516 , when disconnecting the cable after a USB connection, the panic screen here introduced is showed.
I can't tell anything more, but surely it reveals either an illegal operation attempt or a wrong management in the patch.
   os-9981buffer_alloc_safety_v2.0.patch (3.5 KiB) 
 apps/playback.c            |    5 ++++-
 firmware/export/buffer.h   |    1 +
 firmware/common/dircache.c |   13 ++++++-------
 firmware/buffer.c          |   10 ++++++++++
 4 files changed, 21 insertions(+), 8 deletions(-)

Comment by Thomas Martitz (kugel.) - Saturday, 17 October 2009, 01:59 GMT+2
I think software USB uses the audio buffer since it needs to stop playback for UMS anyway. It probably frees it after again. I assume the panic is due to that.
Comment by Rosso Maltese (asettico) - Saturday, 17 October 2009, 10:26 GMT+2
Sync to r23221.

About the panic: should it have meaning to avoid the check when disconnecting USB?
   os-9981buffer_alloc_safety_v2.0.patch (3.5 KiB) 
 apps/playback.c            |    5 ++++-
 firmware/export/buffer.h   |    1 +
 firmware/common/dircache.c |   13 ++++++-------
 firmware/buffer.c          |   10 ++++++++++
 4 files changed, 21 insertions(+), 8 deletions(-)

Comment by Andree Buschmann (Buschel) - Friday, 21 October 2011, 23:58 GMT+2
Isn't this outdated due to the dircache/buflib rework?

Loading...