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.

Sign up for GitHub

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

Prevent clicks when looping in AudioStreamWAV #83341

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+68 −20
Open

Prevent clicks when looping in AudioStreamWAV #83341

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 65 additions & 19 deletions scene/resources/audio_stream_wav.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,16 @@ void AudioStreamPlaybackWAV::seek(double p_time) {
}

template <class Depth, bool is_stereo, bool is_ima_adpcm>
void AudioStreamPlaybackWAV::do_resample(const Depth *p_src, AudioFrame *p_dst, int64_t &p_offset, int32_t &p_increment, uint32_t p_amount, IMA_ADPCM_State *p_ima_adpcm) {
void AudioStreamPlaybackWAV::do_resample(const Depth *p_src, AudioFrame *p_dst, int64_t &p_offset,
int32_t &p_increment, uint32_t p_amount, IMA_ADPCM_State *p_ima_adpcm, int64_t p_limit,
int64_t p_border) const {
// this function will be compiled branchless by any decent compiler

if (is_stereo) {
p_limit <<= 1;
p_border <<= 1;
}

int32_t final, final_r, next, next_r;
while (p_amount) {
p_amount--;
Expand Down Expand Up @@ -188,10 +195,19 @@ void AudioStreamPlaybackWAV::do_resample(const Depth *p_src, AudioFrame *p_dst,
}

if (is_stereo) {
next = p_src[pos + 2];
next_r = p_src[pos + 3];
if (pos + 2 < p_limit) {
next = p_src[pos + 2];
next_r = p_src[pos + 3];
} else {
next = p_src[p_border];
next_r = p_src[p_border + 1];
}
} else {
next = p_src[pos + 1];
if (pos + 1 < p_limit) {
next = p_src[pos + 1];
} else {
next = p_src[p_border];
}
}

if constexpr (sizeof(Depth) == 1) {
Expand Down Expand Up @@ -354,7 +370,12 @@ int AudioStreamPlaybackWAV::mix(AudioFrame *p_buffer, float p_rate_scale, int p_
limit = (increment < 0) ? begin_limit : end_limit;

/* compute what is shorter, the todo or the limit? */
aux = (limit - offset) / increment + 1;
if (increment > 0) {
/* stop short of the limit */
aux = (limit - offset + increment - 1) / increment;
} else {
aux = (limit - offset) / increment + 1;
}
target = (aux < todo) ? aux : todo; /* mix target is the shorter buffer */

/* check just in case */
Expand All @@ -365,30 +386,55 @@ int AudioStreamPlaybackWAV::mix(AudioFrame *p_buffer, float p_rate_scale, int p_

todo -= target;

// To avoid clicks when interpolating across the loop end, we need to think about which
// sample should act as the successor of the last sample with regard to interpolation.
const int64_t sample_limit = end_limit >> MIX_FRAC_BITS;
int64_t sample_border;

switch (loop_format) {
case AudioStreamWAV::LOOP_FORWARD:
case AudioStreamWAV::LOOP_BACKWARD:
// Interpolate the last sample with the loop start (wrap).
sample_border = begin_limit >> MIX_FRAC_BITS;
break;
case AudioStreamWAV::LOOP_PINGPONG:
// Interpolate the last sample with the second-to-last sample (mirror).
sample_border = MAX(0, sample_limit - 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks right for handling the playback bouncing off the last sample of the loop (and thank you for thinking about extremely short sound files) but will this do the right thing when the ping pong loop bounces off the start position? I'm also a little bit unclear as to why the forward and backwards cases are handled identically.

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, it took me some time, to get my head around which possible cases we're dealing with here. In the end, we only ever need to worry about the right edge because do_resample will always interpolate between the "previous" sample and the "next" regardless of the direction we're moving in. offset is mapped to samples by discarding the lower bits, so we can be up to almost a whole sample after the (start of) the last sample, but we cannot be before the first sample. I found it somewhat less obvious that we can end up after the (start of) the last sample when walking backwards and wrapping around to the loop end (because the sub-sample part is retained), but that then maps to the same case of having to deal with the loop end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if I'm doing a very good job at explaining what I mean 😅, so here's an example: If we have a loop that starts at 0 and has a length of 10, then the loop end is 10 and the last sample that is part of the loop is 9. offset is a fixed-point number but if it were a float, its valid values would be between 0.0 and 9.99... I believe the original code correctly ensures that offset will never leave the [0, 10) range. The problem was that for positions > 9, the interpolation was done between samples 9 and 10, although 10 isn't part of the loop region. On the lower end, however, even the extreme value 0.0 would be correctly interpolated between samples 0 and 1, so there's no issue there. When starting at 3.5 and playing 10 samples backwards, the code would play 4 samples starting from 3.5 (positions 3.5, 2.5, 1.5, 0.5) and then 6 samples starting at 9.5, so the critical case is still in the 9-10 range.

break;
default:
// Interpolate the last sample with itself (clamp).
sample_border = MAX(0, sample_limit - 1);
break;
}

switch (base->format) {
case AudioStreamWAV::FORMAT_8_BITS: {
case AudioStreamWAV::FORMAT_8_BITS:
if (is_stereo) {
do_resample<int8_t, true, false>((int8_t *)data, dst_buff, offset, increment, target, ima_adpcm);
do_resample<int8_t, true, false>((int8_t *)data, dst_buff, offset, increment, target,
ima_adpcm, sample_limit, sample_border);
} else {
do_resample<int8_t, false, false>((int8_t *)data, dst_buff, offset, increment, target, ima_adpcm);
do_resample<int8_t, false, false>((int8_t *)data, dst_buff, offset, increment, target,
ima_adpcm, sample_limit, sample_border);
}
} break;
case AudioStreamWAV::FORMAT_16_BITS: {
break;
case AudioStreamWAV::FORMAT_16_BITS:
if (is_stereo) {
do_resample<int16_t, true, false>((int16_t *)data, dst_buff, offset, increment, target, ima_adpcm);
do_resample<int16_t, true, false>((int16_t *)data, dst_buff, offset, increment, target,
ima_adpcm, sample_limit, sample_border);
} else {
do_resample<int16_t, false, false>((int16_t *)data, dst_buff, offset, increment, target, ima_adpcm);
do_resample<int16_t, false, false>((int16_t *)data, dst_buff, offset, increment, target,
ima_adpcm, sample_limit, sample_border);
}

} break;
case AudioStreamWAV::FORMAT_IMA_ADPCM: {
break;
case AudioStreamWAV::FORMAT_IMA_ADPCM:
if (is_stereo) {
do_resample<int8_t, true, true>((int8_t *)data, dst_buff, offset, increment, target, ima_adpcm);
do_resample<int8_t, true, true>((int8_t *)data, dst_buff, offset, increment, target,
ima_adpcm, sample_limit, sample_border);
} else {
do_resample<int8_t, false, true>((int8_t *)data, dst_buff, offset, increment, target, ima_adpcm);
do_resample<int8_t, false, true>((int8_t *)data, dst_buff, offset, increment, target,
ima_adpcm, sample_limit, sample_border);
}

} break;
break;
}

dst_buff += target;
Expand Down
4 changes: 3 additions & 1 deletion scene/resources/audio_stream_wav.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ class AudioStreamPlaybackWAV : public AudioStreamPlayback {
Ref<AudioStreamWAV> base;

template <class Depth, bool is_stereo, bool is_ima_adpcm>
void do_resample(const Depth *p_src, AudioFrame *p_dst, int64_t &p_offset, int32_t &p_increment, uint32_t p_amount, IMA_ADPCM_State *p_ima_adpcm);
void do_resample(const Depth *p_src, AudioFrame *p_dst, int64_t &p_offset,
int32_t &p_increment, uint32_t p_amount, IMA_ADPCM_State *p_ima_adpcm, int64_t p_limit,
int64_t p_border) const;

public:
virtual void start(double p_from_pos = 0.0) override;
Expand Down
Loading