Andrew Larkin
2017-Jul-06 04:28 UTC
[opus] Suggested patch, opus_encoder.c, decide_dtx_mode()
While porting opus 1.2.1 to esp32, my compiler had a grumble (error) about
possible use of uninitialized variables in decide_dtx_mode()
Existing code:
--------SNIP---------
/* Decides if DTX should be turned on (=1) or off (=0) */
static int decide_dtx_mode(float activity_probability, /* probability
that current frame contains speech/music */
int *nb_no_activity_frames, /* number of
consecutive frames with no activity */
opus_val32 peak_signal_energy, /* peak energy of
desired signal detected so far */
const opus_val16 *pcm, /* input pcm
signal */
int frame_size, /* frame size */
int channels,
int is_silence, /* only digital
silence detected in this frame */
int arch
)
{
int is_noise;
opus_val32 noise_energy;
int is_sufficiently_quiet;
if (!is_silence)
{
is_noise = activity_probability < DTX_ACTIVITY_THRESHOLD;
if (is_noise)
{
noise_energy = compute_frame_energy(pcm, frame_size, channels,
arch);
is_sufficiently_quiet = peak_signal_energy >= (PSEUDO_SNR_THRESHOLD
* noise_energy);
}
}
if (is_silence || (is_noise && is_sufficiently_quiet))
{
/* The number of consecutive DTX frames should be within the allowed
bounds */
(*nb_no_activity_frames)++;
if (*nb_no_activity_frames > NB_SPEECH_FRAMES_BEFORE_DTX)
{
if (*nb_no_activity_frames <= (NB_SPEECH_FRAMES_BEFORE_DTX +
MAX_CONSECUTIVE_DTX))
/* Valid frame for DTX! */
return 1;
else
(*nb_no_activity_frames) = NB_SPEECH_FRAMES_BEFORE_DTX;
}
} else
(*nb_no_activity_frames) = 0;
return 0;
}
--------SNIP---------
The problem is in the case of function parameter is_silence != 0, the code:
if (is_silence || (is_noise && is_sufficiently_quiet))
is using uninitialized local variables "is_noise" and
"is_sufficiently_quiet". The compiler complaint is valid.
While this will have no functional effect due to the state of
"is_silence",
the problem can be addressed by restructuring the code. The proposed new
code also eliminates unnecessary temporary variables and reduces operators
as well.
New code:
--------SNIP---------
/* Decides if DTX should be turned on (=1) or off (=0) */
static int decide_dtx_mode(float activity_probability, /* probability
that current frame contains speech/music */
int *nb_no_activity_frames, /* number of
consecutive frames with no activity */
opus_val32 peak_signal_energy, /* peak energy of
desired signal detected so far */
const opus_val16 *pcm, /* input pcm
signal */
int frame_size, /* frame size */
int channels,
int is_silence, /* only digital
silence detected in this frame */
int arch
)
{
opus_val32 noise_energy;
if (!is_silence)
{
if( activity_probability < DTX_ACTIVITY_THRESHOLD ) /* is_noise */
{
noise_energy = compute_frame_energy(pcm, frame_size, channels,
arch);
is_silence = peak_signal_energy >= (PSEUDO_SNR_THRESHOLD *
noise_energy); /* but is_sufficiently_quiet */
}
}
if (is_silence)
{
/* The number of consecutive DTX frames should be within the allowed
bounds */
(*nb_no_activity_frames)++;
if (*nb_no_activity_frames > NB_SPEECH_FRAMES_BEFORE_DTX)
{
if (*nb_no_activity_frames <= (NB_SPEECH_FRAMES_BEFORE_DTX +
MAX_CONSECUTIVE_DTX))
/* Valid frame for DTX! */
return 1;
else
(*nb_no_activity_frames) = NB_SPEECH_FRAMES_BEFORE_DTX;
}
} else
(*nb_no_activity_frames) = 0;
return 0;
}
--------SNIP---------
Felicia Lim
2017-Jul-06 18:07 UTC
[opus] Suggested patch, opus_encoder.c, decide_dtx_mode()
Thanks Andrew, this has now been merged. On Wed, Jul 5, 2017 at 9:36 PM Andrew Larkin <andrewl at arcadius.com.au> wrote:> While porting opus 1.2.1 to esp32, my compiler had a grumble (error) about > possible use of uninitialized variables in decide_dtx_mode() > > Existing code: > > --------SNIP--------- > /* Decides if DTX should be turned on (=1) or off (=0) */ > static int decide_dtx_mode(float activity_probability, /* probability > that current frame contains speech/music */ > int *nb_no_activity_frames, /* number of > consecutive frames with no activity */ > opus_val32 peak_signal_energy, /* peak energy of > desired signal detected so far */ > const opus_val16 *pcm, /* input pcm > signal */ > int frame_size, /* frame size */ > int channels, > int is_silence, /* only digital > silence detected in this frame */ > int arch > ) > { > int is_noise; > opus_val32 noise_energy; > int is_sufficiently_quiet; > > if (!is_silence) > { > is_noise = activity_probability < DTX_ACTIVITY_THRESHOLD; > if (is_noise) > { > noise_energy = compute_frame_energy(pcm, frame_size, channels, > arch); > is_sufficiently_quiet = peak_signal_energy >> (PSEUDO_SNR_THRESHOLD > * noise_energy); > } > } > > if (is_silence || (is_noise && is_sufficiently_quiet)) > { > /* The number of consecutive DTX frames should be within the allowed > bounds */ > (*nb_no_activity_frames)++; > > if (*nb_no_activity_frames > NB_SPEECH_FRAMES_BEFORE_DTX) > { > if (*nb_no_activity_frames <= (NB_SPEECH_FRAMES_BEFORE_DTX + > MAX_CONSECUTIVE_DTX)) > /* Valid frame for DTX! */ > return 1; > else > (*nb_no_activity_frames) = NB_SPEECH_FRAMES_BEFORE_DTX; > } > } else > (*nb_no_activity_frames) = 0; > > return 0; > } > --------SNIP--------- > > The problem is in the case of function parameter is_silence != 0, the code: > > if (is_silence || (is_noise && is_sufficiently_quiet)) > > is using uninitialized local variables "is_noise" and > "is_sufficiently_quiet". The compiler complaint is valid. > > While this will have no functional effect due to the state of "is_silence", > the problem can be addressed by restructuring the code. The proposed new > code also eliminates unnecessary temporary variables and reduces operators > as well. > > New code: > > --------SNIP--------- > /* Decides if DTX should be turned on (=1) or off (=0) */ > static int decide_dtx_mode(float activity_probability, /* probability > that current frame contains speech/music */ > int *nb_no_activity_frames, /* number of > consecutive frames with no activity */ > opus_val32 peak_signal_energy, /* peak energy of > desired signal detected so far */ > const opus_val16 *pcm, /* input pcm > signal */ > int frame_size, /* frame size */ > int channels, > int is_silence, /* only digital > silence detected in this frame */ > int arch > ) > { > opus_val32 noise_energy; > > if (!is_silence) > { > if( activity_probability < DTX_ACTIVITY_THRESHOLD ) /* is_noise */ > { > noise_energy = compute_frame_energy(pcm, frame_size, channels, > arch); > is_silence = peak_signal_energy >= (PSEUDO_SNR_THRESHOLD * > noise_energy); /* but is_sufficiently_quiet */ > } > } > > if (is_silence) > { > > /* The number of consecutive DTX frames should be within the allowed > bounds */ > (*nb_no_activity_frames)++; > > if (*nb_no_activity_frames > NB_SPEECH_FRAMES_BEFORE_DTX) > { > if (*nb_no_activity_frames <= (NB_SPEECH_FRAMES_BEFORE_DTX + > MAX_CONSECUTIVE_DTX)) > /* Valid frame for DTX! */ > return 1; > else > (*nb_no_activity_frames) = NB_SPEECH_FRAMES_BEFORE_DTX; > } > } else > (*nb_no_activity_frames) = 0; > > return 0; > } > --------SNIP--------- > > > _______________________________________________ > opus mailing list > opus at xiph.org > http://lists.xiph.org/mailman/listinfo/opus >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20170706/df3aa671/attachment.html>