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

Sas: Correct delay in playing samples #11097

Merged
merged 2 commits into from
Jun 6, 2018

Conversation

unknownbrackets
Copy link
Collaborator

After #8950 (I think, didn't double check), we stopped passing the various tests for sceSas.

This was mainly because the key on delay was applied twice (delay and STATE_KEYON_STEP.) I think we can remove STATE_KEYON_STEP maybe, but haven't checked the pitch behavior carefully. This makes it walk the curve during the delay, playing audio 32 samples earlier (a bit under a millisecond.)

This also avoids the resampleHist delay for the first render (we already have delay after all), which makes it align neatly to tests. They all pass again.

Lastly, I discovered while testing that we were decoding uninitialized memory during the test. We set 0x100 bytes of generated VAG data (16 blocks), yet it decoded 17 blocks - and one of them made no sense. Now it ends after it reads the last block inside size.

Played some VAG music, voices, and sound effects and they sound good to me - but I don't have the best ear.

-[Unknown]

@unknownbrackets unknownbrackets added this to the v1.7.0 milestone May 29, 2018
@hrydgard
Copy link
Owner

Nice! So much good stuff to merge now after 1.6 is done :)

@weihuoya
Copy link
Contributor

weihuoya commented Jun 2, 2018

tekken 5 has problem with this PR. bgm and sound volume.

@unknownbrackets
Copy link
Collaborator Author

Does it happen with only the first commit? Does it happen if you change delay = ignorePitch ? 32 : (32 * (u32)voice.pitch) >> PSP_SAS_PITCH_BASE_SHIFT; to delay = 32;?

Any impact to volume should mean an issue with ADSR.

-[Unknown]

@weihuoya
Copy link
Contributor

weihuoya commented Jun 2, 2018

if (curBlock_ == numBlocks_) this fx tekken 5 issue.

@unknownbrackets
Copy link
Collaborator Author

Hm. Might need to log what it's sending. Maybe it rounds up or something, when the size is uneven?

-[Unknown]

@unknownbrackets
Copy link
Collaborator Author

It'd be useful to know what vagSize is in VagDecoder::Start. In theory, it should always be a multiple of 16.

Does it help to change numBlocks_ = vagSize / 16; to numBlocks_ = (vagSize + 15) / 16;?

I can revert that last commit to get the rest merged, but just wondering what's happening that the volume would go down...

-[Unknown]

@weihuoya
Copy link
Contributor

weihuoya commented Jun 4, 2018

numBlocks_ = (vagSize + 15) / 16; or delay = 32; not help.

We still need to walk during the delay to "use it up."  Need to test more
to see if we can just walk once directly into ATTACK - might depend on
pitch.

This also makes the first play ignore the resampleHist, which matches
samples to tests properly, and ignores linear interp for exact pitch.

These changes fix all the sascore tests that used to work.
Not sure why everything is negative one indexed, but this prevents reading
beyond the size of the buffer.

This shouldn't change sound output, but it may fix a crash if VAG is at
the edge of memory (unlikely, though.)
@unknownbrackets
Copy link
Collaborator Author

Got it - I see where I messed up now.

Once end_ is set, it ignores the samples from that block and all others. So it was outputting correct audio before, just wasting time (and reading outside a buffer) at the end.

Does it work now?

-[Unknown]

@weihuoya
Copy link
Contributor

weihuoya commented Jun 6, 2018

it works.

@hrydgard
Copy link
Owner

hrydgard commented Jun 6, 2018

Looks good to me.

@hrydgard hrydgard merged commit 46683c5 into hrydgard:master Jun 6, 2018
@unknownbrackets unknownbrackets deleted the sas-minor branch June 6, 2018 16:55
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