Rockbox

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

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#5992 - Reworked playback.c to fix voice bugs

Attached to Project: Rockbox
Opened by Steve Bavin (pondlife) - Thursday, 14 September 2006, 13:42 GMT+2
Task Type Patches
Category Music playback
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

This is a fairly major reworking of playback.c/pcmbuf.c, to simplify the code and fix crashes which occur when playback stops naturally while using voice and when skipping tracks quickly.

Having removed quite a few wait loops and a few special cases, I quite expect it will introduce more problems ;-)

Please can you test this and report back with recipes for any problems!
This task depends upon

Closed by  Steve Bavin (pondlife)
Monday, 18 September 2006, 13:16 GMT+2
Reason for closing:  Accepted
Additional comments about closing:  Well, it\'s in there and out there now...
Comment by Steve Bavin (pondlife) - Thursday, 14 September 2006, 14:17 GMT+2
And here's the patch!
(application/octet-stream)    playbackvoicefixed.patch (22.7 KiB) 
 apps/playback.c |  204 +++++++++++++++++++++++++++++++++-----------------------
 apps/pcmbuf.c   |   41 ++++++-----
 2 files changed, 145 insertions(+), 100 deletions(-)

Comment by Steve Bavin (pondlife) - Thursday, 14 September 2006, 17:58 GMT+2
Bug: Pressing stop during a crossfade can leave the CPU boosted (due to PCMBUF.C). I've noticed this has been reported before the rework (http://www.rockbox.org/tracker/5520), so hopefully it's not a backwards step!
Comment by Brandon Low (lostlogic) - Thursday, 14 September 2006, 23:47 GMT+2
Ahhh, you fixed what was broken recently after my fixing it previously with the track display being all funky and possibly out of sync. Thanks. (Still reading the rest of the patch)
Comment by Brandon Low (lostlogic) - Thursday, 14 September 2006, 23:54 GMT+2
Code wise, all of your changes make sense to me. One of the stylistic only changes in pcmbuf.c addes a few bytes to the code size, but I won't make too much noise about that. We're not coding for archoses.
Comment by Brandon Low (lostlogic) - Friday, 15 September 2006, 00:05 GMT+2
Something I just noticed, and I'm not sure if this is from these changes or previously is that it takes longer for the WPS to initially display than previously. Perhaps something to do with when you set playing?
Comment by Steve Bavin (pondlife) - Friday, 15 September 2006, 10:35 GMT+2
Hi Brandon - many thanks for checking it over.

I had to move all of the state transitions inside the queue reading thread to fix a stop/skip race condition (at least I think that's what it was).
Maybe we need a new wps_playing variable to allow the WPS to be updated faster?
I'll add some comments next time round to the meanings (as I see them) of playing, voice_is_playing, audio_codec_loaded and voice_codec_loaded. There was (and likely still is) a little confusion regarding the these variables.
Comment by Brandon Low (lostlogic) - Friday, 15 September 2006, 20:02 GMT+2
Yeah, that's why I had moved them into the queue in the first place. It's where they belong. A major part of my rework from this spring was to tighten down the threading model so that the right threads do things always.

A wps_playing variable might make sense. I think that some combination of other variables used to indicate this state, but simplification can come _after_ functioning.
Comment by Steve Bavin (pondlife) - Sunday, 17 September 2006, 18:38 GMT+2
Hmm, I see this has now been committed (:-o) so I hope it works ok; I didn't consider it quite ready for prime time yet (and won't have much time this week to work further...). The good news is I've been running it much of the weekend with no problems, in my configurarion at least!

p.s. Maybe we need a marker on Flyspray patches to say whether work-in-progress or not?
Comment by Dominik Riebeling (bluebrother) - Sunday, 17 September 2006, 20:25 GMT+2
Steve, the "Percent Completed" entry could be used for that IMO. If this patch has been committed completely, shouldn't it get closed? Or was only a part of it applied?

Loading...