Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve pitch handling in sas #5509

Merged
merged 9 commits into from
Feb 18, 2014
Merged

Conversation

unknownbrackets
Copy link
Collaborator

Based on testing:

  • Fixes the 32-sample lead-in on keyon with various pitch values.
  • Completes the vag sample before ending the envelope. This is likely to (slightly) affect a lot of games, but is correct per my tests - it should process the final samples at the normal height before ending the envelope.
  • Resets sampleFrac at key on - otherwise, a new keyon could persist the old pitch fraction.
  • Does basic interpolation of the samples. I'm not sure if this is exactly right (especially at irregular pitches), but it seems better.

-[Unknown]

Before it would be more or less depending on pitch.  It's always 32.
Wait instead until the end of mixing to end the envelope.
Since we're starting the samples over.
Doesn't seem to be a big help but may matter more on mobile.
It seems to be supported only below 0x1000.  Also, drop optimization for
double for now, simpler this way and doesn't seem common?
@unknownbrackets
Copy link
Collaborator Author

Hmm, I think I found an issue in Final Fantasy Tactics, there seems to be a bit of fuzz, so I probably have a bug here...

But I think it also makes the teletype sound more correct.

-[Unknown]

volumeShift += MAX_CONFIG_VOLUME - g_Config.iSFXVolume;

for (int i = 0; i < grainSize; i++) {
const int readIndex = sampleFrac / PSP_SAS_PITCH_BASE;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have an aversion to using / and % when we know it's by a power of two and could just as well have used shifts and masks. I know that the compiler will strength-reduce them away when we build with optimizations, but when we don't, it probably won't.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay then, I'll change it.

-[Unknown]

@unknownbrackets
Copy link
Collaborator Author

Something must be wrong with my sampling after all, it causes faint but noticeable static in the background in Final Fantasy Tactics, grr...

-[Unknown]

@hrydgard
Copy link
Owner

I wonder if you reach the end of the resampleBuffer and end up interpolating with a zero or other garbage after the last sample, so need to always be a few samples ahead.

@unknownbrackets
Copy link
Collaborator Author

Hmm, the "smoothness hackery" should take care of that maybe?

        // Smoothness HACKERY
        resampleBuffer[2 + numSamples] = resampleBuffer[2 + numSamples - 1];

Although, technically I skip a sample I guess every time it ends on a non-even sampleFrac... hmm...

-[Unknown]

Fixes static in Final Fantasy Tactics music.
@unknownbrackets
Copy link
Collaborator Author

Using the resample buffer made it go away, so I guess that was it.

-[Unknown]

@hrydgard
Copy link
Owner

That offset thing feels a little iffy, but close enough, I guess.

I will revisit this later and add four-point cubic interpolation as well. Not sure which interpolation the PSP uses though but it's likely to be better than two-point linear.

hrydgard added a commit that referenced this pull request Feb 18, 2014
@hrydgard hrydgard merged commit 106656f into hrydgard:master Feb 18, 2014
@unknownbrackets
Copy link
Collaborator Author

The offset should be correct, since if sampleFrac is 0 it means the whole sample was used and it should use the next one, but if it's != 0, that means it needs to use a portion of the previous sample (and so will be one behind for the entire duration... not sure if we overread samples in this case from the source.)

IIRC, this matched the output of 0x0800 pitch exactly (although with a slight offset, I think it's starting a sample too early or something..), but it doesn't match e.g. 0x1400 properly. It could very well use more points in its interpolation.

And it does definitely match 0x1000 correctly, so that must not have any blurring. I can't remember now if I fixed 0x2000, which was off-by-one (I think the PSP was giving odd samples and PPSSPP even, or the other way around.)

Of course, resampling better than the PSP probably would not hurt anyone.

-[Unknown]

@unknownbrackets unknownbrackets deleted the sas-minor branch February 18, 2014 08:26
@solarmystic
Copy link
Contributor

@unknownbrackets
Just did some belated testing in games which were senstive to similar commits in the past, it seems like nothing has changed for the worst in terms of the audio, so that's pretty good.

I remember a previous ADSR related change made Valkyria Chronicles 3 have an infinite vehicular weapons fire SFX bug, (which you fixed) so I was curious to see whether it'd happen again.

Some games that use VAG extensively like the Tekken games were also tested, and were found to be clear of any audio issues too after the commit.

In fact (it might just be a placebo effect), many games sound "better" so to speak.

@unknownbrackets
Copy link
Collaborator Author

It's resampling better (similar to having Texture Filtering set to Linear instead of Nearest) at certain pitches, and not dropping off the tail of some samples that end instead of looping. It should sound better, if only slightly.

-[Unknown]

@unknownbrackets
Copy link
Collaborator Author

Actually, I think I was wrong about the output matching 0x0800 exactly, seems like it doesn't in fact. I did most of my pitch testing with linear pcm values to make it easy, which is what I got to match (and of course that would likely match exactly even if it was using four-point...)

-[Unknown]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants