Rockbox

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

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#10739 - Playback.c refactor

Attached to Project: Rockbox
Opened by Jeffrey Goode (Blue_Dude) - Friday, 30 October 2009, 00:49 GMT+2
Last edited by Jeffrey Goode (Blue_Dude) - Sunday, 01 November 2009, 01:13 GMT+2
Task Type Patches
Category Music playback
Status Closed
Assigned To No-one
Player Type SW-codec
Severity Low
Priority Normal
Reported Version Release 3.4
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
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. ;)
   playback refactor.patch (188 KiB) 
 apps/cuesheet.c                     |    2 
 apps/recorder/recording.c           |    1 
 apps/audio_thread.c                 | 2157 +++++++++++++++++++++++++++
 apps/audio_thread.h                 |   83 +
 apps/settings.c                     |    2 
 apps/gui/skin_engine/skin_parser.c  |    2 
 apps/gui/skin_engine/skin_tokens.c  |    2 
 apps/gui/skin_engine/skin_display.c |    2 
 apps/gui/wps.c                      |    2 
 apps/menus/settings_menu.c          |    2 
 apps/menus/playback_menu.c          |    2 
 apps/metadata.c                     |    1 
 apps/mpeg.c                         |    2 
 apps/talk.c                         |    3 
 apps/codec_thread.c                 |  760 +++++++++
 apps/codec_thread.h                 |   30 
 apps/common_thread.c                |   46 
 apps/common_thread.h                |   97 +
 apps/playback.c                     | 2792 ------------------------------------
 apps/playback.h                     |   85 -
 apps/voice_thread.c                 |    2 
 apps/SOURCES                        |    4 
 apps/plugin.h                       |    2 
 apps/iap.c                          |    2 
 apps/debug_menu.c                   |    7 
 apps/scrobbler.c                    |    3 
 apps/main.c                         |    2 
 apps/misc.c                         |    1 
 apps/tagtree.c                      |    1 
 29 files changed, 3193 insertions(+), 2904 deletions(-)

This task depends upon

Closed by  Jeffrey Goode (Blue_Dude)
Sunday, 01 November 2009, 01:13 GMT+2
Reason for closing:  Accepted
Additional comments about closing:  Committed as r23444
Comment by Jeffrey Goode (Blue_Dude) - Friday, 30 October 2009, 03:20 GMT+2
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.
   playback refactor.patch (107 KiB) 
 apps/recorder/recording.c |    1 
 apps/metadata.c           |    1 
 apps/mpeg.c               |    2 
 apps/talk.c               |    3 
 apps/codec_thread.c       | 2164 +---------------------------------------------
 apps/codec_thread.h       |   62 -
 apps/playback.c           |  855 ++----------------
 apps/playback.h           |   51 -
 apps/SOURCES              |    1 
 apps/plugin.h             |    2 
 apps/debug_menu.c         |    5 
 apps/scrobbler.c          |    3 
 apps/misc.c               |    1 
 apps/tagtree.c            |    1 
 14 files changed, 244 insertions(+), 2908 deletions(-)

Comment by Jeffrey Goode (Blue_Dude) - Friday, 30 October 2009, 04:17 GMT+2
Bug fix. Missing #defines etc.
   playback refactor.patch (106.6 KiB) 
 apps/recorder/recording.c |    1 
 apps/metadata.c           |    1 
 apps/mpeg.c               |    2 
 apps/talk.c               |    3 
 apps/codec_thread.c       | 2164 +---------------------------------------------
 apps/codec_thread.h       |   62 -
 apps/playback.c           |  853 ++----------------
 apps/playback.h           |   51 -
 apps/SOURCES              |    1 
 apps/plugin.h             |    1 
 apps/debug_menu.c         |    5 
 apps/scrobbler.c          |    3 
 apps/tagtree.c            |    1 
 13 files changed, 243 insertions(+), 2905 deletions(-)

Comment by Maurus Cuelenaere (mcuelenaere) - Friday, 30 October 2009, 12:42 GMT+2
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, 13:15 GMT+2
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, 13:21 GMT+2
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, 15:48 GMT+2
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...