Rockbox

Tasklist

FS#10739 - Playback.c refactor

Attached to Project: Rockbox
Opened by Jeffrey Goode (Blue_Dude) - Thursday, 29 October 2009, 23:49 GMT
Last edited by Jeffrey Goode (Blue_Dude) - Sunday, 01 November 2009, 00:13 GMT
Task Type Patches
Category Music playback
Status Closed
Assigned To No-one
Operating System SW-codec
Severity Low
Priority Normal
Reported Version Release 3.4
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

This is an initial refactoring of playback.h and playback.c. This first attempt separates the codec thread from the audio thread. There are some similarities in a shared common codebase, mostly a few common variables. It compiles cleanly for a couple of targets and there's no apparent harm, but this project needs a fresh set of eyes.

Other than fixing a couple of small items, there should be no change. That comes later. ;)
This task depends upon

Closed by  Jeffrey Goode (Blue_Dude)
Sunday, 01 November 2009, 00:13 GMT
Reason for closing:  Accepted
Additional comments about closing:  Committed as r23444
Comment by Jeffrey Goode (Blue_Dude) - Friday, 30 October 2009, 02:20 GMT
Here's a much cleaner version. It removes the common code files and uses a SVN copy to produce the new codec_thread files. This gives a much clearer sense of what has been changed.
Comment by Jeffrey Goode (Blue_Dude) - Friday, 30 October 2009, 03:17 GMT
Bug fix. Missing #defines etc.
Comment by Maurus Cuelenaere (mcuelenaere) - Friday, 30 October 2009, 11:42 GMT
I'm not sure how relevant this is to your work, but could you change the way codecs wait for data to be ready and start decoding? (the "while (!*ci->taginfo_ready && !ci->stop_codec) ci->sleep(1);" part that is in every codec)

This should definitely use wakeups (or queues if you don't want the increased binsize) instead of the busy-looping, but I'm not sure if this applies to your rework. If not, could you try generalizing the part that sets ci->stop_codec and ci->taginfo_ready, so it'll be easier for someone to change it to wakeups/queues later on?
Comment by Jeffrey Goode (Blue_Dude) - Friday, 30 October 2009, 12:15 GMT
To apply this patch cleanly, you should make subversion copies of playback.c and playback.h. Use: "svn cp apps/playback.c apps/codec_thread.c" and "svn cp apps/playback.h apps/codec_thread.h". This will give the new files something to patch to.
Comment by Jeffrey Goode (Blue_Dude) - Friday, 30 October 2009, 12:21 GMT
mcuelenaere: with queues there will be some sort of looping anyway, something like "while(queue_empty) yield()", or even "while(queue_empty) queue_wait_tmo()". Maybe that's not the same thing. Anyway, I'm not there yet. I'm trying to do a coarse code sort before starting refinements.
Comment by Maurus Cuelenaere (mcuelenaere) - Friday, 30 October 2009, 14:48 GMT
Does it? Doesn't queue_wait() block/sleep indefinitely till someone posts a message on that queue (which wakes up the thread)?

If not, wakeup objects should be used (which definitely have that behaviour).

Loading...