<div dir="ltr">Thank you Mark.<div><br><div>I agree and have now updated the pull request with a new commit, addressing your comments.</div><div>Please take a look.</div><div><br></div><div>/Gustaf</div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 5 Apr 2019 at 11:41, Mark Harris <<a href="mailto:mark.hsj@gmail.com">mark.hsj@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 2019-04-01 3:37, Gustaf Ullberg wrote:<br>
> Hi everyone,<br>
> <br>
> Some time ago, I sent a pull request<br>
> <<a href="https://github.com/xiph/opus/pull/107" rel="noreferrer" target="_blank">https://github.com/xiph/opus/pull/107</a>> to the Opus github page.<br>
> Jean-Marc asked me to post it to the mailing list so everyone can have a<br>
> look at it.<br>
> <br>
> You can find the description and code changes below. Please let me know<br>
> if you have any questions or concerns.<br>
> <br>
> Best regards<br>
> Gustaf Ullberg<br>
> <br>
> In WebRTC, we would like to be able to differentiate between "normal"<br>
> frames and the comfort noise update frames that are encoded before and<br>
> in-between DTX frames. These CNG update frames could then be flagged for<br>
> routing or statistics reasons. Such flagging could also make it easier<br>
> for a jitterbuffer to differ DTX from packet loss.<br>
> <br>
> This PR implements an encoder CTL named OPUS_GET_IN_DTX. OPUS_GET_IN_DTX<br>
> returns 1 if the last encoded frame was either a DTX frame, or a CNG<br>
> update frame that is sent every 420 ms between the DTX frames.<br>
> <br>
> This is achieved by checking if the number of consecutive inactive<br>
> frames (nb_no_activity_frames) is greater or equal to<br>
> NB_SPEECH_FRAMES_BEFORE_DTX.<br>
> <br>
> <br>
> diff --git a/include/opus_defines.h b/include/opus_defines.h<br>
> index fbf5d0eb..e35114e4 100644<br>
> --- a/include/opus_defines.h<br>
> +++ b/include/opus_defines.h<br>
> @@ -168,6 +168,7 @@ extern "C" {<br>
>  /* Don't use 4045, it's already taken by OPUS_GET_GAIN_REQUEST */<br>
>  #define OPUS_SET_PHASE_INVERSION_DISABLED_REQUEST 4046<br>
>  #define OPUS_GET_PHASE_INVERSION_DISABLED_REQUEST 4047<br>
> +#define OPUS_GET_IN_DTX_REQUEST              4049<br>
>  <br>
>  /** Defines for the presence of extended APIs. */<br>
>  #define OPUS_HAVE_OPUS_PROJECTION_H<br>
> @@ -715,6 +716,16 @@ extern "C" {<br>
>    * </dl><br>
>    * @hideinitializer */<br>
>  #define OPUS_GET_PHASE_INVERSION_DISABLED(x)<br>
> OPUS_GET_PHASE_INVERSION_DISABLED_REQUEST, __opus_check_int_ptr(x)<br>
> +/** Gets the DTX state of the encoder.<br>
> +  * Returns wheter the last encoded frame was either a comfort noise update<br>
<br>
<br>
s/wheter/whether/<br>
<br>
<br>
> +  * during DTX or not encoded because of DTX.<br>
> +  * @param[out] x <tt>opus_int32 *</tt>: Returns one of the following<br>
> values:<br>
> +  * <dl><br>
> +  * <dt>0</dt><dd>The encoder is not in DTX.</dd><br>
> +  * <dt>1</dt><dd>The encoder is in DTX.</dd><br>
> +  * </dl><br>
> +  * @hideinitializer */<br>
> +#define OPUS_GET_IN_DTX(x) OPUS_GET_IN_DTX_REQUEST, __opus_check_int_ptr(x)<br>
>  <br>
>  /**@}*/<br>
>  <br>
> diff --git a/src/opus_encoder.c b/src/opus_encoder.c<br>
> index cbeb40ae..0d84737a 100644<br>
> --- a/src/opus_encoder.c<br>
> +++ b/src/opus_encoder.c<br>
> @@ -2725,6 +2725,27 @@ int opus_encoder_ctl(OpusEncoder *st, int<br>
> request, ...)<br>
>              ret = celt_encoder_ctl(celt_enc, OPUS_SET_ENERGY_MASK(value));<br>
>          }<br>
>          break;<br>
> +        case OPUS_GET_IN_DTX_REQUEST:<br>
> +        {<br>
> +            opus_int32 *value = va_arg(ap, opus_int32*);<br>
> +            if (!value)<br>
> +            {<br>
> +                goto bad_arg;<br>
> +            }<br>
> +            *value = 0;<br>
> +            if (st->silk_mode.useDTX) {<br>
> +                /* DTX determined by Silk. */<br>
> +                void *silk_enc = (char*)st+st->silk_enc_offset;<br>
> +                *value =<br>
> ((silk_encoder*)silk_enc)->state_Fxx[0].sCmn.noSpeechCounter >=<br>
> NB_SPEECH_FRAMES_BEFORE_DTX;<br>
> +            }<br>
<br>
<br>
It looks like this does not have the same behavior as the code that<br>
decides whether to produce a DTX frame.  For example, in the case of<br>
stereo the SILK DTX code considers both channels, but this code looks<br>
only at state_Fxx[0].  Shouldn't the behavior match?<br>
<br>
 - Mark<br>
<br>
<br>
<br>
> +#ifndef DISABLE_FLOAT_API<br>
> +            else if (st->use_dtx) {<br>
> +                /* DTX determined by Opus. */<br>
> +                *value = st->nb_no_activity_frames >=<br>
> NB_SPEECH_FRAMES_BEFORE_DTX;<br>
> +            }<br>
> +#endif<br>
> +        }<br>
> +        break;<br>
>  <br>
>          case CELT_GET_MODE_REQUEST:<br>
>          {<br>
> <br>
> <br>
</blockquote></div>