Rockbox

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

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#12189 - Simplify the codec API and don't always loop MODs.

Attached to Project: Rockbox
Opened by Sean Bartell (wtachi) - Monday, 11 July 2011, 09:26 GMT+2
Task Type Patches
Category Codecs
Status Unconfirmed
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 0%
Private No

Details

This patch makes some changes to codec_api to help remove dependencies for my project.

The unused strcasestr callback is removed.

The global_settings pointer was being used by several codecs to check whether REPEAT_ONE mode is on; this has been replaced with a callback "should_loop()".

The MOD codec has been modified not to restart the file when it reaches the end. This does not affect MOD files that use "position jump" to loop part of the song. Ideally the MOD codec would only follow such loops if should_loop() returns 1, but detecting loops is nontrivial.
   0001-Simplify-the-codec-API-and-don-t-always-loop... (10.3 KiB) 
 b/apps/codec_thread.c       |    7 +++++++
 b/apps/codecs.c             |    4 ++--
 b/apps/codecs.h             |   11 ++++-------
 b/apps/codecs/adx.c         |    2 +-
 b/apps/codecs/mod.c         |    9 +++++----
 b/apps/codecs/nsf.c         |   13 +++++--------
 b/apps/codecs/spc.c         |   10 +++++-----
 b/apps/plugins/test_codec.c |    9 +++++++--
 8 files changed, 36 insertions(+), 29 deletions(-)

This task depends upon

Comment by Sean Bartell (wtachi) - Wednesday, 17 August 2011, 18:53 GMT+2
Updated for libgme and to make the MOD fix actually work.
   0001-Simplify-the-codec-API-and-don-t-always-loop... (14.7 KiB) 
 b/apps/codec_thread.c       |    7 +++++++
 b/apps/codecs.c             |    4 ++--
 b/apps/codecs.h             |   11 ++++-------
 b/apps/codecs/adx.c         |    2 +-
 b/apps/codecs/ay.c          |    6 +++---
 b/apps/codecs/gbs.c         |    4 ++--
 b/apps/codecs/hes.c         |    4 ++--
 b/apps/codecs/kss.c         |    2 +-
 b/apps/codecs/mod.c         |   27 +++++++++++++++------------
 b/apps/codecs/nsf.c         |    4 ++--
 b/apps/codecs/sgc.c         |    2 +-
 b/apps/codecs/spc.c         |   10 +++++-----
 b/apps/codecs/vgm.c         |    2 +-
 b/apps/plugins/test_codec.c |    9 +++++++--
 14 files changed, 53 insertions(+), 41 deletions(-)

Comment by Nils Wallménius (nls) - Tuesday, 30 August 2011, 15:33 GMT+2
I don't see why the mod change is relevant here and i think it should be separated out and submitted as a separate patch otherwise this looks good to me
Comment by Andree Buschmann (Buschel) - Tuesday, 30 August 2011, 16:03 GMT+2
I also think the mod-changes should be handled separately. Removing global_settings from the codec interface is a good thing imho, but the naming "should_loop()" is a bit strange... "repeat_track()" or "loop_track()" better fits to my personal taste.
Comment by Andree Buschmann (Buschel) - Tuesday, 30 August 2011, 21:58 GMT+2
The changes to codec API have been submitted with r30391. The mod changes are left (see attached patch).
   mod_v3.patch (2.4 KiB) 
 apps/codecs/mod.c |   27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

Comment by sideral (sideral) - Wednesday, 31 August 2011, 10:02 GMT+2
The codec API change (remove global_settings pointer from codec interface) breaks FS#11891 (Add mp3 gap skipping support to improve gapless playback).

I understand the motivation for removing the global_settings member. However, that change also removes a generic mechanism for configuring codecs. What would now be a good way to configure a codec – either in a generic way, or if that's impractical, in this specific case (boolean flag to turn codec-specific gap elision on or off)?
Comment by Andree Buschmann (Buschel) - Wednesday, 31 August 2011, 11:49 GMT+2
We might discuss this general topic via ML or on irc. If you want to update FS#11891 you might just add a dedicated function similar to "loop_track()".
Comment by Michael Sevakis (MikeS) - Wednesday, 31 August 2011, 16:21 GMT+2
One idea: Use a codec configuration struct that isn't global_settings, passed from the codec to a function in the codec api, that returns config parameters. In a core build, they can copy from global_settings or anywhere else, outside the core in some app, they may be filled-in however the program determines is best.

Loading...