flac: earlier 16bit check and prevent error hang

[arbruijn]

Previously when trying to play a 24bit flac in a loop (eg. with playmus -l) the application hung on exit because FLAC_getsome kept trying to decode data while holding the audio lock.

Add earlier 16bit check to prevent starting to play the file at all. And stop on error status codes from FLAC__stream_decoder_get_state to prevent a hang when flac decode fails.

[sezero]

Does the issue happen with a specific flac file? If so, please attach it here or give a link for testing?

Does the issue happen with SDL2 branch too, or only in the SDL-1.2 branch?

[arbruijn]

Example file: musopen-bwv988v4.zip (from https://archive.org/details/MusopenCollectionAsFlac)

playmus -l musopen-bwv988v4.flac hangs with 100% cpu usage

The SDL2 branch supports 24bit flac, it plays fine.

[sezero]

Hm. With my fresh build from SDL-1.2 branch, that command simply exits (i686-linux), so I can't reproduce (i686-linux).

In any case: If this patch is to be applied, the duplicated error path in flac_write_music_cb() should be removed?

[arbruijn]

Hm, it hangs for me on Ubuntu 20.04 i686/x86_64 (pulseaudio) and on Windows x64 (on Windows it hangs in the background).

It looks like the error path in flac_write_music_cb() comes the flac example, which was probably done that way because flac files can be decoded from an abitrary point in the file, in which case there won't be a metadata block.

But SDL_mixer always reads the metadata at the start of the file, so I think that indeed the error path in flac_write_music_cb() can be removed. Shall I update the PR to remove those checks?

[sezero]

But SDL_mixer always reads the metadata at the start of the file, so I think that indeed the error path in flac_write_music_cb() can be removed. Shall I update the PR to remove those checks?

Yes please.

I also think whether we should backport the 20 / 24 bit file support from SDL2 branch to SDL-1.2

CC: @icculus, @slouken

[icculus]

I think it's a waste of time to backport it to SDL 1.2 (as it will never see an official release), but if someone wants to do it, there's no reason it shouldn't go in revision control.

[arbruijn]

PR is updated to remove the checks in flac_write_music_cb().

I have tested the PR with the flac files from https://samples.ffmpeg.org/flac/ and it seems to work correctly.

file	format	current	with PR
24-bit_192kHz.flac	192kHz stereo 24bit	hang	error
24-bit_96kHz_RICE2_pop.flac	96kHz stereo 24bit	hang	error
Chelsea Wolfe - Pain Is Beauty - 01 - Feral Love.flac	44.1kHz stereo 16bit	ok	ok
children.hall.24bit.5.flac	44.1kHz stereo 24bit	hang	error
dilvie_-_the_dragonfly.flac	44.1kHz stereo 16bit	ok	ok
larger_than_64k_flac_metadata.flac	44.1kHz stereo 16bit	ok	ok
short.flac	11.025kHz mono 16bit	no sound	error
starts_with_id3v2.flac	44.1kHz stereo 16bit	ok	ok
When I Grow Up.flac	44.1kHz stereo 16bit	ok	ok
Yesterday.flac	44.1kHz stereo 16bit	ok	ok

[icculus]

Instead of checking a large list of errors, can we just check if this is != a value that means success?

Just so this doesn't fail if a layer libFLAC adds new error codes.

[sezero]

Instead of checking a large list of errors, can we just check if this is != a value that means success?

Just so this doesn't fail if a layer libFLAC adds new error codes.

That part of the patch to the getsome() procedure is also of interest for the SDL2 branch too, not for 24 bit files but for e.g. malformed ones.

[arbruijn]

Instead of checking a large list of errors, can we just check if this is != a value that means success?

Just so this doesn't fail if a layer libFLAC adds new error codes.

The FLACStreamDecoderState enum has both various success and various error codes unfortunately: https://xiph.org/flac/api/groupflacstreamdecoder.html#ga3adb6891c5871a87cd5bbae6c770ba2d

But the chance of new error codes is probably higher than of new success codes. Shall I change it to check for success codes?

SDL2 handles this by checking if the decode function keeps returning 0 samples (zero_cycles in music_pcm_getaudio), but that looks like a more invasive change.

[icculus]

Eh, let's just leave it as it is, then. I'll merge this now, thanks!