[opus] Suggested patch, opus_encoder.c, decide_dtx_mode()

[Felicia Lim]

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>