Implement the builtin sound/null.wav file
#1854
Conversation
illwieckz
changed the title
audio/sound.wav filesound/null.wav file
illwieckz
changed the title
sound/null.wav filesound/null.wav file
illwieckz
changed the title
sound/null.wav filesound/null.wav file
illwieckz
changed the title
sound/null.wav filesound/null.wav file
illwieckz
force-pushed
the
illwieckz/builtin-null-wav
branch
from
1b73d61 to
8dce432
Compare
|
One drawback of such implementation is that: |
|
The good thing in that implementation is that the null samples now only takes 2 bytes in memory when loaded! Once compiled the code implementing it is probably smaller than the 46 bytes of the “smallest silent WAV file ever” that could be embedded instead. |
illwieckz
force-pushed
the
illwieckz/builtin-null-wav
branch
from
8dce432 to
3cee189
Compare
src/engine/audio/SoundCodec.cpp
Outdated
|
|
||
| static int numSoundLoaders = ARRAY_LEN(soundLoaders); | ||
|
|
||
| static bool IsNullSample(std::string& filename) |
There was a problem hiding this comment.
How about simply filename == "sound/null" || filename == "sound/null.wav"? This would be more correct in the case of something like sound/null.notarealextension
There was a problem hiding this comment.
Hmm, right, someone may look for null.not expecting to load null.not.opus.
So the best would be to look for the basename with known extensions instead.
There was a problem hiding this comment.
For the moment the code doesn't care for unknown extensions:
]/playMusic sound/ui/heartbeat.plop
Debug: Loading Sample 'sound/ui/heartbeat.plop'
]/playMusic sound/ui/heartbeat1.haha
Debug: Loading Sample 'sound/ui/heartbeat1.haha'
So there may be redesigns to do that are not part of this PR.
There was a problem hiding this comment.
I taken care of not misdetecting non-existent extensions, but actually the existing code is very perfectible and should be revamped at some point (see below).
Also it's funny that the sound/null sample is now the only sound file that fails to load when attempting to load it with an unknown extension.
There was a problem hiding this comment.
What's wrong with just filename == "sound/null" || filename == "sound/null.wav"? The loader iteration thing is unnecessarily complex. There is no reason for anyone to ever write sound/null.opus.
There was a problem hiding this comment.
Well, I prefer to be exhaustive… I did in Sample.cpp without ext check.
src/engine/audio/SoundCodec.cpp
Outdated
|
|
||
| AudioData LoadSoundCodec(std::string filename) | ||
| { | ||
| if (IsNullSample(filename)) { |
There was a problem hiding this comment.
Nit: I think the more conceptually correct place for this would be in Sample::Load, since in this case we aren't using a codec.
There was a problem hiding this comment.
The soundExtToLoaderMap_t isn't known from Sample.cpp so testing for the extension would not keep the code self-contained in a single file.
src/engine/audio/SoundCodec.cpp
Outdated
| // 16bit mono 8KHz, 1 sample, silence. | ||
| unsigned char *null_wav = (unsigned char*) calloc( 1, 2 ); | ||
| AudioData out { 8000, 2, 1 }; | ||
| out.rawSamples.assign( null_wav, null_wav + 2 ); |
There was a problem hiding this comment.
out.rawSamples.resize(2) (fills with zeroes) ? The current version does a memory leak. Also why not 8-bit? :-)
There was a problem hiding this comment.
Also why not 8-bit? :-)
If the engine allows it, why not. I'll try.
There was a problem hiding this comment.
The engine itself doesn't complain, but when I do:
]/playMusic sound/null
I hear a click noise, likely from the audio subsystem itself when switching the resampling mode or so. That should not happen, but we better do not randomly change the size and keep the things done as usual.
There was a problem hiding this comment.
The null_wav memory leak still needs to be fixed. One alternative would be out.rawSamples.resize(2) to fill with two zeroes.
There was a problem hiding this comment.
Oh lol that's the worst occurrence of ghost code resurfacing on rebase I ever seen!
Reimplemented the .resize(2) way.
|
Also don't forget to delete it from |
illwieckz
force-pushed
the
illwieckz/builtin-null-wav
branch
from
3cee189 to
6c83ef0
Compare
|
I reworked the detection of the null sound file to avoid misdetecting A lot of things is messy in the existing code: the code reports as empty a missing file, it reports the loading of sound files with any extension that doesn't exist and you can reload it as many time as you request a different extension, it does string copies all the way down, etc. But revamping that would require its own PR and that would be a large PR. That's why I stick to the least intrusive effort of hijacking I now deleted the file from the dpkdir. |
illwieckz
force-pushed
the
illwieckz/builtin-null-wav
branch
from
6c83ef0 to
01d939d
Compare
|
@slipher any new comment? |
illwieckz
force-pushed
the
illwieckz/builtin-null-wav
branch
from
01d939d to
9ebb630
Compare
|
LGTM |
Implement the builtin
sound/null.wavfile.