Rockbox

Hello! I like the patch, at least how the UI feels now on H120. I think we could still discuss whether the screen should be replaced by two independent menues as proposed by Paul Louden. But it's another matter.

Some remarks:

1. In the semitone mode, if I go up to the limit, I get 199.9% and not 200.0% It's not that much, but I'd just expect to see zeroes for an octave

2. I don't know if and how it's related to this patch, but if I increase the speed to 120% and lower the pitch to 50%, the playback in simulator stops. After I change the values towards 100% playback is started again. When I close the simulator and start it again, the playlist position is not the same it was before shutdown. (Normally, it's saved between "sessions".)

3. The patch requires an additional key to independently switch semitone and time stretch modes. While this is good in general, it might be quite challenging to find free keys on other targets H120 is good in this regard as it has many buttons.

One more question: why did you have to increase the precision of the pitch to two digits after the decimal point (in percent notation)? Doesn't hardware allow only one digit anyway?

The most obvious problem here is that most targets have less buttons, not more.

Try designing the screen to work around five buttons, for ideal minimization. Or five buttons and a scroll wheel. Ideally you could just test it on a simulator for an iPod. To be best, I'd go for an iPod Video sim and an iPod 3rd Gen sim. Both have the same number of buttons, but positioned radically differently (useful for considering whether they physical layout of the screen is still intuitive to the controls - a real problem for the pitchscreen).

The main reason I suggested replacing it with normal list style menus is because of these radical differences in physical controls and layout that make the layout of the screen more or less impossible to easily align with how the actual controls are positioned on the player.

Regarding semitone mode reaching 199.9%, yes, I had noticed that. I already upped the semitone precision to 64 bits (not sure if that's a no-no, but the 64 bit math only happens when increasing or decreasing by semitone, so it shouldn't realy impact runtime), so it would be easy to add one more digit of precision to the semitone code. That might get it to correctly hit 200%. On the other hand, it may be an issue of the pitch/speed precision (currently set at 2 decimal places), in which case I might be able to up it one more digit (I'll have to see if the math works out for that – I'd rather not have to increase _those_ to 64 bits!).

I suppose one option would be to have semitone mode go in increments – display the number of semitones, rather than the percentage. The only thing you'd lose with that is the ability to adjust a little bit by percentage, and then adjust by semitones from there. You could still get the same effect, though, by first adjusting be semitone and then by percentage. Another disadvantage is that you might have to change the pitch adjustment when entering semitone mode in order to restrict it back to semitone increments.

Regarding the sound cutting out with certain settings, yes, I've noticed that too. I haven't been using the base code for a while, but I don't think my changes would affect that problem. I would think that the same thing would happen in the core code, that it's an issue with the time stretching code. If Pondlife is reading this, perhaps you could coment?

I increased the pitch precision because I wasn't getting consistent results when calculating the speed or pitch as a product of the other and stretch. The problem was that I was that the "limit" was inconsistent. My could would say "okay, you've reached the limit", but then it would still be possible to adjust further on the other parameter. So I bumped up the precision and the problem was solved.

Regarding number of buttons: I suppose it would be easy to bind all the "modes" to one button – perhaps specifically for targets with fewer buttons, keeping the two button thing for the platforms that can handle it. I really like having them bound separately, but I see the point about it being a problem on other players. Already the pitch screen required six buttons – arrows, reset, and mode. Not sure how that was handled on platforms with fewer buttons. I think I like the idea of making it all into more of a menu-type thing, but I didn't want to rock the boat too much with this patch. And on the other hand, I think the direction-oriented controls as they stand are fairly intuitive.

On the other hand, on screens like this I always forget what the mode and record buttons are supposed to do, so I'm not super excited about even using them at all.

To 199.9%: the current code manages to calculate the values exactly, using just 32 bit ints. I don't quite understand why it's necessary to use 64 bit ints.

I'd welcome displaying the pitch in semitones (or finer, but semitone based units). But this would require a more massive rework of all this area. I'd like to have it list based (as suggested by Paul Louden). I'd have a setting for enabling/disabling time stretch (I'd actually name it "independent changing of pitch and speed", as "time stretch" seems a bit too technical to me) and then, depending on the value of this setting, either one or two other settings.

If "timestretch" is disabled, then I'd have the setting "Pitch/Speed" (or "Rate" or … you name it). The settings allows to change the speed and pitch (together) in 0.1% steps. If it's enabled then I'd have two settings. "Speed" would allow to change the speed in 0.1% steps, and "Pitch" would allow to change the pitch in 0.5 semitone steps. The pitch would always "snap" to these values. I.e. it wouldn't be possible (unlike the current SVN) to have a value between two 0.5semitone values. But IMO this is enough.

Having it this way would give you full control of what and how you change, while not require many buttons. Just the standard ones are used.

To the consistent results: wouldn't it be better to have the speed and pitch as "primary values" and calculate the stretch factor as needed? IIUC, now the speed is calculated from the pitch and the stretch, which gives inconsistent results for the speed. If we'd calculate the stretch instead, the results for the pitch and the speed would always be consistent, and for the stretch probably not. But it's not important at all since the stretch is an internal (non observable by the user) value.

199.9%: 64 bit math is necessary because with the new higher-precision speed and pitch variables the math was overflowing when calculating semitones. Some of the intermediate values were in the 10s of billions, which is beyond the capacity of a 32 bit integer.I'm starting to think, though, that it might be easier just to keep track of semitone percentages with a table rather than a calculation….

I had thought about using some text other than "timestretch" but I couldn't think of anything that would fit nicely in the limited space available. "Independent changing of pitch and speed" seems too big for our small screens.

I definitely disagree with only allowing semitone changes when in timestretch mode. I can think of plenty of situations where I'd want to do non-semitone pitch adjustments without affecting speed.

In this patch speed and pitch _are_ primary values. Stretch is calculated anew whenever speed or pitch is modified. I only ever calculate speed or pitch based on stretch when I hit the upper and lower stretch limit. For instance, if you're adjusting pitch up and you hit the stretch limit (STRETCH_MIN), it'll calculate the maximum possible pitch by using the current speed and STRETCH_MIN and use that value instead of the value that the user was aiming for. In semitone mode, though, it just doesn't do the change, since calculating the max pitch like that would put it at a non-semitone value.

Here's my input, for what it's worth (all IMHO).

1) When using semitones, could we display the pitch as x semitones + x.x% - the latter part only being displayed if it's non-zero (which won't happen if users purely shift by semitones).
2) I'm probabaly misunderstanding, but in semitone mode we're only able to get within one semitone of 200%, so 199.9% is quire possibly correct and better than hard-limiting to 200% especiially combined with point 1.
3) Semitones should move in whole semitone steps (not 0.5 semitones)
4) I don't see much point of increasing the precision of variables with the current stretch algorithm Max pitch is 200.0% (2000), max stretch is 250% (250), so max speed (which is pitch * stretch) = 500000. Even if we use permille for stretch too we're still only hitting 5000000 so 64 bit should not be needed.
5) Personally, I'd rather not persist the speed and pitch settings as they're normally per-track, not global, but it's not a biggie as reset is easy enough. I do think the mode settings should be persisted though.
6) I'd rather keep with a single mode button (i.e. 4 modes) than use any extra buttons. (But do keep the modes as independent settings, or maybe a bitset would save some memory?)

Like I said, all IMHO, and all open for discussion…

199.9%: 64 bit math is necessary because with the new higher-precision speed and pitch variables the math was overflowing when calculating semitones

Ok, now I understand

"Independent changing of pitch and speed" seems too big for our small screens

Yes, that's true! I just would try to come up with a more "human friendly" and a less technical description. And "independent ….." is what it means.

As for semitones: Could you describe me a situation where you would want to adjust the pitch (but not the speed!) in steps less than 0.5 semitone? Or a 0.25 semitone? I think, only a well trained ear could hear the difference. What I mean is that we should use semitone based units (just choose them fine enough). IMO 0.5 semitone would be fine enough. Personally I can't think of a scenario where I'd need finer control.

To STRETCH_MIN: is this value dictated by how the stretching algorithm is implemented? If yes, couldn't we (if it's possible of course!) choose the pitch and speed limits so that the stretch limit would never be touched? Because if it limits some changes, it's very hard for a user to see why there's a limit since the stretch is a non visible value. E.g. I know I can go to as high as 200% in pitch, but am allowed to go just to 190%. Why? I couldn't understand that without knowing the technical details. I.e. I'd rather have a little bit smaller, but *rectangular* area (in the speed-pitch-coordinates) than a bit larger area but of a cloudy form.

When using semitones, could we display the pitch as x semitones + x.x% - the latter part only being displayed if it's non-zero (which won't happen if users purely shift by semitones).

Mathematically, it's definitely possible! But it would require a change in the implementation (a table of factors corresponding to semitones whould have to be stored somewhere). But if we'd use a semitone based scale this wouldn't be needed. Semitones are just a more natural units for measuring pitch than % (like dB and % for volume) IMO.

As for semitones: Could you describe me a situation where you would want to adjust the pitch (but not the speed!) in steps less than 0.5 semitone? Or a 0.25 semitone?
Maybe you want to practise piano (or other hard-to-tune instrument) alongside some music that's not in tune (i.e. has been sped up a little during production - ever try any Beatles?).

I can think of 3 use cases for the pitch screen (and I'm sure there are more that I can't think of):
- Musicians - modify rate (or pitch) to play along with something in tune, or reduce speed to work out a tricky part
- DJs - modify rate (or speed) to beatmatch
- Audiobook users - increase speed to listen to spoken word faster without any chipmunk effect.

In the first two cases, rate is generally the preferred method as the (current) timestratch algorithm does horrible things to music (especially bass guitar). DJs might use timestretch, but more as an effect, I'd think. Anyone who considers themself an audiophile will not use timestretch (at the moment at least).

We need to store pitch as a percentage/permille internally so people can mix and match with timestretch and rate changes, right?

- Musicians - modify rate (or pitch) to play along with something in tune, or reduce speed to work out a tricky part

There are two use cases there (modify pitch to play in tune, reduce speed to play a tricky part) – which I use mostly (I'm not a DJ). And I'd never need steps less than 0.5 semitones.

We need to store pitch as a percentage/permille internally so people can mix and match with timestretch and rate changes, right?

I'd suppose yes, but it's debatable. We can consider the two modes (with and without time stretch) as fully independent. This would solve some problems (no need to match) but would also lead to some funny/unexpected effects (jump in pitch/speed) if the switch would be done "live". Now we need to do a reboot so that it wouldn't be that bad.

Now we need to do a reboot so that it wouldn't be that bad.

Not true. As long as timestretch is available (i.e. was enabled at the last reboot) you can use both rate and timestretch at the same time (i.e. in combination). This ought to be maintained, I think. There's no reason to have one-or-the-other.

I have had some overlapping improvements (better variable names, allowing use of timestretch + semitones) sat on my hard drive for a while and would like to commit them.

David, would you mind if I commit? You'll have a bit of resync work to do, but hopefully it'll make it easier for you to add the other UI improvements (especially the mode display stuff).

Later , I'd like to add some voice support - not reading the numbers (as that would get in the way of the actual use of the pitchscreen), but to speak the mode upon first entry and whenever it's changed.

Sure, go ahead and make your commit. I'll endeavor to integrate everything correctly and post a new patch.

Semitones + x%: Maybe this would work. I wouldn't be sure whether to then change pitch by whole semitones or to change to the nearest semitone in the appropriate direction. Also, as fml2 mentioned, it would probably require a table of semitone values, which I don't think would be a horrible thing. From 50% to 200% there would only be about 24 entries, at 2 bytes each. In fact, the savings of code might even make up for that difference….

In semitone mode 200% should be one of the exact values. 200% is 12 semitones above 100%. The 199.9% value is because of rounding errors in the calculations. A table would fix this, as would bumping up the precision of one or more calculations.

Re 64 bit variables: I only used 64 bits in calculating the semitone values. High precision is needed there to get accurate results. When I increased speed and pitch to two decimal places it pushed things over the edge and I had to go from long to long long – but just for the semitone calculation variables. 64 bit is not needed, and is not used, for anything other than semitone calculation. Speed, pitch and stretch are all int32_t. The reason I brought those up to 32 bits is because I wasn't able to get a consistent calculation of where the "limit" was.

Speaking of "limit": I'd be happy to get rid of it, and keep the speed/pitch domain rectangular. I wasn't sure what lower or higher values would do to pitch and speed, though, so I was timid about messing with those. Furthermore I didn't want to eliminate the ability to adjust either value down to 50% and up to 200% (which I think would be acceptable limits for both, if we have to have limits). That's why I put in that little "limit" indicator – to let the user push the algorithm to its limit, and then know why they can't go any further. I agree that it's potentially confusing, though, and I'd love to find a better solution.

I totally agree about persisting the mode settings and not the numerical values, and that's how this patch has it set up. Mode is saved but numbers are reset when rebooting. That said, I was also thinking it might be nice to have a menu item that lets the user choose to persist the numbers, or perhaps choose to have them reset when moving to a new song?

Seems like the consensus is to change it back to one mode button, so I'll do that, and just cycle through the four "super-modes".

Smaller-than-semitone pitch adjustments: Pondlife gave the exact example I was thinking of: playing along with a Beatles song. Some of their recordings were tweaked slightly, and I'd reather not have to retune my piano to play along with them.

I disagree about rate being the preferred method for musicians playing along with music. While I wouldn't use the current timestretching algorithm in performance, I find it quite adequate for practice. Most of the time I expect I'll probably stick with speed-only changes for learning a song, occasionally also modifying pitch if the song isn't in my range and I'm using a capo on my guitar. I think it would be rare for me to want the same percentage at the same time for pitch and speed modifications. So I guess the difference really is between practice and performance mentality. Performance seems more applicable to DJs (which I have no connection to and little opinion about).

Re storing pitch as percent in semitone mode: If we used a lookup table, I was figuring we'd store both the current speed and the current semitone index. We'd just have to recalculate the semitone index when entering semitone mode.

Smaller-than-semitone pitch adjustments: Pondlife gave the exact example I was thinking of: playing along with a Beatles song
I'm not against smaller-than-semitone pitch adjustment, but I'd like to have only semitone-based units, i.e. not just 0.1%. 0.5 semitone would be fine enough IMO.

I disagree about rate being the preferred method for musicians playing along with music
I agree with this disagreement!

Most of the time I expect I'll probably stick with speed-only changes for learning a song, occasionally also modifying pitch if the song isn't in my range and I'm using a capo on my guitar.
These are exactly the two cases I use the feature for. I'm not a DJ and hence don't use the "rate" mode (which seems to be needed mainly by DJs). But exactly because of this reason I'd prefer to have lists for it.

Having said that, I'd like to add that I like what the patch does. All my concerns written above (in earlier posts in this FS task), i.e. organizing the control in menu like structures and throwing the pitch screen away are out of scope of this patch and should be discussed on the mailing list. Here we should only concentrate on what this patch is about. And, IIRC, the primary goal was to correctly display the speed as the speed perceived by the user (and not some internally used values).

OK, my committing is done for now (I think), take it away!

I disagree about rate being the preferred method for musicians playing along with music
> I agree with this disagreement!
As David said - practise vs performance. If you use rate, the audio quality is maintained. If you use timestretch, then not only is the output mono, but basslines get horribly mangled (try almost anything off "Pet Sounds" if you want an example!). Now play that mangled stuff through a PA… (This rationale only holds with the current algorithm, of course.)

About limits; I think that the current "rectangular" limits are too extreme for the dsp code - hence the problems where audio goes silent, as also reported on  FS#10321 . It might get even worse if you're playing stuff that needs further resampling, but I've not had time to look into it yet. It would be useful to find out more details, and fix up the dsp code (if that's practical) or clamp the UI better (if needed).

I do think a semitone table (at least for one octave) is the way to go - indeed for both octaves if it results in simpler/smaller code. We then always work on permille internally, then lookup when displaying the pitch setting in semitone mode to obtain x semitones + y.y% format.

Actually, I've found that the audio is still stereo when timestretching is taking place. Am I going crazy? Is that impossible? I don't have my player with me to verify again at the moment….

Also, I was thinking something as I was changing back to using a single mode button: it might be a little annoying to have to switch back to rate mode in order to go from timestretch+semitone to timestretch+percent, since (the way it's written in my patch) it'll reset speed to be equal to pitch. I could make it so that it doesn't do that reset until you start modifying the rate, but I'm not sure if that would be intuitive or make sense to the user.

I'm also thinking about people with scroll wheels, who've said that the current pitch screen is hard to use. I feel like changing over to a list-based format may be the only way to make things better for them. Then they could scroll to the thing they want to change, select, scroll to modify the value, and select to finish. We could also completely do away with the various modes, and just have different entries in the list for each thing, like this:

Rate: 100.0%
Speed: 100.0%
Pitch: 100.0%
Semitone: +0.00
Reset

or

Rate: 132.3%
Speed: 132.3%
Pitch: 132.3%
Semitone: +4.87
Reset

of

Rate: — Speed: 85.7%
Pitch: 132.3%
Semitone: +4.87
Reset

Also, not that I made the semitone readout be semitones plus cents, which I think makes more sense than percent. That said, I expect it'll be a bit of a calculation to get cents. If I've done my math right, the number of cents that pitch x is above pitch y is 1200 * log2(x/y). Does rockbox even have an integer-based log function available? This makes me wonder if I'd to need to switch things over to 16.16 bitwise fixed point math rather than the decimal stuff we're doing now. But I think expressing pitch as semitone.cents might be worth it….

-David

Would all four values be displayed on one screen? You then choose an entry (with UP/DOWN) and change the value (with LEFT/RIGHT). If timestretch is disabled then only the first entry (rate) would be selectable. Or, maybe, all other entries would be adjusted if one entry is changed.

If that is what you're proposing wouldn't it be easier to just use standard settings lists for that?

Oy. I've been trying to work out how to do semitones and cents, but there's really no way around it: it would require either a big ol' table of relative pitch (one with 25 entries for each semitone from 50% to 200%, and another with 100 entries or some division thereof for the cent proportions) or a log function and a power function. Log isn't that hard, but power looks to be more than I want to tackle.

Actually, I'm wondering now if the idea of a table isn't as bad as I was thinking. I could probably just do another 25-entry table and do linear interpolation for the missing entries and it would probably be Close Enough(TM).

I dunno. What does everyone else think? I feel like having semitones and cents would be nice (from a musician's perspective) but where do you draw the line between features and program size?

Alexander: yes, I was thinking all the values would be on the same screen. Everything would be updated correctly if a value was changed that affected others. Unfortunately we'd probably still need a semitone mode, since both rate and timestretch should be modifiable via semitone.

And, with a little experimentation with a spreadsheet, it turns out the maximum deviation between true cent values and linear interpretations between semitones is about .04%. That's about the size of a single cent, so with dumb linear interpretation the cent readout would be accurate +/- 1 cent. Adding in a few subdivisions, storing a table with four values (20, 40, 60 and 80 cents) gets the maximum deviation down to 0.00176 – nicely beyond the precision of the cent unit. That assumes that the values stored in the approximation table are high enough precision to not mess much with the interpolation precision (shouldn't be a problem).

So yeah, that's probably the way to do it.

Also, it occurs to me that the main semitone table could be half the size I was originally thinking, since 0→12 (100% →200%) will be exactly double the values of -12→0 (50% → 100%).

I too think that a table of the log values corresponding to semitones (0 to +12) would be a good solution. (BTW: the log values for -12..0 are not the doubles but rather negated values of 0..+12).

Would using semitone mode cause the values to "snap" to that table? As of current SVN, using semitone mode does not snap. It's just a multiplication. I could live with both (snap and not snap), just wanted to clarify.

Since S = 12 log2(rate) (S – number of semitones, log2 is log with basis 2, i.e. log2(x)=log(x)/log(2), rate is the pitch rate as a number, e.g. 1.5 for 150%), and from the fact that log_a(1+x) ~ x/log(a), we have the following math:

Assume that we know (from the table) that the rate value of M corresponds to exactly N semitones. We'd like to interpolate S for the rate x, where M is the next lower value. We have:

S = 12 log2(x) = 12 log2(M + d) = 12 log2(M (1+d/M)) = 12 (log2(M) + log2(1+d/M)) = N + 12 log2(1+d/M) ~/Taylor approx./~ N + 12/log(2) d/M

This is the interpolation that should be used IMO, it's not much more complicated than the linear interpolation. The factor 12/log(2) can be precomputed and defined as a constant. All math can be performed in ints, just multiply everything with, say, 10000.

Personally I'd welcome the list version of the pitch screen.

PS. Though the values for lower (<100%) values can be derived from those >100%, a simpler solution would be to store them all IMO. Less code, less computation (rounding) errors etc.

I see your point re halving the table size – probably not worth it.

If I understand your math correctly, the final formula you suggested IS a linear interpolation: N + 12 / log(2) * d / M, or Ad+B, where A and B are constants and d is the variable. That won't end up improving the error. Am I misunderstanding you somehow? Perhaps including the second term of the Taylor series (x^2 / 2) would help the accuracy? I guess I'd have to do another spreadsheet to figure out how much that would help.

Hrm…. Yes, it's a linear interpolation! But A and B are not constants. A is the nearest known value, B is the f'(x0). So it's just a Taylor series with only the linear part. I don't think higher terms are necessary.

I meant that A and B are constants for a given semitone interval. The max error of the plain linear approach is about .04% without taking rounding errors into account. That's equal to about one cent. The single term Taylor approximation gives an error of almost 3 cents at S=100. With rounding errors I'm able to get a maximum error of .01% with just four entries in an interpolation table (about a quarter of a cent). After doing some experimentation in excel I've found that adding in the second term of the Taylor series – -(x^2 / 2) – brings the max error of the Taylor method down to .1 cents (without accounting for rounding error).

And it just occurred to me that we're really talking about opposite functions. Yours is for finding % from semitone while mine (at least in my mind) is for finding semitone from % (though it would also work in the other direction).

Anyway, I think we're beating a dead horse here. I munna go ahead and implement something.

Yours is for finding % from semitone while mine (at least in my mind) is for finding semitone from %

I thought that mine is also for finding semitones from % , i.e. semitone = 12 log2(rate)

Anyway, I think we're beating a dead horse here. I munna go ahead and implement something

You're absolutely correct. Ho ahead and implement how you like it. I'm sure it will be OK.

For anyone who's waiting for this, I'm sorry for the delay. I've actually got the next version working but I just need to do a compile for archos to make sure it works on a non-SWCODEC platform.

Okay, here's the new patch. The changes are:

1. Switched back to having a single mode button that toggles through all four sub-modes.
2. Added cent display in semitone mode. Currently in semitone mode it'll adjust by 10 cents at a time, or one semitone when you hold down the button. I figure this is a pretty good compromise – I can't even really head the difference between two notes 10 cents apart (though I suppose in really precise situations it would create an interference beat pattern). Cents are implemented with a lookup table with linear interpolation. I haven't checked how precise the implementation is, but if my spreadsheet is correct it should be within a quarter of a cent of correct at all times (near speed = 100%. At 50% it might be more like +/- 1/2 cent).
3. Changed "Speed" to be "Rate" in non-timestretch mode.
4. The mode settings are now saved correctly between sessions. If you're in semitone/timestretch mode and then reboot, you'll still be in semitone/timestretch mode. Speed and pitch are still not saved between sessions, though (seems like that's the consensus for how it should behave).

Everything is working in both the h120 sim and the archos recorder sim. The one weirdness is that on the recorder sim, when I hit the mode button (f1) it changes the mode correctly but it also exits the pitch screen. I don't know why it would be doing that. I don't think it's my code's fault, but I can't really check since I don't have a recorder ui sim built from unmodified code.

Anyway, let me know what you think.

   pitch_ui_improvements_v2.diff (43.1 KiB)

Great, a few points (H300 sim):
1) In the timestretch modes, increasing pitch should not change speed Changing speed alone isn't possible either.

…The rest are from a cursory look at the patch so I might be mistaken…

2) semitone_table and cent_interp should be static const.
3) at_limit should be static
4) Do you need to pass semitone to pitchscreen_draw() ? It should be able to work that out from pitch (if I understand the function of get_semitone_from_pitch() correctly).
5) I'd name the settings variables pitch_mode_semitone and pitch_mode_timestretch - I think it's clearer.
6) The settings names should be all lower case, IIRC.
7) You probably know, but for the checklist… there are some #include "debug.h" lines to remove.

Thanks for the comments.

1) I'm pretty sure it was working correctly on my h100 sim. I didn't do tests on any other sims apart from recorder. I would think h300 would behave exactly the same as h100 since (afaik) controls and hardware are extremely similar (lcd, rtc and i/o audio ports are the only significant differences?). I'll test h300 to figure out what's going on.
2) yes, of course. It's been a while since I did C (not C++) development, so I'm a little rusty on the data type modifiers. I didn't even remember that C allowed the const modifier.
3) ditto
4) I pass semitone to pitchscreen_draw() because I didn't want to muddy the waters with multiple conversions (with accompanying loss of precision). Since I'm clamping to 10 cent intervals it would be weird for it to sometimes be .01 and sometimes .99, when it's supposed to be .00
5) good idea.
6) Do you mean the strings that are displayed on the screen? If so, I suppose that's an easy change to make.
7) Yes, I had figured that out based on some check-in comments I've seen. Is there an actual check-in checklist somewhere? That could be helpful….

Also, do you have any suggestions for making the diff easier when irrelevant changes have been made in relation to other patches?

I have only briefly looked at the patch (not tested yet) and have the following comments (in addition to those by pondlife):

1. LANG_RATE as the name is a bit unspecific IMO. There may be many different rates. "Rate" is a quite common word. How about LANG_PLAYBACK_RATE or something similar?
2. I think a comment should be written that explains how the values in semitone_table have been calculated ("blah … 2^(1/12) … blah")calculated
3. There is a typo: Multibly → Multiply
4. Statement of the "else" branch should be on a separate line IMO (e.g. in "else pitch_delta = PITCH_SMALL_DELTA") Or replace those with the "?:" statement

I'll try to test it "live" and will report further then. Thank you for your work!

Okay, it turned out I messed something up in fixing everything to work on non-SWCODEC. It was just a problem in pitchscreen_draw (purely cosmetic). Anyway, here's the new patch.

   pitch_ui_improvements_v2.1.di... (43 KiB)

Okay, I think I've addressed all the comments so far. Here's my new version of the patch:

   pitch_ui_improvements_v2.2.di... (43.2 KiB)

I've tried the last patch and I must say: I like it! The app behaves as I would expect. When I use extreme values, it stucks sometimes (at least in the sim), and once I was taken to another song from the playlist, but I don't think it's something caused by the patch.

I only have the following minor comments/questions/suggestions:

1. NUM_SEMITONES: define it as sizeof(semitone_table) / sizeof(int)
2. Introduce a #define for semitone table entries (instead of repeating the complex expressions)
3. get_semitone_from_pitch: semitone should be int, not int32_t, no?
4. The first "while" in get_semitone_from_pitch would be simpler if start with semitone = 1 (I think)
5. get_pitch_from_semitone: semitone_index is not int. Is it OK (see 3)?

And: we have to also adjust the manual. Will the list based variant the next patch by you?

I think I need to bow out for the next patch. This has taken a lot more of my time than I had planned, and I have another project that I should really be working on.

I've never worked in TeX before, though I suppose I can probably figure it out from context to update the manual.

I also need to update all the language files to match english.lang. Anything else I'm forgetting?

Here is an updated patch. I implemented some of the points above. If I find the time I'll write a patch for the manual. I also added a space between ":" and the value (in Pitch: XX%)

   pitch_ui_improvements.3.diff (42.9 KiB)

Oh, I'm sorry, I didn't mean that I wouldn't do the rest of the cleanup on this. I just meant that I didn't want to do the next modification – changing it to menu form, or whatever ends up happening. I was hoping this patch could be integrated into SVN and then things would move on from there.

Anyway, thanks for picking up the ball. I'm going to go take a look at the manual source.

Yes, that is the way I understood you. But I thought the things I changed are so easy and dumb to do that I could do them myself. Making the pitch screen menu based should be done in another patch, sure.

Before I try to commit the patch: have you also experienced weird things with playlist while changing pitch/speed? On the H120 sim, the playback sometimes stops and then I get to another song. For example, I was at the song 1, and then get to the song 3. And once I had "Nothing to resume". I wonder how this code interferes with the playback code.

Yes, that's happening to me too. It seems to be specifically when speed is taken really low, though I haven't isolated a specific test case. Perhaps it's about the processor being able to keep up?

Anyway, I don't think my code is the cause, though I can't be sure. If you look at the debug output you'll see that when that happens it says "sdl_audio_callback: No Data." It's the same thing that was happening before and then Pondlife submitted a change that mostly fixed it. Perhaps he could comment on what might be causing it?

Anyway, it seems that when that error occurs the song either skips ahead or skips to the next track entirely, sometimes skipping past multiple songs.

So yeah, it would be nice to fix that if possible, though I suspect that the problem is buried in the tdspeed/dsp code and is outside my field of Rockbox knowledge.

Here's a first stab at the update for the manual. I can't seem to get latex to work so I wasn't able to test it out, though.

   pitch_screen_manual.diff (2.9 KiB)

Minor improvements (IMO) in the manual. The manual does get built, at least for H120.

   pitch_screen_manual.2.diff (4.6 KiB)