Hey Chris, hey Levente,

doing a proper upgrade to support 24bit playback appears as quite the task. The assumption of 16bit signed buffers is hardcoded down to the vm platform plugins, see e.g.:
https://github.com/OpenSmalltalk/opensmalltalk-vm/blob/Cog/platforms/unix/vm-sound-pulse/sqUnixSoundPulseAudio.c#L761
https://github.com/OpenSmalltalk/opensmalltalk-vm/blob/Cog/platforms/win32/plugins/SoundPlugin/sqWin32Sound.c#L311

Regarding the usability concerns: the problem came up when some students attempted to play a 24bit sound file that they had downloaded somewhere. They ended up taking away that Squeak was not able to play sounds on their system. In further attempts, they reexported the file themselves, which by chance saved it as 16bit file, without them noticing. Now it "suddenly" worked.

I personally am fine with automatic truncation of the precision but I could understand some people preferring to keep that precision. The four options I see are
1. a resumable warning (annoying when files are loaded automatically),
2. a parameter allowing automatic truncation, e.g. SamplesSound>>#fromWaveFileNamed:lossy: or something more descriptive,
3. a system-wide preference (not making it better in terms of having fewer preferences, but at least (coupled with a resumable warning) the system could then tell me *once* that I am losing precision and I can inform it that I don't care, I just want my sounds to play), or
4. silently truncating (likely making your average sound user happy, but giving a false impression to any advanced user).

@Levente: great catch about the bitshift. That did not occur to me at the time.
It appears that we're currently only handling little endian in Squeak, as per this site: http://soundfile.sapp.org/doc/WaveFormat/ which states that the header will contain a different value should it be in big endian. I don't find any mention of the "RIFX" (big endian) magic word in my image, only "RIFF" (little endian).
I intentionally avoided caseOf:, because I heard its usage was preferred only in places where performance matters. I don't mind it though, so if a majority prefers using it here, we could also go for that.

All the best,
Tom
________________________________________
From: Squeak-dev <squeak-dev-bounces at lists.squeakfoundation.org> on behalf of Levente Uzonyi <leves at caesar.elte.hu>
Sent: Friday, December 27, 2019 1:04:00 AM
To: squeak-dev at lists.squeakfoundation.org
Subject: Re: [squeak-dev] The Inbox: Sound-tobe.67.mcz

On Wed, 25 Dec 2019, commits at source.squeak.org wrote:

> A new version of Sound was added to project The Inbox:
> http://source.squeak.org/inbox/Sound-tobe.67.mcz
>
> ==================== Summary ====================
>
> Name: Sound-tobe.67
> Author: tobe
> Time: 25 December 2019, 9:52:59.799378 pm
> UUID: 3cf4e2e2-48db-4cb4-bbe4-46a2a464e38f
> Ancestors: Sound-eem.66
>
> Support loading 24bit Wave files by converting to 16bit
>
> While Squeak expects Sound files to be in 16bit format in its SoudBuffer, with this change we now fall back to losing precision of 24bit files instead of pretending to load them and then playing back noise.
>
> =============== Diff against Sound-eem.66 ===============
>
> Item was added:
> + ----- Method: SampledSound class>>convertBytesTo24BitSamples: (in category 'utilities') -----
> + convertBytesTo24BitSamples: aByteArray
> +
> +     | data n src |
> +     n := aByteArray size // 3.
> +     data := SoundBuffer newMonoSampleCount: n.
> +     src := 1.
> +     1 to: n do: [:i | | b1 b2 b3 w |
> +             b1 := aByteArray at: src.
> +             b2 := aByteArray at: src + 1.
> +             b3 := aByteArray at: src + 2.
> +
> +             " SoundBuffer only support 16bit at this time, so we divide the amplitudes here to fit "
> +             w := ((b3 bitShift: 16) + (b2 bitShift: 8) + b1) // (2 ** 8).

The above line can be simplified to:

                w := (b3 bitShift: 8) + b2.

That also makes b1 unnecessary.

Btw, do 24-bit wav files always contain little-endian data?

> +             w > 32767 ifTrue: [w := w - 65536].
> +             data at: i put: w.
> +             src := src + 3].
> +
> +     ^ data!
>
> Item was added:
> + ----- Method: SampledSound class>>createSoundBufferFrom:bitsPerSample: (in category 'instance creation') -----
> + createSoundBufferFrom: aByteArray bitsPerSample: aNumber
> +
> +     aNumber = 8 ifTrue: [^ self convert8bitUnsignedTo16Bit: aByteArray].
> +     aNumber = 16 ifTrue: [^ self convertBytesTo16BitSamples: aByteArray mostSignificantByteFirst: false].
> +     aNumber = 24 ifTrue: [^ self convertBytesTo24BitSamples: aByteArray].
> +     ^ self error: 'Sound files with ', aNumber asString, ' bits per sample are currently not supported'!

#caseOf:otherwise: would fit here perfectly:

        ^aNumber
                caseOf: {
                        [ 8 ] -> [ self convert8bitUnsignedTo16Bit: aByteArray ].
                        [ 16 ] -> ...
                        ... }
                otherwise: [ self error: 'Sound files with ', aNumber asString, ' bits per sample are currently not supported' ]


Levente

>
> Item was changed:
>  ----- Method: SampledSound class>>fromWaveStream: (in category 'instance creation') -----
>  fromWaveStream: fileStream
>
>       | stream header data type channels samplingRate blockAlign bitsPerSample leftAndRight |
>       header := self readWaveChunk: 'fmt ' inRIFF: fileStream.
>       data := self readWaveChunk: 'data' inRIFF: fileStream.
>       fileStream close.
>       stream := ReadStream on: header.
>       type := self next16BitWord: false from: stream.
>       type = 1 ifFalse: [^ self error:'Unexpected wave format'].
>       channels := self next16BitWord: false from: stream.
>       (channels < 1 or: [channels > 2])
>               ifTrue: [^ self error: 'Unexpected number of wave channels'].
>       samplingRate := self next32BitWord: false from: stream.
>       stream skip: 4. "skip average bytes per second"
>       blockAlign := self next16BitWord: false from: stream.
>       bitsPerSample := self next16BitWord: false from: stream.
>       (bitsPerSample = 8 or: [bitsPerSample = 16])
>               ifFalse: [  "recompute bits per sample"
>                       bitsPerSample := (blockAlign // channels) * 8].
> +
> +     data := self createSoundBufferFrom: data bitsPerSample: bitsPerSample.
> +
> -
> -     bitsPerSample = 8
> -             ifTrue: [data := self convert8bitUnsignedTo16Bit: data]
> -             ifFalse: [data := self convertBytesTo16BitSamples: data mostSignificantByteFirst: false].
> -
>       channels = 2 ifTrue: [
>               leftAndRight := data splitStereo.
>               ^ MixedSound new
>                       add: (self samples: leftAndRight first samplingRate: samplingRate) pan: 0.0;
>                       add: (self samples: leftAndRight last samplingRate: samplingRate) pan: 1.0;
>                       yourself].
>
>       ^ self samples: data samplingRate: samplingRate
>  !