Rockbox

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

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#6145 - Pitch screen: up/down by half tone

Attached to Project: Rockbox
Opened by Alexander Levin (fml2) - Sunday, 08 October 2006, 17:08 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 patch allows to change pitch not only by a certain amount of percents but also in halftone steps. Pitch changing mode is toggled by pressing MODE button while in the pitch screen.

Key mapping is only provided for H1xx since I don't know which buttons are the best candidates for this on other players.

Also a small bug is corrected reg. nudging: if nudging would exceed the allowed range then it's not executed. But on button release, the "denudging" was always executed -- regardless whether the nudging was previously applied.

TODO:
- the active mode should somehow be displayed on the screen (an asterisk?)
- half tone pitching is not very accurate, the error is accumulated. But for simple pupose of transposing a piece it's OK.

I'm very interested in your opinions about this patch.
(application/octet-stream)    pitch_halftone.patch (8.6 KiB) 
 apps/action.h                   |    1 
 apps/screens.c                  |  132 +++++++++++++++++++++++++++++-----------
 apps/keymaps/keymap-h1x0_h3x0.c |   21 +++---
 3 files changed, 110 insertions(+), 44 deletions(-)

This task depends upon

Closed by  Linus Nielsen Feltzing (linusnielsen)
Monday, 06 November 2006, 11:12 GMT+2
Reason for closing:  Accepted
Comment by Alexander Levin (fml2) - Monday, 09 October 2006, 23:13 GMT+2
Updated the patch:

- on large screens, the mode is displayed (Pitch Up/Down or Semitone Up/Down). On very small screens, there is no mode indicator. The right word is 'semitone', not 'halftone'.
- in semitone mode, repeat is switched off
- pitch mode is retained between 'pitch sessions'

TODO: semitone pitching is still not very accurate. Pitching up from 100% and then down will not result back in 100%. But for my purpose (comfortable strumming while listening to music) this is enough.

It's now committable IMHO.
(application/octet-stream)    pitch_semitone.patch (9.6 KiB) 
 apps/action.h                   |    1 
 apps/screens.c                  |  152 ++++++++++++++++++++++++++++++----------
 apps/keymaps/keymap-h1x0_h3x0.c |    1 
 apps/lang/english.lang          |   28 +++++++
 4 files changed, 144 insertions(+), 38 deletions(-)

Comment by Alexander Levin (fml2) - Tuesday, 10 October 2006, 13:40 GMT+2
PS. E.g. going one semitone up from 100.0% results in 105.9%. Then, going one semitone down results in 99.9% (and not 100.0%). This is due to the fact that pitch is held as an int.
Comment by Barry Wardell (barrywardell) - Tuesday, 10 October 2006, 20:27 GMT+2
Would it be possible to just use a hardcoded array of values for the percentage, rather than calculating each time and having to deal with round off errors?
Comment by Alexander Levin (fml2) - Tuesday, 10 October 2006, 22:45 GMT+2
> Would it be possible to just use a hardcoded array of values for the percentage, rather than calculating each time and having to deal with round off errors?

No, I'm afraid no. Since you can change pitch in normal mode a bit, then switch to semitone mode, and then further fine adjust the pitch in normal mode.
Comment by Alexander Levin (fml2) - Tuesday, 10 October 2006, 22:46 GMT+2
Further updated the patch:

- solved the problem with inaccurate semitone pitch change. Now it's really accurate.
- added key mapping for main device

TODO: key mapping for remote. I can't and will not do that since I've never used the remote. Anyone?

Another note: on H120, reset to 100% on the main unit is mapped to PLAY. Why? Wouldn't it be better to do it with NAVI? This would also allow to leave the pitch screen with PLAY.
(application/octet-stream)    pitch_semitone2.patch (14.8 KiB) 
 apps/action.h                   |    1 
 apps/screens.c                  |  153 ++++++++++++++++++++++++++++++----------
 apps/keymaps/keymap-gigabeat.c  |    5 -
 apps/keymaps/keymap-h10.c       |    1 
 apps/keymaps/keymap-h1x0_h3x0.c |    1 
 apps/keymaps/keymap-ipod.c      |    1 
 apps/keymaps/keymap-ondio.c     |    1 
 apps/keymaps/keymap-recorder.c  |    1 
 apps/keymaps/keymap-x5.c        |    1 
 apps/lang/english.lang          |   28 +++++++
 10 files changed, 153 insertions(+), 40 deletions(-)

Comment by Steve Bavin (pondlife) - Wednesday, 11 October 2006, 08:13 GMT+2
Why not have a seperate pitch offset, then look that up from a table of 12 semitones and add it to the current pitch value? You could of course double/halve for octaves (if that much range is needed).
Comment by Alexander Levin (fml2) - Wednesday, 11 October 2006, 19:17 GMT+2
Committers? Anyone? Is the patch not good enough? I'm afraid if it doesn't get committed fast something will become stale and will require much effort to keep up. (That probably happens to 90% of submitted patches.)
Comment by Linus Nielsen Feltzing (linusnielsen) - Thursday, 12 October 2006, 09:03 GMT+2
We're not too happy about floating point operations in the core code. Use fixed point.
Comment by Alexander Levin (fml2) - Thursday, 12 October 2006, 17:07 GMT+2
> We're not too happy about floating point operations in the core code.

Why not? Is it because not all devices (CPUs) support it? Because a performance issue it is definitely not.
Comment by Thom Johansen (preglow) - Thursday, 12 October 2006, 18:15 GMT+2
None of our CPUs support it, so emulation code needs to be linked in, which makes both bigger and slower code than necessary. We use fixed point absolutely every other place where math like this is done, so we should do it here too.
Comment by Linus Nielsen Feltzing (linusnielsen) - Thursday, 12 October 2006, 18:59 GMT+2
None of the targets so far support floating point natively. I agree that it is hardly a performance issue in this case, however. I suspect that the binary size increases somewhat when you include floating point code.
Comment by Alexander Levin (fml2) - Thursday, 12 October 2006, 21:33 GMT+2
Updated the patch: replaced floating point arithmetics with integers (well, actually long). I hope, unsigned long is at least 4 bytes long on all platforms.

Here are the pitches (as speed promille) when changing it in semitone steps (for quality assurance :-):

Up: 1059, 1122, 1189, 1260, 1335, 1414, 1498, 1587, 1681, 1781, 1887, 1999
Down: 944, 891, 841, 794, 749, 707, 667, 630, 595, 562, 530, 500
(application/octet-stream)    pitch_semitone3.patch (15.5 KiB) 
 apps/action.h                   |    1 
 apps/screens.c                  |  170 +++++++++++++++++++++++++++++++---------
 apps/keymaps/keymap-gigabeat.c  |    5 -
 apps/keymaps/keymap-h10.c       |    1 
 apps/keymaps/keymap-h1x0_h3x0.c |    1 
 apps/keymaps/keymap-ipod.c      |    1 
 apps/keymaps/keymap-ondio.c     |    1 
 apps/keymaps/keymap-recorder.c  |    1 
 apps/keymaps/keymap-x5.c        |    1 
 apps/lang/english.lang          |   28 ++++++
 10 files changed, 170 insertions(+), 40 deletions(-)

Comment by Alexander Levin (fml2) - Thursday, 12 October 2006, 21:45 GMT+2
Sorry, forgot one thing: the constant should be used for initialising the pitch_mode
(application/octet-stream)    pitch_semitone3_2.patch (15.5 KiB) 
 apps/action.h                   |    1 
 apps/screens.c                  |  170 +++++++++++++++++++++++++++++++---------
 apps/keymaps/keymap-gigabeat.c  |    5 -
 apps/keymaps/keymap-h10.c       |    1 
 apps/keymaps/keymap-h1x0_h3x0.c |    1 
 apps/keymaps/keymap-ipod.c      |    1 
 apps/keymaps/keymap-ondio.c     |    1 
 apps/keymaps/keymap-recorder.c  |    1 
 apps/keymaps/keymap-x5.c        |    1 
 apps/lang/english.lang          |   28 ++++++
 10 files changed, 170 insertions(+), 40 deletions(-)

Comment by Alexander Levin (fml2) - Thursday, 12 October 2006, 22:14 GMT+2
PS. The values above are computed when starting at 100.0%. If you change the pitch in normal mode and then switch to semitone mode, you'll of course have other values.
Comment by Alexander Levin (fml2) - Thursday, 12 October 2006, 22:21 GMT+2
One more small correction: use 'uint32_t' instead of 'unsigned long.'
(application/octet-stream)    pitch_semitone3_3.patch (15.5 KiB) 
 apps/action.h                   |    1 
 apps/screens.c                  |  170 +++++++++++++++++++++++++++++++---------
 apps/keymaps/keymap-gigabeat.c  |    5 -
 apps/keymaps/keymap-h10.c       |    1 
 apps/keymaps/keymap-h1x0_h3x0.c |    1 
 apps/keymaps/keymap-ipod.c      |    1 
 apps/keymaps/keymap-ondio.c     |    1 
 apps/keymaps/keymap-recorder.c  |    1 
 apps/keymaps/keymap-x5.c        |    1 
 apps/lang/english.lang          |   28 ++++++
 10 files changed, 170 insertions(+), 40 deletions(-)

Loading...