ref: 4a48ce899c9da90e102d451a44689817d5d6d7b6
parent: c84f4866564c7f5891c55653d81dc21c4f6df0ea
author: Jean-Marc Valin <jmvalin@jmvalin.ca>
date: Mon Dec 19 09:33:27 EST 2016
Update draft: addressing WGLC comments
--- a/doc/draft-ietf-codec-opus-update.xml
+++ b/doc/draft-ietf-codec-opus-update.xml
@@ -10,7 +10,7 @@
<?rfc inline="yes"?>
<?rfc compact="yes"?>
<?rfc subcompact="no"?>
-<rfc category="std" docName="draft-ietf-codec-opus-update-04"
+<rfc category="std" docName="draft-ietf-codec-opus-update-05"
ipr="trust200902">
<front>
<title abbrev="Opus Update">Updates to the Opus Audio Codec</title>
@@ -47,7 +47,7 @@
- <date day="21" month="October" year="2016" />
+ <date day="19" month="December" year="2016" />
<abstract>
<t>This document addresses minor issues that were found in the specification
@@ -61,9 +61,24 @@
implementation of the Opus codec that serves as the specification in
<xref target="RFC6716">RFC 6716</xref>. Only issues affecting the decoder are
listed here. An up-to-date implementation of the Opus encoder can be found at
- http://opus-codec.org/. The updated specification remains fully compatible with
- the original specification.
- </t>
+ https://opus-codec.org/.</t>
+ <t>
+ Some of the changes in this document update normative behaviour in a way that requires
+ new test vectors. The English text of the specification is unaffected, only
+ the C implementation is. The updated specification remains fully compatible with
+ the original specification.
+ </t>
+
+ <t>
+ Note: due to RFC formatting conventions, lines exceeding the column width
+ in the patch above are split using a backslash character. The backslashes
+ at the end of a line and the white space at the beginning
+ of the following line are not part of the patch. A properly formatted patch
+ including the three changes above is available at
+ <eref target="https://jmvalin.ca/misc_stuff/opus_update.patch"/>. (EDITOR:
+ change to an ietf.org link when ready)
+ </t>
+
</section>
<section title="Terminology">
@@ -155,7 +170,7 @@
the batch sizes are reasonable enough that none of the issues above
was ever a problem. However, proving that is non-obvious.
</t>
- <t>The code can be fixed by applying the following changes to line 70 of silk/resampler_private_IIR_FIR.c:
+ <t>The code can be fixed by applying the following changes to line 78 of silk/resampler_private_IIR_FIR.c:
</t>
<figure>
<artwork><![CDATA[
@@ -213,15 +228,6 @@
}
]]></artwork>
</figure>
- <t>
- Note: due to RFC formatting conventions, lines exceeding the column width
- in the patch above are split using a backslash character. The backslashes
- at the end of a line and the white space at the beginning
- of the following line are not part of the patch. A properly formatted patch
- including the three changes above is available at
- <eref target="https://jmvalin.ca/misc_stuff/opus_update.patch"/>. (EDITOR:
- change to an ietf.org link when ready)
- </t>
</section>
<section title="Integer wrap-around in inverse gain computation">
@@ -416,7 +422,8 @@
<section anchor="Acknowledgements" title="Acknowledgements">
<t>We would like to thank Juri Aedla for reporting the issue with the parsing of
- the Opus padding.</t>
+ the Opus padding. Also, thanks to Jonathan Lennox and Mark Harris for their
+ feedback on this document.</t>
</section>
</middle>
--- /dev/null
+++ b/doc/opus_update.patch
@@ -1,0 +1,244 @@
+diff --git a/celt/bands.c b/celt/bands.c
+index 6962587..32e1de6 100644
+--- a/celt/bands.c
++++ b/celt/bands.c
+@@ -1234,9 +1234,23 @@ void quant_all_bands(int encode, const CELTMode *m, int start, int end,
+ b = 0;
+ }
+
+- if (resynth && M*eBands[i]-N >= M*eBands[start] && (update_lowband || lowband_offset==0))
++ if (resynth && (M*eBands[i]-N >= M*eBands[start] || i==start+1) && (update_lowband || lowband_offset==0))
+ lowband_offset = i;
+
++ if (i == start+1)
++ {++ int n1, n2;
++ int offset;
++ n1 = M*(eBands[start+1]-eBands[start]);
++ n2 = M*(eBands[start+2]-eBands[start+1]);
++ offset = M*eBands[start];
++ /* Duplicate enough of the first band folding data to be able to fold the second band.
++ Copies no data for CELT-only mode. */
++ OPUS_COPY(&norm[offset+n1], &norm[offset+2*n1 - n2], n2-n1);
++ if (C==2)
++ OPUS_COPY(&norm2[offset+n1], &norm2[offset+2*n1 - n2], n2-n1);
++ }
++
+ tf_change = tf_res[i];
+ if (i>=m->effEBands)
+ {+@@ -1257,7 +1271,7 @@ void quant_all_bands(int encode, const CELTMode *m, int start, int end,
+ fold_start = lowband_offset;
+ while(M*eBands[--fold_start] > effective_lowband);
+ fold_end = lowband_offset-1;
+- while(M*eBands[++fold_end] < effective_lowband+N);
++ while(++fold_end < i && M*eBands[fold_end] < effective_lowband+N);
+ x_cm = y_cm = 0;
+ fold_i = fold_start; do {+ x_cm |= collapse_masks[fold_i*C+0];
+diff --git a/celt/quant_bands.c b/celt/quant_bands.c
+index e5ed9ef..82fb823 100644
+--- a/celt/quant_bands.c
++++ b/celt/quant_bands.c
+@@ -552,6 +552,7 @@ void log2Amp(const CELTMode *m, int start, int end,
+ {+ opus_val16 lg = ADD16(oldEBands[i+c*m->nbEBands],
+ SHL16((opus_val16)eMeans[i],6));
++ lg = MIN32(QCONST32(32.f, 16), lg);
+ eBands[i+c*m->nbEBands] = PSHR32(celt_exp2(lg),4);
+ }
+ for (;i<m->nbEBands;i++)
+diff --git a/silk/LPC_inv_pred_gain.c b/silk/LPC_inv_pred_gain.c
+index 60c439b..6c301da 100644
+--- a/silk/LPC_inv_pred_gain.c
++++ b/silk/LPC_inv_pred_gain.c
+@@ -84,8 +84,13 @@ static opus_int32 LPC_inverse_pred_gain_QA( /* O Returns inver
+
+ /* Update AR coefficient */
+ for( n = 0; n < k; n++ ) {+- tmp_QA = Aold_QA[ n ] - MUL32_FRAC_Q( Aold_QA[ k - n - 1 ], rc_Q31, 31 );
+- Anew_QA[ n ] = MUL32_FRAC_Q( tmp_QA, rc_mult2 , mult2Q );
++ opus_int64 tmp64;
++ tmp_QA = silk_SUB_SAT32( Aold_QA[ n ], MUL32_FRAC_Q( Aold_QA[ k - n - 1 ], rc_Q31, 31 ) );
++ tmp64 = silk_RSHIFT_ROUND64( silk_SMULL( tmp_QA, rc_mult2 ), mult2Q);
++ if( tmp64 > silk_int32_MAX || tmp64 < silk_int32_MIN ) {++ return 0;
++ }
++ Anew_QA[ n ] = ( opus_int32 )tmp64;
+ }
+ }
+
+diff --git a/silk/NLSF_stabilize.c b/silk/NLSF_stabilize.c
+index 979aaba..2ef2398 100644
+--- a/silk/NLSF_stabilize.c
++++ b/silk/NLSF_stabilize.c
+@@ -134,7 +134,7 @@ void silk_NLSF_stabilize(
+
+ /* Keep delta_min distance between the NLSFs */
+ for( i = 1; i < L; i++ )
+- NLSF_Q15[i] = silk_max_int( NLSF_Q15[i], NLSF_Q15[i-1] + NDeltaMin_Q15[i] );
++ NLSF_Q15[i] = silk_max_int( NLSF_Q15[i], silk_ADD_SAT16( NLSF_Q15[i-1], NDeltaMin_Q15[i] ) );
+
+ /* Last NLSF should be no higher than 1 - NDeltaMin[L] */
+ NLSF_Q15[L-1] = silk_min_int( NLSF_Q15[L-1], (1<<15) - NDeltaMin_Q15[L] );
+diff --git a/silk/dec_API.c b/silk/dec_API.c
+index efd7918..21bb7e0 100644
+--- a/silk/dec_API.c
++++ b/silk/dec_API.c
+@@ -72,6 +72,9 @@ opus_int silk_InitDecoder( /* O Returns error co
+ for( n = 0; n < DECODER_NUM_CHANNELS; n++ ) {+ ret = silk_init_decoder( &channel_state[ n ] );
+ }
++ silk_memset(&((silk_decoder *)decState)->sStereo, 0, sizeof(((silk_decoder *)decState)->sStereo));
++ /* Not strictly needed, but it's cleaner that way */
++ ((silk_decoder *)decState)->prev_decode_only_middle = 0;
+
+ return ret;
+ }
+diff --git a/silk/resampler_private_IIR_FIR.c b/silk/resampler_private_IIR_FIR.c
+index dbd6d9a..91a43aa 100644
+--- a/silk/resampler_private_IIR_FIR.c
++++ b/silk/resampler_private_IIR_FIR.c
+@@ -75,10 +75,10 @@ void silk_resampler_private_IIR_FIR(
+ silk_resampler_state_struct *S = (silk_resampler_state_struct *)SS;
+ opus_int32 nSamplesIn;
+ opus_int32 max_index_Q16, index_increment_Q16;
+- opus_int16 buf[ RESAMPLER_MAX_BATCH_SIZE_IN + RESAMPLER_ORDER_FIR_12 ];
++ opus_int16 buf[ 2*RESAMPLER_MAX_BATCH_SIZE_IN + RESAMPLER_ORDER_FIR_12 ];
+
+ /* Copy buffered samples to start of buffer */
+- silk_memcpy( buf, S->sFIR, RESAMPLER_ORDER_FIR_12 * sizeof( opus_int32 ) );
++ silk_memcpy( buf, S->sFIR, RESAMPLER_ORDER_FIR_12 * sizeof( opus_int16 ) );
+
+ /* Iterate over blocks of frameSizeIn input samples */
+ index_increment_Q16 = S->invRatio_Q16;
+@@ -95,13 +95,13 @@ void silk_resampler_private_IIR_FIR(
+
+ if( inLen > 0 ) {+ /* More iterations to do; copy last part of filtered signal to beginning of buffer */
+- silk_memcpy( buf, &buf[ nSamplesIn << 1 ], RESAMPLER_ORDER_FIR_12 * sizeof( opus_int32 ) );
++ silk_memmove( buf, &buf[ nSamplesIn << 1 ], RESAMPLER_ORDER_FIR_12 * sizeof( opus_int16 ) );
+ } else {+ break;
+ }
+ }
+
+ /* Copy last part of filtered signal to the state for the next call */
+- silk_memcpy( S->sFIR, &buf[ nSamplesIn << 1 ], RESAMPLER_ORDER_FIR_12 * sizeof( opus_int32 ) );
++ silk_memcpy( S->sFIR, &buf[ nSamplesIn << 1 ], RESAMPLER_ORDER_FIR_12 * sizeof( opus_int16 ) );
+ }
+
+diff --git a/src/opus_decoder.c b/src/opus_decoder.c
+index 0cc56f8..8a30fbc 100644
+--- a/src/opus_decoder.c
++++ b/src/opus_decoder.c
+@@ -595,16 +595,14 @@ static int opus_packet_parse_impl(const unsigned char *data, int len,
+ /* Padding flag is bit 6 */
+ if (ch&0x40)
+ {+- int padding=0;
+ int p;
+ do {+ if (len<=0)
+ return OPUS_INVALID_PACKET;
+ p = *data++;
+ len--;
+- padding += p==255 ? 254: p;
++ len -= p==255 ? 254: p;
+ } while (p==255);
+- len -= padding;
+ }
+ if (len<0)
+ return OPUS_INVALID_PACKET;
+diff --git a/run_vectors.sh b/run_vectors.sh
+index 7cd23ed..4841b0a 100755
+--- a/run_vectors.sh
++++ b/run_vectors.sh
+@@ -1,3 +1,5 @@
++#!/bin/sh
++#
+ # Copyright (c) 2011-2012 IETF Trust, Jean-Marc Valin. All rights reserved.
+ #
+ # This file is extracted from RFC6716. Please see that RFC for additional
+@@ -31,10 +33,8 @@
+ # NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+ # SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+-#!/bin/sh
+-
+-rm logs_mono.txt
+-rm logs_stereo.txt
++rm -f logs_mono.txt logs_mono2.txt
++rm -f logs_stereo.txt logs_stereo2.txt
+
+ if [ "$#" -ne "3" ]; then
+ echo "usage: run_vectors.sh <exec path> <vector path> <rate>"
+@@ -45,18 +45,23 @@ CMD_PATH=$1
+ VECTOR_PATH=$2
+ RATE=$3
+
+-OPUS_DEMO=$CMD_PATH/opus_demo
+-OPUS_COMPARE=$CMD_PATH/opus_compare
++: ${OPUS_DEMO:=$CMD_PATH/opus_demo}++: ${OPUS_COMPARE:=$CMD_PATH/opus_compare}+
+ if [ -d $VECTOR_PATH ]; then
+ echo Test vectors found in $VECTOR_PATH
+ else
+ echo No test vectors found
+- #Don't make the test fail here because the test vectors will be
+- #distributed separately
++ #Don't make the test fail here because the test vectors
++ #will be distributed separately
+ exit 0
+ fi
+
++if [ ! -x $OPUS_COMPARE ]; then
++ echo ERROR: Compare program not found: $OPUS_COMPARE
++ exit 1
++fi
++
+ if [ -x $OPUS_DEMO ]; then
+ echo Decoding with $OPUS_DEMO
+ else
+@@ -82,9 +87,11 @@ do
+ echo ERROR: decoding failed
+ exit 1
+ fi
+- $OPUS_COMPARE -r $RATE $VECTOR_PATH/testvector$file.dec tmp.out >> logs_mono.txt 2>&1
++ $OPUS_COMPARE -r $RATE $VECTOR_PATH/testvector${file}.dec tmp.out >> logs_mono.txt 2>&1+ float_ret=$?
+- if [ "$float_ret" -eq "0" ]; then
++ $OPUS_COMPARE -r $RATE $VECTOR_PATH/testvector${file}m.dec tmp.out >> logs_mono2.txt 2>&1++ float_ret2=$?
++ if [ "$float_ret" -eq "0" ] || [ "$float_ret2" -eq "0" ]; then
+ echo output matches reference
+ else
+ echo ERROR: output does not match reference
+@@ -111,9 +118,11 @@ do
+ echo ERROR: decoding failed
+ exit 1
+ fi
+- $OPUS_COMPARE -s -r $RATE $VECTOR_PATH/testvector$file.dec tmp.out >> logs_stereo.txt 2>&1
++ $OPUS_COMPARE -s -r $RATE $VECTOR_PATH/testvector${file}.dec tmp.out >> logs_stereo.txt 2>&1+ float_ret=$?
+- if [ "$float_ret" -eq "0" ]; then
++ $OPUS_COMPARE -s -r $RATE $VECTOR_PATH/testvector${file}m.dec tmp.out >> logs_stereo2.txt 2>&1++ float_ret2=$?
++ if [ "$float_ret" -eq "0" ] || [ "$float_ret2" -eq "0" ]; then
+ echo output matches reference
+ else
+ echo ERROR: output does not match reference
+@@ -125,5 +134,10 @@ done
+
+
+ echo All tests have passed successfully
+-grep quality logs_mono.txt | awk '{sum+=$4}END{print "Average mono quality is", sum/NR, "%"}'+-grep quality logs_stereo.txt | awk '{sum+=$4}END{print "Average stereo quality is", sum/NR, "%"}'++mono1=`grep quality logs_mono.txt | awk '{sum+=$4}END{if (NR == 12) sum /= 12; else sum = 0; print sum}'`++mono2=`grep quality logs_mono2.txt | awk '{sum+=$4}END{if (NR == 12) sum /= 12; else sum = 0; print sum}'`++echo $mono1 $mono2 | awk '{if ($2 > $1) $1 = $2; print "Average mono quality is", $1, "%"}'++
++stereo1=`grep quality logs_stereo.txt | awk '{sum+=$4}END{if (NR == 12) sum /= 12; else sum = 0; print sum}'`++stereo2=`grep quality logs_stereo2.txt | awk '{sum+=$4}END{if (NR == 12) sum /= 12; else sum = 0; print sum}'`++echo $stereo1 $stereo2 | awk '{if ($2 > $1) $1 = $2; print "Average stereo quality is", $1, "%"}'