core: fix garbage tail in stereo recordings on media bug close - #3105
Open
PegasusS wants to merge 1 commit into
Open
core: fix garbage tail in stereo recordings on media bug close#3105PegasusS wants to merge 1 commit into
PegasusS wants to merge 1 commit into
Conversation
The SWITCH_ABC_TYPE_CLOSE drain loop in record_callback() computes the write length without dividing by the channel count, so on stereo recordings it asks switch_core_file_write() for twice as many sample frames as switch_core_media_bug_read() actually produced. The surplus is read from the uninitialised tail of the on-stack buffer and encoded into the file, producing a short full-scale broadband burst at the end of every recording. The READ_PING branch of the same function already divides by frame.channels; this makes the CLOSE branch consistent with it. switch_core_media_bug_read() doubles frame->datalen when SMBF_STEREO is set, while the len argument of switch_core_file_write() is expressed in per-channel sample frames. Dividing only by 2 therefore yields twice the correct value. With the default 8 kHz stereo recording profile the surplus is 640 bytes = 160 stereo samples = 20.000 ms per close. Measured burst widths on affected files had a median of 19.9 ms. Signed-off-by: shihaifeng <shihf_dc@163.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
core: fix garbage tail in stereo recordings on media bug close
Problem
Every recording produced with a stereo record profile ends with a short
full-scale broadband burst — audible as a click. It is present in the PCM
before any encoding, so it survives every codec and every post-processing
step, and it appears once per
switch_ivr_record_session()close (includingeach segment when recordings are split).
Root cause
record_callback()insrc/switch_ivr_async.c,SWITCH_ABC_TYPE_CLOSEbranch:Two facts make this wrong for stereo:
switch_core_media_bug_read()doublesframe->datalenwhenSMBF_STEREOis set (it interleaves both legs into the caller's buffer).
lenargument ofswitch_core_file_write()is expressed inper-channel sample frames, not in total samples.
So
datalen / 2yields twice the correct frame count.switch_core_file_write()then consumes
len * channels * 2bytes fromdata[], i.e. twice whatswitch_core_media_bug_read()actually filled. The surplus comes from theuninitialised tail of the on-stack
data[SWITCH_RECOMMENDED_BUFFER_SIZE]buffer and is encoded into the file.
The
SWITCH_ABC_TYPE_READ_PINGbranch of the very same function already hasthe correct form:
The two branches simply disagree; this patch makes CLOSE match READ_PING.
Quantitative check
For the common 8 kHz stereo case the surplus is exactly
Measured burst widths across 27 affected recordings had a median of 19.9 ms,
with 14 of 27 falling inside a 1.3 ms band (18.6–19.9 ms) — consistent with a
fixed-length artefact rather than signal-dependent noise.
Affected versions
The CLOSE branch has been missing the divisor in every release checked, while
READ_PING has always had it:
… / 2;… / 2 / frame.channels;… / 2;… / 2 / frame.channels;… / 2;… / 2 / frame.channels;… / 2;… / 2 / frame.channels;… / 2;… / 2 / frame.channels;… / 2;… / 2 / frame.channels;This is a long-standing defect, not a regression.
Reproduction
RECORD_STEREO=true).uuid_record … stop, or hang up).dual-channel burst is present, including on channels that were digitally
silent for the entire call.
The last point is the cleanest discriminator: the burst appears on a channel
that carried no audio at all, so it cannot be signal.
Verification performed
The one-line change was built and deployed on two systems doing continuous
stereo recording:
libfreeswitch.sobyte-for-byte (whole file, not just.text),establishing that the build environment matched the running binary.
67 regions, with a displacement distribution of
{-0x10: 66, +0x10: 1}:every difference is pure code displacement, no semantic change outside
record_callback().record_callback()went from 4 to 5, with the new division located inside the CLOSE drain loop,
ahead of the
switch_core_media_bug_read()call.events went from 27 to 0 on the first system; on the second, 11 calls /
49 recording seams produced 0 hits, with a same-run positive control over
pre-fix material confirming the detector still fires on the defect.
Note on side effects
switch_core_file_write()advancesrh->fh->samples_outbylen. Because thepatch halves
lenfor stereo,samples_outalso halves. Deployments that set anon-zero
RECORD_MIN_SECshould be aware that the CLOSE branch comparessamples_outagainstsamplerate * min_secand may callswitch_file_remove();recordings that previously sat just above that threshold could now fall below it.
The previous value was inflated, so the new comparison is the correct one — but
it is a visible behaviour change for anyone who tuned
RECORD_MIN_SECagainstthe inflated figure.