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

lib/util/flac.cpp: Add support for flac data where bits_per_sample != 16. #11848

Merged
merged 6 commits into from
Dec 31, 2023

Conversation

wilbertpol
Copy link
Contributor

No description provided.

@ICEknigh7
Copy link
Contributor

ICEknigh7 commented Dec 17, 2023

Seems to work fine with the 24 bit FLACs I've been trying on the Spectrum core (even better than WAV support, actually).
image image
The only "problem" I've found is that it's only possible to play the Left channel (akin to loading from a Stereo source, rather than the recommended Monaural which would do the audio sum of both channels), but I suppose that's likely caused by something in the drivers themselves and not because of the FLAC decoding.

@wilbertpol
Copy link
Contributor Author

The only "problem" I've found is that it's only possible to play the Left channel (akin to loading from a Stereo source, rather than the recommended Monaural which would do the audio sum of both channels), but I suppose that's likely caused by something in the drivers themselves and not because of the FLAC decoding.

Partially; the driver needs to do a set_stereo() on the cassette device (currently there seem to be no drivers using this). And it also needs this small change:

--- a/src/devices/imagedev/cassette.cpp
+++ b/src/devices/imagedev/cassette.cpp
@@ -452,7 +452,7 @@ void cassette_image_device::sound_stream_update(sound_stream &stream, std::vecto
 
                for (int ch = 0; ch < outputs.size(); ch++)
                {
-                       cassette->get_samples(0, time_index, duration, outputs[0].samples(), 2, &m_samples[0], cassette_image::WAVEFORM_16BIT);
+                       cassette->get_samples(ch, time_index, duration, outputs[0].samples(), 2, &m_samples[0], cassette_image::WAVEFORM_16BIT);
                        for (int sampindex = 0; sampindex < outputs[ch].samples(); sampindex++)
                                outputs[ch].put_int(sampindex, m_samples[sampindex], 32768);
                }

That fixed channels for flacs but gave me some weird echoing results on wavs before; it could be that I tested it wrongly though.

@wilbertpol
Copy link
Contributor Author

It also works with 8 bit flacs. For example, abcstack from the abc80_cass software list converted to flac. This never worked for me before.

Comment on lines 637 to 640
if (m_uncompressed_swap)
{
sample = FLAC__int32(((uint32_t(sample) >> 24) & 0x000000ff) | ((uint32_t(sample) >> 16) & 0x0000ff00) | ((uint32_t(sample) >> 8) & 0x00ff0000) | (uint32_t(sample) & 0xff000000));
}
Copy link
Member

Choose a reason for hiding this comment

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

You could use swapendian_int32 from osdcomn.h

Comment on lines 635 to 636
int16_t flac_decoder::convert_to_int16(const ::FLAC__Frame *frame, FLAC__int32 sample)
{
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason this isn’t const qualified? It shouldn’t change decoder state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, made it const.

Comment on lines 641 to 644
if (frame->header.bits_per_sample >= 16)
return int16_t(uint32_t(sample >> (frame->header.bits_per_sample - 16)));
else
return int16_t(uint32_t(sample << (16 - frame->header.bits_per_sample)));
Copy link
Member

Choose a reason for hiding this comment

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

You should round towards even when reducing resolution, and you should splat right when increasing resolution.

Comment on lines 637 to 644
if (m_uncompressed_swap)
{
sample = FLAC__int32(((uint32_t(sample) >> 24) & 0x000000ff) | ((uint32_t(sample) >> 16) & 0x0000ff00) | ((uint32_t(sample) >> 8) & 0x00ff0000) | (uint32_t(sample) & 0xff000000));
}
if (frame->header.bits_per_sample >= 16)
return int16_t(uint32_t(sample >> (frame->header.bits_per_sample - 16)));
else
return int16_t(uint32_t(sample << (16 - frame->header.bits_per_sample)));
Copy link
Member

Choose a reason for hiding this comment

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

This won’t work if the output format isn’t native Endianness. You’re converting to output format before trying to adjust resolution. Do this on paper and see what happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I had the operations swapped.

Copy link
Member

@cuavas cuavas left a comment

Choose a reason for hiding this comment

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

The trouble with this is it’s a massive pessimisation. Remember this is going to be run for tens of thousands of samples at the very least. The current code is written with pipeline performance in mind – shifts are pre-calculated before the loop so the actual format conversion in the loop doesn’t require any conditional code.

With this change, tests based on input and output formats are being done for every sample.

Please think about how you can restructure the code to lift as much conditional code out of the loop as possible, and to make the code in the loop work with conditional moves rather than requiring more expensive branches.

Also, that doesn’t round towards even – it rounds towards positive infinity.

@wilbertpol
Copy link
Contributor Author

wilbertpol commented Dec 22, 2023

I have moved most of the old conditional code outside the loop by using templating.

@rb6502 rb6502 merged commit 9c95144 into mamedev:master Dec 31, 2023
5 checks passed
@wilbertpol wilbertpol deleted the flac_bps branch February 13, 2024 20:58
einstein95 pushed a commit to einstein95/mame that referenced this pull request Mar 2, 2024
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.

None yet

4 participants