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

YM2610 ADPCM-A/B sample padding is incorrect #1910

Open
TheHpman opened this issue May 26, 2024 · 5 comments
Open

YM2610 ADPCM-A/B sample padding is incorrect #1910

TheHpman opened this issue May 26, 2024 · 5 comments
Assignees
Labels
bug Something isn't working feedback Further information is requested

Comments

@TheHpman
Copy link

There is an issue with the way samples are padded to fit the 256 bytes boundaries restriction.

Currently, samples are encoded and stored in ROM data, the data is then padded with 0x00 to the next 0x100 boundary. This produces incorrect data at the end of the sample, albeit for a very small time period:
image

Samples should be extended to the correct size prior to encoding to produce correct padding up to the next boundary in ROM data.

@tildearrow tildearrow added the bug Something isn't working label May 27, 2024
@TheHpman
Copy link
Author

TheHpman commented May 27, 2024

I need to add this is from the Neo Seaside NG sample track coming with Furnace, pictured data is ADPCM-B data block from VGM export (after assigning each sample to the correct region, as track puts them in both A & B by default; So basically B region only has that one sample).

Also that sample actually ends @0x1a5ff, so reported issue is still valid but only in range 0x1a5fb - 0x1a5ff unlike pictured.
However this shows us data banks have extra 256bytes of unused blank data added at their end. (0x1a600-0x1a6ff here, found that extra block in A and B regions)

So this is the actual issue, sorry for the confusion:
image

tildearrow added a commit that referenced this issue Dec 11, 2024
issue #1910
may or may not fix the issue. testing needed.
@tildearrow tildearrow self-assigned this Dec 11, 2024
@tildearrow tildearrow added the feedback Further information is requested label Dec 11, 2024
@tildearrow
Copy link
Owner

A fix has been pushed. I will test later but if you want to help, feel free to test.

@TheHpman
Copy link
Author

Better but still not 100% correct: padding should be either 0x80 or 0x08 depending on last encoded sample, and that can't be told before running the encoder.

Correct fix is to insert 0 value samples at the end of source data until reaching a 512 boundary, yma_encode/ymb_encode will therefor produce a 256 bytes boundary sample with the correct padding.

Sorry I'm not familiar with the codebase, not sure where to resize source data for a clean fix.

@tildearrow
Copy link
Owner

tildearrow commented Dec 13, 2024

Sadly, that's difficult. Sorry.

The problem is when the sample in the song is in ADPCM-A or ADPCM-B format (this is the case for Neo Volley) - we don't know how to end the sample.

@TheHpman
Copy link
Author

TheHpman commented Dec 13, 2024

Another easy way you can fix this is edit yma_encode/ymb_encode, so it continues encoding 0 values when len isn't 512 multiple.
Supplied output buffer is already 256 multiple, so this should be a transparent fix:

void yma_encode(int16_t *buffer,uint8_t *outbuffer,long len)
{
	long i;
	long padded_len = (len+511)&(~0x1ff);

	int16_t history = 0;
	uint8_t step_hist = 0;
	uint8_t buf_sample = 0, nibble = 0;
	
	for(i=0;i<padded_len;i++)
	{
		int16_t sample = (i < len ? *buffer++ : 0);
		[...]
	}
}

(or make a separate yma_encode_padded function if that suits you better)

Issue with arbitrary 0x80 fill is it will hold the last sample value instead of falling back to 0, with the extremely rare odd case of over/underflow depending on what the last value was.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feedback Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants